[5.4.0] Allow sync adapters to save/load/delete multiple titles at once
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
interface SyncAdapter {
/** Callback for the sync adapter to call to request a poll */
subscribe?(cb: () => void): void;
saveTiddlers?(options: {
syncer: Syncer,
tiddlers: Tiddler[],
onNext: (title: string, adaptorInfo: any, revision: string) => void,
onDone: () => void,
onError: (err: Error) => void
}): void;
loadTiddlers?(options: {
syncer: Syncer,
titles: string[],
onNext: (tiddlerFields: TiddlerFields) => void,
onDone: () => void,
onError: (err: Error) => void
}): void;
deleteTiddlers?(options: {
syncer: Syncer,
titles: string[],
onNext: (title: string) => void,
onDone: () => void,
onError: (err: Error) => void
}): void;
}
I basically just took the existing tasks and separated the callback code into onNext, onDone, and onError.
onNextis 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,onNextshould not be called. The existing syncer behavior does not care about tiddlers that failed because it will just try them again.onErrorandonDonecall the task callback.
I believe the behavior should be exactly identical to single tiddler tasks.
Deploy Preview for tiddlywiki-previews ready!
| Name | Link |
|---|---|
| Latest commit | d6bd6b689708ffd56af40fdb9c4e0c44fcdea3b4 |
| Latest deploy log | https://app.netlify.com/projects/tiddlywiki-previews/deploys/69441abf9cd229000703dece |
| Deploy Preview | https://deploy-preview-9117--tiddlywiki-previews.netlify.app |
| Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
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: empty.html
| 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 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.
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.
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.
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 also takes care of #3227.
@saqimtiaz ... IMO should be scheduled for v5.4.0
@Jermolene @saqimtiaz where are we with this and can we merge it?
Hi @Arlen22 I notice that this PR weighs in at +4KB, which I think is problematic given that it doesn't it doesn't provide benefits with the integrated syncadaptor.
I am fairly sure that the task classes used by the syncer are encapsulated to the degree that we can radically change the implementation, and don't need to worry about backwards compatibility for this internal interface. Does that give and scope for a more radical refactoring that has better code reuse?
📊 Build Size Comparison: empty.html
| Branch | Size |
|---|---|
| Base (master) | 2529.0 KB |
| PR | 2529.5 KB |
Diff: ⬆️ Increase: +0.5 KB
⚠️ Change Note Status
This PR appears to contain code changes but doesn't include a change note.
Please add a change note by creating a .tid file in editions/tw5.com/tiddlers/releasenotes/<version>/
📚 Documentation: Release Notes and Changes
💡 Note: If this is a documentation-only change, you can ignore this message.
@Jermolene good point. I've combined them now.
Thanks @Arlen22, that's a good improvements in the code size delta. However, it is still 2KB so I wondered if there was any scope for further refactoring so that syncadaptor plugins can themselves implement the syncer task classes. We'd add a new syncadaptor method that the syncer invokes during initialisation. The syncadaptor would return its own implementation of the syncer task classes, overwriting the default null implementation of the multi-tiddler classes.
@Jermolene well if I'm going to refactor it that much I would just get rid of them and dump what's left straight into the function that calls them because it's already so paired down there's not much left anymore. I'll take a look at it today.
@Jermolene I simplified the code to just use functions instead of entire classes. I think that's about the simplest I'm going to be able to get it for now.