Implement partial snapshot recovery #6206

ffuugoo · 2025-03-19T15:58:22Z

This PR extends ShardReplicaSet::restore_local_replica_from to support partial snapshot recovery.

APIs to create and recover partial snapshots and integration tests will be added in a follow-up PRs.

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 · 2025-03-24T15:06:39Z

I've already implemented the APIs (and also merged take_snapshot and take_partial_snapshot methods, because they are very similar to each other), but I decided to split these changes into separate PRs, that I will open shortly.

lib/segment/src/segment/segment_ops.rs

lib/collection/src/shards/replica_set/snapshots.rs

Condition to mark as `Dead` was inverted the whole time... 😅 Thanks coderabbitai!

lib/segment/src/segment/snapshot.rs

timvisee · 2025-03-28T15:46:28Z

lib/segment/src/data_types/segment_manifest.rs

+    pub fn validate(&self) -> OperationResult<()> {
+        for (segment_id, manifest) in &self.manifests {
+            for (file, version) in &manifest.file_versions {
+                if version.or_segment_version(manifest.segment_version) > manifest.segment_version {


I see manifest.segment_version on both sides of the > here, is that on purpose?

Would it make sense to take an option instead and do .is_some_and(version < segment version)?

Yeah, is_some_and would make more sense here, I'll fix it.

Ah. It's not an Option, but a enum FileVersion { Version(SeqNumberType), Unversioned } and or_segment_version is basically Option::unwrap_or for Unversioned. I can add is_versioned_and method, but in this particular case version.or_segment_version(manifest.segment_version) > manifest.segment_version works the same.

timvisee · 2025-04-02T08:42:25Z

lib/segment/src/segment/snapshot.rs

+fn failed_to_add(what: &str, path: &Path, err: impl fmt::Display) -> OperationError {
+    OperationError::service_error(format!(
+        "failed to add {what} {} into snapshot: {err}",
+        path.display(),
+    ))
+}


Since this is exclusively used in the function above, how do you feel about inlining it?

I never bother with explicit inlining, but sure.

lib/collection/src/shards/replica_set/snapshots.rs

timvisee · 2025-04-02T08:54:18Z

lib/collection/src/shards/replica_set/snapshots.rs

@@ -82,6 +104,7 @@ impl ShardReplicaSet {
    pub async fn restore_local_replica_from(


This function is humongous, spanning a few hundred lines. I think we should split it, but we can do so in a separate PR.

KShivendu

LGTM. Looking forward to the tests and integration of this feature :)

Please split the restore_local_replica_from function.

- Fix creating partial snapshot without RocksDB backup - Fix `segment_manifest.json` file placement - Implement partial snapshot recovery - Allow restoring partial snapshot for all shard types - Validate segment manifests when recovering partial snapshot - Log error, if streaming shard/partial snapshot failed - Contextualize errors in `snapshot_files` function - Tweak log messages in `Segment::take_snapshot` method - Remove unnecessary lifetimes from `SegmentHolder` iter methods

ffuugoo force-pushed the partial-snapshot-recover branch 2 times, most recently from 06f8a7f to f4dcec0 Compare March 24, 2025 14:57

ffuugoo changed the title ~~WIP: Implement partial snapshot recovery~~ Implement partial snapshot recovery Mar 24, 2025

ffuugoo marked this pull request as ready for review March 24, 2025 15:02

ffuugoo requested review from timvisee and generall March 24, 2025 15:02

This comment was marked as resolved.

Sign in to view

github-actions bot mentioned this pull request Mar 24, 2025

Flaky test index::tests::hw_counter_test::test_hw_counter_for_plain_sparse_search #6231

Closed

ffuugoo force-pushed the partial-snapshot-recover branch from f4dcec0 to cc6c0db Compare March 25, 2025 11:53

This comment was marked as outdated.

Sign in to view

timvisee reviewed Mar 25, 2025

View reviewed changes

github-actions bot mentioned this pull request Mar 25, 2025

Flaky test hnsw_discover_test::hnsw_discover_precision #2973

Open

ffuugoo force-pushed the partial-snapshot-recover branch from 5d7581d to 0bf5d2b Compare March 26, 2025 16:25

This comment was marked as outdated.

Sign in to view

ffuugoo force-pushed the partial-snapshot-recover branch from 0bf5d2b to 4e38c00 Compare March 26, 2025 17:38

ffuugoo requested a review from timvisee March 26, 2025 17:39

This comment was marked as resolved.

Sign in to view

ffuugoo force-pushed the partial-snapshot-recover branch from 4e38c00 to 65ec3d9 Compare March 28, 2025 12:29

This comment was marked as resolved.

Sign in to view

ffuugoo force-pushed the partial-snapshot-recover branch from 65ec3d9 to d55e366 Compare March 28, 2025 13:27

ffuugoo added 9 commits March 31, 2025 17:51

Fix creating partial snapshot without RocksDB backup

884fe6b

Fix segment_manifest.json file placement

3295842

Implement partial snapshot recovery

9b73913

Allow restoring partial snapshot for all shard types

38952e6

Validate segment manifests when recovering partial snapshot

669d4ea

Fix marking replica as Dead if shard snapshot recovery failed

7e97899

Condition to mark as `Dead` was inverted the whole time... 😅 Thanks coderabbitai!

Log error, if streaming shard/partial snapshot failed

69c4a10

Contextualize errors in snapshot_files function

284186e

Tweak log messages in Segment::take_snapshot method

cbad0b9

Remove unnecessary lifetimes from SegmentHolder iter methods

ed5f2e1

ffuugoo force-pushed the partial-snapshot-recover branch from d55e366 to ed5f2e1 Compare March 31, 2025 15:56

timvisee approved these changes Apr 2, 2025

View reviewed changes

KShivendu approved these changes Apr 2, 2025

View reviewed changes

ffuugoo merged commit dce4a94 into dev Apr 2, 2025
17 checks passed

ffuugoo deleted the partial-snapshot-recover branch April 2, 2025 13:11

coderabbitai bot mentioned this pull request Apr 2, 2025

Use MaybeUninit in vector storages and query scorers #6303

Merged

generall added this to the Incremental Snapshots milestone Apr 20, 2025

		@@ -82,6 +104,7 @@ impl ShardReplicaSet {
		pub async fn restore_local_replica_from(

Implement partial snapshot recovery #6206

Implement partial snapshot recovery #6206

Uh oh!

Conversation

ffuugoo commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All Submissions:

New Feature Submissions:

Changes to Core Features:

Uh oh!

This comment was marked as resolved.

ffuugoo commented Mar 24, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

timvisee Mar 28, 2025

Choose a reason for hiding this comment

Uh oh!

ffuugoo Apr 2, 2025

Choose a reason for hiding this comment

Uh oh!

ffuugoo Apr 2, 2025

Choose a reason for hiding this comment

Uh oh!

timvisee Apr 2, 2025

Choose a reason for hiding this comment

Uh oh!

ffuugoo Apr 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

timvisee Apr 2, 2025

Choose a reason for hiding this comment

Uh oh!

KShivendu Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KShivendu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ffuugoo commented Mar 19, 2025 •

edited

Loading

KShivendu Apr 2, 2025 •

edited

Loading

KShivendu left a comment •

edited

Loading