Diff transfer improvements #3645

ffuugoo · 2024-02-19T18:33:47Z

Minor fixes/improvements/cleanups for diff transfer related code.

Tracked in: #3477

All Submissions:

Contributions should target the dev branch. Did you create your branch from dev?
Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

Does your submission pass tests?
Have you formatted your code locally using cargo +nightly fmt --all command prior to submission?
Have you checked your code using cargo clippy --all --all-features command?

Changes to Core Features:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your core changes, as applicable?
Have you successfully ran tests with your changes locally?

timvisee

Some of my comments on the current state.

lib/collection/src/shards/local_shard/mod.rs

lib/collection/src/shards/shard.rs

lib/collection/src/wal.rs

lib/collection/src/wal_delta.rs

ffuugoo · 2024-02-23T14:16:34Z

Resolved all previous comments, organized history, rebased on dev, added a few more improvements. Also removed proto file changes (will open a separate PR in a moment).

timvisee

Some comments (and ⛏️s) on a quick pass.

timvisee · 2024-02-23T17:06:15Z

lib/collection/src/shards/local_shard/clock_map.rs

-            if clock_tag.clock_tick <= *tick {
+    /// Remove a clock referenced by the clock tag from this recovery point, if the clock is
+    /// *newer or equal* than the tick in the tag.
+    pub fn remove_clock_if_newer_than_or_equal_to_tag(&mut self, tag: ClockTag) {


In my opinion names like this are a bit too explicit:

'clock's are obsolete because the only thing we manage is clocks

'tag' is obsolete because the parameter already clarifies this

This screams Java

Kinda agree about "clock" part... but not about the "tag".

The problem with remove_if_newer_or_equal is that it's not 100% clear if:

we remove clock, if clock is newer than tag

or we remove clock, if tag is newer than clock

It's more reasonable to read clock_map.remove_if_newer_or_equal(tag) as "remove clock, if clock is newer than tag". But both cases are "reasonable enough". And so you have to go check documentation/code to verify what the actual behavior is. If we add _tag at the end, it's clear just from the method name.

I don't see how it is clear. The argument name does not change the fact, that you can read "if newer or equal" both ways. What are we comparing to what? clock > tag or tag > clock?

It does make more sense to read "left to right", e.g., clock > tag, but, like, do you absolutely always do the most logical thing, or are you, like, human? :D

I mean, when you read this code for the first time, how can you be sure that it's clock > tag? You have to go read the documentation/code to verify that it is indeed clock > tag. Using newer_than_or_equal_to_tag clarifies clock > tag.

It is clear to you, because you've been working with it for the last two (three?) weeks.

remove_ge_tag?

The only reasonable assumption I can make about that is: we remove clocks, if greater or equal to the given tag.

Or retain_old_clocks(tag) with a comment describing the exact condition.

remove_ge_tag?
The only reasonable assumption I can make about that is: we remove clocks, if greater or equal to the given tag.

You still can read it both ways. Like, it literally reads as "remove greater or equal tag". Which is a lot like "remove if tag is greater of equal". Why are you so against the conjunctions? Just add remove_ge_to_tag, and it's clear. Though, I'd still prefer fully verbose one. Not a huge fan of ge and such.

Or retain_old_clocks(tag) with a comment describing the exact condition.

That's exactly what I don't want. I don't want to have to go read the comment. I want it to be obvious when you just read the method name. :/

lib/collection/src/shards/local_shard/mod.rs

timvisee · 2024-02-23T17:10:25Z

lib/collection/src/shards/local_shard/shard_ops.rs


            channel_permit.send(UpdateSignal::Operation(OperationData {
                op_num: operation_id,
                operation: operation.operation,
                sender: callback_sender,
                wait,
            }));
-            drop(wal_lock);


I put it here so we don't make the same mistake again 😄

Ok, I kinda get your point now, but, IMO, it's not very effective and this part simply requires an explanation comment (with a lot of exclamation marks in it, maybe :D).

It's still super easy to just look at this code, see the drop, think "WTF is this here for?" and just remove the drop and refactor the wal_lock away same way we did earlier.

If we add the comment, then it will explain that the drop is intentional, and we have to hold the wal_lock for the duration of send... but at this point there's not much use in doing explicit drop, IMO.

If I see a drop I always think: it must be there for a reason, lets be very careful about it.

Comments don't do the same to me while refactoring.

But anyway, fine with both.

Hmmm...... I usually do as well, but in this case it's kinda obvious that it's the end of the scope, so I usually assume it's just a leftover from a refactoring or something.

lib/collection/src/shards/local_shard/mod.rs

timvisee · 2024-02-23T17:16:54Z

lib/collection/src/wal_delta.rs

@@ -154,10 +176,10 @@ impl RecoverableWal {
 /// If `None` - the remote WAL is already equal, and we don't have to send any records.
 /// If `Err` - no delta can be resolved.
 fn resolve_wal_delta(
+    operations: impl DoubleEndedIterator<Item = (u64, Option<ClockTag>)>,


This one is potentially dangerous, because we say nothing about the order we expect.

...and cleanup `clock_map` module documentation

...to `newest_observed_clocks` and `oldest_resolvable_clocks`

...instead of WAL, to simplify unit-testing

ffuugoo · 2024-02-27T19:20:15Z

Ok, so the failing CI was my own fault (obviously, lol). It's fixed now. I think we can give it one last pass and merge.

(Forgot to rename newest_observer/oldest_resolvable. Will do soon-ish.)

lib/collection/src/wal.rs

...to `newest_clocks` and `oldest_clocks`

ffuugoo mentioned this pull request Feb 19, 2024

Tracking issue: shard diff transfer - WAL delta #3477

Closed

100 tasks

timvisee reviewed Feb 20, 2024

View reviewed changes

ffuugoo force-pushed the diff-transfer-cleanup branch 4 times, most recently from a5dba1f to cd9c0f0 Compare February 23, 2024 14:14

ffuugoo marked this pull request as ready for review February 23, 2024 14:15

ffuugoo requested a review from timvisee February 23, 2024 14:16

github-actions bot mentioned this pull request Feb 23, 2024

Flaky test index::hnsw_index::tests::test_graph_connectivity::test_graph_connectivity #2875

Closed

ffuugoo force-pushed the diff-transfer-cleanup branch 3 times, most recently from bd7ae82 to ad44698 Compare February 23, 2024 17:05

timvisee reviewed Feb 23, 2024

View reviewed changes

This was referenced Feb 26, 2024

Add resolve_multiple_operations_with_the_same_tick unit-test #3687

Closed

Prototype zero_increment_forward_proxy_inconcistency test #3688

Closed

ffuugoo force-pushed the diff-transfer-cleanup branch from 7c92ecd to 6afaa10 Compare February 26, 2024 15:01

ffuugoo added 12 commits February 27, 2024 18:19

Move RecoveryPoint::insert method to the bottom of the impl block

a622987

Rename RecoveryPoint methods to be more descriptive...

e759a8b

...and cleanup `clock_map` module documentation

Rename RecoverableWal::from to RecoverableWal::new

1e27740

Rename highest_clocks and cutoff_clocks...

c679ddc

...to `newest_observed_clocks` and `oldest_resolvable_clocks`

Rename local_recovery_point and local_cutoff_point...

a047e2e

...to `newest_observed_clocks` and `oldest_resolvable_clocks`

Cleanup resolve_wal_delta

ddeca36

Remove async-std dev dependency from the collection crate

516fc63

Cleanup wal_delta module and tests

e5e5c98

Generalize Wal::read methods

86dfa43

Only allow diff-transfer related operations on non-proxified local shard

3b442b7

Cleanup LocalShard::update

2296c14

Refactor resolve_wal_delta to take iterator of operations...

c38fdfe

...instead of WAL, to simplify unit-testing

ffuugoo force-pushed the diff-transfer-cleanup branch from 1179325 to c38fdfe Compare February 27, 2024 18:00

ffuugoo requested a review from timvisee February 27, 2024 19:20

timvisee reviewed Feb 28, 2024

View reviewed changes

lib/collection/src/wal.rs Outdated Show resolved Hide resolved

lib/collection/src/wal.rs Outdated Show resolved Hide resolved

ffuugoo added 2 commits February 28, 2024 13:34

Remove non-critical TODOs

15ece72

Rename newest_observed_clocks and oldest_resolvable_clocks...

0a5eeaf

...to `newest_clocks` and `oldest_clocks`

ffuugoo requested a review from timvisee February 28, 2024 12:52

timvisee approved these changes Feb 29, 2024

View reviewed changes

ffuugoo merged commit cba268c into dev Feb 29, 2024
17 checks passed

ffuugoo deleted the diff-transfer-cleanup branch February 29, 2024 11:41

timvisee pushed a commit that referenced this pull request Mar 5, 2024

Diff transfer improvements (#3645)

1539087

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Diff transfer improvements #3645

Diff transfer improvements #3645

ffuugoo commented Feb 19, 2024 •

edited

timvisee left a comment

ffuugoo commented Feb 23, 2024

timvisee left a comment

timvisee Feb 23, 2024

ffuugoo Feb 26, 2024

ffuugoo Feb 28, 2024 •

edited

timvisee Feb 28, 2024

timvisee Feb 28, 2024

ffuugoo Feb 28, 2024 •

edited

timvisee Feb 23, 2024

ffuugoo Feb 26, 2024 •

edited

timvisee Feb 28, 2024

ffuugoo Feb 28, 2024

timvisee Feb 23, 2024

ffuugoo commented Feb 27, 2024 •

edited

Diff transfer improvements #3645

Diff transfer improvements #3645

Conversation

ffuugoo commented Feb 19, 2024 • edited

All Submissions:

New Feature Submissions:

Changes to Core Features:

timvisee left a comment

Choose a reason for hiding this comment

ffuugoo commented Feb 23, 2024

timvisee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffuugoo Feb 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffuugoo Feb 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffuugoo Feb 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffuugoo commented Feb 27, 2024 • edited

ffuugoo commented Feb 19, 2024 •

edited

ffuugoo Feb 28, 2024 •

edited

ffuugoo Feb 28, 2024 •

edited

ffuugoo Feb 26, 2024 •

edited

ffuugoo commented Feb 27, 2024 •

edited