-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[5.4.0] Allow sync adapters to save/load/delete multiple titles at once #9117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
- backward compatible with existing syncers - this commit is the backward compatible part
- this commit has the code for handling multiple titles
✅ Deploy Preview for tiddlywiki-previews ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Confirmed: Arlen22 has already signed the Contributor License Agreement (see contributing.md) |
📊 Build Size Comparison:
|
Branch | Size |
---|---|
Base (master) | 2529.0 KB |
PR | 2533.4 KB |
Diff: ⬆️ Increase: +4.4 KB
I did not read or test the code yet. -- But is this a preparation for the MWS plugin? For "batch" processing |
I'm not sure. Server-sent events and other more advanced syncing mechanisms may introduce additional complexity that the syncer isn't built for. But yes, this allows batch processing and for now MWS would probably use it. |
@Jermolene I'm marking this for 5.3.7 because the changes are quite simple and I'm targeting es5. It's up to you whether you want to include it this last minute. |
f28117c
to
5998755
Compare
I've added this to MWS for now so if anyone wants to test how it works you can open a codespace on this commit of MWS and then run the following commands npm install
npm run prisma:generate
npm start init-store
npm start |
I support addressing this need, but would prefer if we delayed until v5.4.0 to allow for further testing and potential refinement. I have a GitHub sync adaptor that has been pending a rewrite to get around exactly these limitations and would benefit from batching updates to avoid the chances of rate limiting by the API. In my use case it would be most beneficial to be able to batch saves and deletes to the server in a single API call (a single git commit). I am just recovering from knee surgery and will likely need a week or two before I am able to properly test these changes. |
core/modules/syncer.js
Outdated
SaveAllTiddlersTask.prototype.run = function(callback) { | ||
var self = this; | ||
this.syncer.logger.log("Dispatching 'save all' task:",this.titles); | ||
var changeCounts = this.titles.reduce(function (n,e) { n[e] = self.syncer.wiki.getChangeCount(e); return n; }, {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm. Is it really needed to use single character variable name like n
and e
to make code unreadable for humans.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change the n to r (short for reduction) and change e to title. Does that work?
804c35c
to
5998755
Compare
I'm adding a subscribe function so the syncadapter can request the syncer to poll. This lets us keep the existing pattern of (mostly) not telling the syncadapter what syncer is using it. |
Since 5.3.7 will be released on 7th of July, I don't think it will merge PR with new features. IMO this PR should target 5.4.0. |
Agreed, that's sooner than I was thinking, so yeah. |
This PR adds save/load/delete tasks which allow a sync adapter to handle all pending saves/loads/deletes at once. This allows optimizations for certain scenarios where it is preferred to bring everything up to date at once rather than trickling tiddler operations through, potentially blocking subsequent syncing for much longer than intended.
Currently the syncer saves one tiddler at a time until all tiddlers are saved, then it syncs from the server, then it deletes all tiddlers that need to be deleted one at a time, then it loads all needed tiddlers one at a time. This check occurs after each operation, so if we are in the middle of loading 3 tiddlers, and suddenly need to save 10k tiddlers, the remaining tiddlers will not be loaded, and no further sync from server will occur, until all 10k tiddlers are saved. This is also rate-limited at 4 per second, although that can be changed, but in practice most web requests take at least that long if not longer.
The disadvantage of this is that most, if not all, storage locations could save 10k tiddlers in a few seconds at the longest, but saving one tiddler at a time tends to take much longer.
This PR allows sync adapters to opt into saving, loading, and deleting multiple tiddlers at once.
This adds the following to the existing sync adapter interface
I basically just took the existing tasks and separated the callback code into
onNext
,onDone
, andonError
.onNext
is called for each title that is successfully processed to allow the syncer to update its own state. This matches existing behavior. For tiddlers where an error occurs,onNext
should not be called. The existing syncer behavior does not care about tiddlers that failed because it will just try them again.onError
andonDone
call the task callback.I believe the behavior should be exactly identical to single tiddler tasks.