[adb] [sync] Make sync target dynamic #1281
Conversation
d77c08f to
3dbc2dd
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements dynamic sync targets for the ADB sync client, allowing the sync target (hash and operation bounds) to be updated during the synchronization process. The key changes include factoring out a SyncTarget struct to encapsulate sync parameters, adding an update channel mechanism for receiving target changes, and implementing validation to ensure targets only move "forward" (bounds increase and hash changes).
- Introduces
SyncTargetstruct to group hash and operation bounds - Adds channel-based mechanism for receiving sync target updates during sync
- Implements validation to prevent backward movement of sync targets
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| storage/src/adb/any/sync/mod.rs | Adds SyncTarget struct, update channel types, new error variants, and sync_with_updates function |
| storage/src/adb/any/sync/client.rs | Refactors Config to use SyncTarget, adds update handling logic, and implements target validation with comprehensive tests |
Comments suppressed due to low confidence (2)
storage/src/adb/any/sync/client.rs:263
- [nitpick] The variable name
update_with trailing underscore is unconventional. Consider using a more descriptive name likenew_updateorpending_update.
while let Ok(Some(update_)) = receiver.try_next() {
storage/src/adb/any/sync/client.rs:591
- [nitpick] Empty line added in the middle of imports section. This should be removed to maintain consistent formatting.
use super::*;
storage/src/adb/any/sync/resolver.rs
Outdated
| } | ||
| } | ||
|
|
||
| #[cfg(test)] |
There was a problem hiding this comment.
We could compile this outside tests, too, but we only use this in tests
There was a problem hiding this comment.
On further thought, all of these implementations are only used in tests.
I removed the #[cfg(test)] here for consistency. Should all of these implementations be under a #[cfg(test)]?
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #1281 +/- ##
==========================================
+ Coverage 91.13% 91.16% +0.03%
==========================================
Files 246 246
Lines 60746 61157 +411
==========================================
+ Hits 55360 55753 +393
- Misses 5386 5404 +18
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| lower_bound_ops: oldest_retained_loc, | ||
| upper_bound_ops: latest_op_loc, | ||
| target: SyncTarget { | ||
| hash: target_hash, |
There was a problem hiding this comment.
root might be a better name if this is indeed some root hash? (i use root implicitly for "root_hash" elsewhere)
There was a problem hiding this comment.
also been trying to use "digest" instead of "hash" (though probably haven't updated all occurrences, as was originally using "hash" throughout.)
There was a problem hiding this comment.
Per Patrick's request, I've renamed all "root_hash" and "root_digest" to just "root" in variable and function names in #1299
| log_size, | ||
| old_bounds = ?config.target, | ||
| new_bounds = ?new_target, | ||
| "Log is stale, reinitializing" |
There was a problem hiding this comment.
super nitty question but do we want to standardize on capitalizing the first word of any log line? I think I've been leaving it all lowercase unless it's really a full sentence with periods.
There was a problem hiding this comment.
(I've also been doing all lowercase)
There was a problem hiding this comment.
Yup, you're right. #1295 changes the logs in adb to start with lowercase.
This PR resolves #1190.
SyncTargetupdate_receiverchannel to client state which receives sync target updates