feat(sync): track incoming sync requests, allow subscriptions without sync, close inactive replicas #1491
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Frando
force-pushed
the
sync-accept-in-engine
branch
from
982263b to
d92142b
Compare
Frando
changed the title
Frando
force-pushed
the
sync-accept-in-engine
branch
from
065a002 to
ff8425c
Compare
Frando
changed the title
Frando
changed the title
3 tasks
| |namespace, _| { | ||
| futures::future::ready( | ||
| bob_replica_store_task | ||
| .open_replica(&namespace) |
There was a problem hiding this comment.
this is a blocking call, we should probably mark it as such
There was a problem hiding this comment.
deferred for later: #1502
| &mut bob_writer, | ||
| &mut bob_reader, | ||
| bob_store, | ||
| |namespace, _| { | ||
| futures::future::ready(bob_store.open_replica(&namespace).map(Into::into)) |
dignifiedquire
approved these changes
Sep 20, 2023
Frando
changed the title
…ve sync (#1495) ## Description Based on #1491 This adds two related features that were missing so far: * Add `iroh_sync::store::close_replica` to remove a replica instance from the store-internal cache of open replicas. This is needed because otherwise it grows unbounded (for anchor nodes especially). * Properly handle open replicas in the `LiveSync`, and through this allow to subscribe to replica events for replicas that are not in the set of actively syncing replicas. This was not possible before (you could only subscribe to replicas that are active for sync) * Add test for subscribing without sync ## Notes & open questions <!-- Any notes, remarks or open questions you have to make about the PR. --> ## Change checklist - [x] Self-review. - [x] Documentation updates if relevant. - [x] Tests if relevant.
3 tasks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 21, 2023
## Description On `main` the test `sync_full_basic` is flakey. This PR (hopefully) fixes it. The reason was: We have the situation that two peers initiate connections to each other at (roughly) the same time. In #1491 this was sometimes prevented, but not reliably. This PR fixes it by: * When connecting, we set a `SyncState::Dialing` for the `(namespace, peer)` * When accepting, if our own state is `Dialing` for the incoming request for `(namespace, peer)` we compare our peer id with that of the incoming request, and abort if ours is higher (doesn't matter which way, we care about a predictable outcome only * Through this, only one of the two simoultanoues connections will survive * Also added a `Abort` frame to the wire protocol to transfer to inform the dialer about the reason of the declined request, which is either "we do double sync, and will take the other conn" (`AlreadySyncing`) or "this replica is not here" (`NotAvailable`) This PR also: * Further improves logging * Improves errors ## Notes & open questions <!-- Any notes, remarks or open questions you have to make about the PR. --> ## Change checklist - [x] Self-review. - [x] Documentation updates if relevant. - [x] Tests if relevant. --------- Co-authored-by: Diva M <divma@protonmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR adds a few missing parts for sync in iroh:
LiveSyncand close them once they're not needed anymore. Therefore addsiroh_sync::store::close_replicato remove a replica instance from the store-internal cache of open replicas. This is needed because otherwise it grows unbounded (for anchor nodes especially).LiveSyncactor and handle them there. Before, incoming connections for sync were always processed if a replica with the requestedNamespaceIdis found in the local replica store. This PR changes this mechanism so that incoming sync connections are passed into the sync engine first. This leads to the changes that motivate this PR:start_sync). Note that this is not persisted at the moment, but should be (separate change though).LiveEvent:.SyncFinishedthat reports all available informationNotes & open questions
Do we want to track history of syncs internally or leave it up to the app via the events? Not sure myself.
The diff of
sync_engine/live.rsis a little hard to read, because there's a change fromTopicIdtoNamespaceIdin a few places that is mostly unrelated. Sorry for the mixup, but it makes sense to track everything byNamespaceIdand it ended up in a commit here.Logging is much improved in this PR.
Maybe we can improve the code flow a little to make things more obvious.
Change checklist