refactor: remove recon client/server and send empty ranges when we don't need anything #509
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
We no longer need exclusive access to recon so removing the client/server loop abstraction as it currently acts as an extra layer to update. I added Clone to a couple things and had to adjust generics.. I think it's okay but probably merits a close review. The tests now use worker threads instead of spawning the server in the background. This changed the order that things get sent, but it looks like all the same messages are exchanged.
dav1do
temporarily deployed
to
github-tests-2024
GitHub Actions
Inactive
dav1do
changed the title
dav1do
temporarily deployed
to
github-tests-2024
GitHub Actions
Inactive
… the remote was missing a range this appears to have reduced round trips in tests without changing the synchronization result
dav1do
force-pushed
the
feat/aes-306-recon-no-read-writes
branch
from
925cd01
to
4ea371a
Compare
dav1do
temporarily deployed
to
github-tests-2024
GitHub Actions
Inactive
this is a modification to the previous commit that actually batches writes until the conversation finishes, however, it changes the semantic a bit as we no longer send the entire hash range. We could probably recognize that range and persist to make sure we're in sync at the end, but if we're okay with this, we won't do that. It does make the tests look a bit odd as we get values but our state isn't updated until the conversation finishes and we're in sync. I can change the batch size to 1 if we want this but would prefer to see the peer state updated in the expect output
dav1do
temporarily deployed
to
github-tests-2024
GitHub Actions
Inactive
nathanielc
requested changes
Aug 30, 2024
There was a problem hiding this comment.
This is great, happy to see the simplification here.
I was concerned about the new reply_with field in remote missing as it didn't quite fit in the Recon model. I played around with it and found a simple way to avoid having to change the SyncState enum.
Here is a PR to this branch that shows my thinking. #510
Yeah, that's much better. I agree the |
dav1do
temporarily deployed
to
github-tests-2024
GitHub Actions
Inactive
dav1do
temporarily deployed
to
github-tests-2024
GitHub Actions
Inactive
nathanielc
approved these changes
Aug 30, 2024
Merged
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.
This has two main changes, the first is to remove the recon client/server and the second is to reply with empty ranges when possible.
We no longer need exclusive access to recon so removing the client/server loop abstraction as it currently acts as an extra layer to update. I added Clone to a couple things and had to adjust generics.. I think it's okay but probably merits a close review. The tests now use worker threads instead of spawning the server in the background. This changed the order that things get sent, but it looks like all the same messages are exchanged.
When the remote is missing a range, we now send an empty response when we don't want anything. We still have to send some ranges in these cases if we're missing the bounding key and need to discover it. This seems to have reduced the round trips in some cases. The tests for this change was updated in the second commit, so looking at each commit to see the test changes is probably wise (or I can split it into two PRs if preferred). The goal of this change is to remove the requirement that the peer "read their writes". Previously, we would send a bunch of values and a hash and they would have to persist them before they could reply to indicate we're in sync. Now we assume the range is done and move on, coming back in a future conversation if necessary.