Repository navigation
[WIP] Refactored sync to batch reads - #7759
lougeniaC64 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
this looks like the correct shape. Seeing if we can do that 1xretry would be perfect too given this is looking so reasonable - we'd treat 2 failures as a "harder" error and would usually end up with a successful sync, which is really where we want to end up - doing that as a followup would be ok, but you might find it fairly reasonable to do here.
| /// already have been passed to `on_batch`, so callers must not advance any | ||
| /// timestamps unless this returns `Ok`. | ||
| /// - `on_batch` is always called at least once, even if the collection is empty, | ||
| /// because some engines (eg, tabs) do housekeeping in `stage_incoming`. |
There was a problem hiding this comment.
worth exploring if there is a better fix here - that seems like a tabs smell rather than something we should handle here. np if that gets complicated though.
There was a problem hiding this comment.
it looks like I'd need to move remove_stale_clients and remove_old_pending_closures out of the tab engine's implementation of stage_incoming to limit on_batch to only be called when the collection isn't empty.
I put those changes in the the tabs-engine-refactor branch (the specific commit for these changes is here). It's not a big commit but enough changes for the tabs engine that I think separating it from the incident response is probably worthwhile.
Anyway, I'm going to add tests for this PR tomorrow and put it out for review.
| telem_engine: &mut telemetry::Engine, | ||
| ) -> Result<(), Error> { | ||
| match result { | ||
| Err(e @ Error::CollectionChangedDuringDownload) => { |
There was a problem hiding this comment.
huh, never seen an "@ binding" before! :)
fd29ae2 to
2027a0f
Compare
|
2027a0f to
7e25495
Compare
Pull Request checklist
[ci full]to the PR title.