Rework `CommitHashRing` consensus message into `CommitRead`/`CommitWrite`/`Finish` #4417

ffuugoo · 2024-06-07T16:12:32Z

Tracked in #4213.

This PR reworks CommitHashRing into three separate operations: CommitRead, CommitWrite and Finish.
We need all three to make resharding process seem "transparent" to the user.

We don't (yet) properly track resharding state, so this PR adds some "stubs" and TODOs in places where we will introduce state later.

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?

ffuugoo · 2024-06-07T16:21:29Z

I've got somewhat stuck with an alternative idea (to filter incoming updates), and I had this PR half-implemented already, so I've decided to push this to a working prototype.

In my simple tests - it works:

run a cluster (or just a single node in a cluster mode)
populate collection with bfb

POST collections/benchmark/cluster
{
  "start_resharding": {}
}

```
POST collections/benchmark/cluster
{
  "commit_read": {}
}
```
- added this operation temporarily, strictly for debug/testing purposes
- also disabled Tim's code that start resharding driver, so that it won't interfere with testing
POST collections/benchmark/points/count
- should return less points than expected, cause they are considered to be moved to the new shard, and so filtered from the result

I suggest we take a look at the PR and consider, once again, which of the two approaches to implement.

lib/segment/src/index/struct_payload_index.rs

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

lib/collection/src/shards/shard_holder.rs

lib/collection/src/collection/resharding.rs

lib/collection/src/hash_ring.rs

lib/segment/src/problems/unindexed_field.rs

timvisee · 2024-06-17T14:51:29Z

lib/collection/src/shards/shard_holder.rs

+        let Some(ring) = self.rings.get(&state.shard_key) else {
+            return None; // TODO(resharding)!?
+        };
+
+        let HashRing::Resharding { new, .. } = ring else {
+            return None; // TODO(resharding)!?
+        };


I would say, error, as we don't expect this.

lib/segment/src/index/query_optimization/condition_converter.rs

ffuugoo · 2024-06-18T10:06:07Z

lib/api/src/grpc/conversions.rs

+                conditions
+                    .into_iter()
+                    .filter(|c| !matches!(c, segment::types::Condition::Resharding(_))) // TODO(resharding)!?
+                    .map(|c| c.into())
+                    .collect()


A bit hacky, but this was much easier, than refactoring all read operations, so that we only propagate Condition::Resharding to local read operations, but not to remote read operations.

Would a condition.is_local_only() make sense here?

ffuugoo · 2024-06-18T10:08:42Z

lib/collection/src/hash_ring.rs

+    fn eq(&self, other: &dyn ReshardingCondition) -> bool {
+        match other.as_any().downcast_ref::<Self>() {
+            Some(other) => self == other,
+            None => false,
+        }
+    }
+
+    fn as_any(&self) -> &dyn any::Any {
+        self
+    }


Might be a bit of an overkill, but this is required to implement PartialEq on Condition.

ffuugoo · 2024-06-18T10:11:51Z

lib/segment/src/types.rs

+            (Self::HasId(this), Self::HasId(other)) => this == other,
+            (Self::Nested(this), Self::Nested(other)) => this == other,
+            (Self::Filter(this), Self::Filter(other)) => this == other,
+            (Self::Resharding(this), Self::Resharding(other)) => this.eq(other.deref()),


Without ReshardingCondition::eq and ReshardingCondition::as_any, we can always return false here, but this will kinda break equality for Condition.

timvisee

Though I don't see any pressing issues, I do see a lot of TODO's. What do we do about those?

If we merge all of the we get a growing number of TODOs which we'll never resolve. I'd suggest as many as possible right away, or remove them if it's something not strictly necessary.

lib/collection/src/collection/point_ops.rs

lib/collection/src/shards/shard_holder.rs

ffuugoo · 2024-06-18T15:35:31Z

If we merge all of the we get a growing number of TODOs which we'll never resolve. I'd suggest as many as possible right away, or remove them if it's something not strictly necessary.

All marked with TODO(resharding), we can remove them later, when finalizing resharding.

…request results

…ish` `cargo clippy --fix`

…trieve request results Add `Condition::is_local_only` method

…trieve request results Clarified a few `TODO`s

…ite`/`Finish` (#4417) * Refactor `CommitHashRing` into `CommitRead`/`CommitWrite`/`Finish` * Add `Resharding` filter condition * Filter "resharded" points from search, scroll by, count and retrieve request results * fixup! Refactor `CommitHashRing` into `CommitRead`/`CommitWrite`/`Finish` `cargo clippy --fix` * Apply suggestions from code review * fixup! Filter "resharded" points from search, scroll by, count and retrieve request results Add `Condition::is_local_only` method * fixup! Add `Resharding` filter condition * fixup! Filter "resharded" points from search, scroll by, count and retrieve request results Clarified a few `TODO`s * Fix clippy suggestions --------- Co-authored-by: Tim Visée <tim+github@visee.me> Co-authored-by: timvisee <tim@visee.me>

ffuugoo requested review from timvisee and generall June 7, 2024 16:12

ffuugoo mentioned this pull request Jun 7, 2024

Tracking issue: resharding #4213

Open

56 tasks

timvisee reviewed Jun 14, 2024

View reviewed changes

ffuugoo force-pushed the commit-read-write-consensus-message branch from 7b5bc6d to 2cfc12a Compare June 17, 2024 12:55

timvisee reviewed Jun 17, 2024

View reviewed changes

ffuugoo force-pushed the commit-read-write-consensus-message branch from 60533e1 to 8739505 Compare June 18, 2024 09:41

ffuugoo marked this pull request as ready for review June 18, 2024 09:58

ffuugoo commented Jun 18, 2024

View reviewed changes

timvisee approved these changes Jun 18, 2024

View reviewed changes

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

lib/collection/src/shards/shard_holder.rs Show resolved Hide resolved

ffuugoo force-pushed the commit-read-write-consensus-message branch from 813f124 to 3494359 Compare June 19, 2024 09:03

ffuugoo and others added 8 commits June 19, 2024 11:03

Refactor CommitHashRing into CommitRead/CommitWrite/Finish

705c38d

Add Resharding filter condition

2a5da0c

Filter "resharded" points from search, scroll by, count and retrieve …

a655c95

…request results

fixup! Refactor CommitHashRing into CommitRead/CommitWrite/`Fin…

a30767c

…ish` `cargo clippy --fix`

Apply suggestions from code review

89a94e8

fixup! Filter "resharded" points from search, scroll by, count and re…

8765840

…trieve request results Add `Condition::is_local_only` method

fixup! Add Resharding filter condition

2003024

fixup! Filter "resharded" points from search, scroll by, count and re…

4ef0c7d

…trieve request results Clarified a few `TODO`s

ffuugoo force-pushed the commit-read-write-consensus-message branch from 3494359 to 4ef0c7d Compare June 19, 2024 09:09

timvisee approved these changes Jun 19, 2024

View reviewed changes

Fix clippy suggestions

85000a6

github-actions bot mentioned this pull request Jun 19, 2024

Flaky test consensus::tests::collection_creation_passes_consensus #2860

Open

timvisee merged commit d4b3201 into dev Jun 19, 2024
17 checks passed

timvisee deleted the commit-read-write-consensus-message branch June 19, 2024 10:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework `CommitHashRing` consensus message into `CommitRead`/`CommitWrite`/`Finish` #4417

Rework `CommitHashRing` consensus message into `CommitRead`/`CommitWrite`/`Finish` #4417

ffuugoo commented Jun 7, 2024 •

edited

ffuugoo commented Jun 7, 2024 •

edited

timvisee Jun 17, 2024

ffuugoo Jun 18, 2024

timvisee Jun 18, 2024

ffuugoo Jun 18, 2024

ffuugoo Jun 18, 2024

timvisee left a comment •

edited

ffuugoo commented Jun 18, 2024

Rework CommitHashRing consensus message into CommitRead/CommitWrite/Finish #4417

Rework CommitHashRing consensus message into CommitRead/CommitWrite/Finish #4417

Conversation

ffuugoo commented Jun 7, 2024 • edited

All Submissions:

New Feature Submissions:

Changes to Core Features:

ffuugoo commented Jun 7, 2024 • edited

timvisee Jun 17, 2024

Choose a reason for hiding this comment

ffuugoo Jun 18, 2024

Choose a reason for hiding this comment

timvisee Jun 18, 2024

Choose a reason for hiding this comment

ffuugoo Jun 18, 2024

Choose a reason for hiding this comment

ffuugoo Jun 18, 2024

Choose a reason for hiding this comment

timvisee left a comment • edited

Choose a reason for hiding this comment

ffuugoo commented Jun 18, 2024

Rework `CommitHashRing` consensus message into `CommitRead`/`CommitWrite`/`Finish` #4417

Rework `CommitHashRing` consensus message into `CommitRead`/`CommitWrite`/`Finish` #4417

ffuugoo commented Jun 7, 2024 •

edited

ffuugoo commented Jun 7, 2024 •

edited

timvisee left a comment •

edited