Avoid shards holder locks on snapshot by generall · Pull Request #8072 · qdrant/qdrant

generall · 2026-02-08T15:04:44Z

Fixes: #7489
Unblocks: #7510

[AI-free]

refactor shard.create_snapshot to own everything necessary in the future for snapshotting
do not lock shards_holder and replica_set during snapshot
separate locks and snapshot creation all the way down

…ure for snapshotting

generall · 2026-02-08T18:04:23Z

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

-    ) -> CollectionResult<()> {
-        let local_read = self.local.read().await;
-
+    ) -> impl Future<Output = CollectionResult<()>> + use<> {


this is the main change

generall · 2026-02-08T18:04:36Z

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

        shard_id: ShardId,
        temp_dir: &Path,
-    ) -> CollectionResult<SnapshotDescription> {
+    ) -> CollectionResult<impl Future<Output = CollectionResult<SnapshotDescription>> + use<>> {


this is the second main change

…level

generall · 2026-02-08T22:33:36Z

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

        manifest: Option<SnapshotManifest>,
        save_wal: bool,
-    ) -> CollectionResult<()> {
+    ) -> CollectionResult<impl Future<Output = CollectionResult<()>> + use<>> {


and this as well

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

ffuugoo · 2026-02-10T14:24:43Z

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

        manifest: Option<SnapshotManifest>,
        save_wal: bool,
-    ) -> CollectionResult<()> {
+    ) -> CollectionResult<impl Future<Output = CollectionResult<()>> + use<>> {


Would + 'static work here? IMO, it's less weird/niche than + use<>, I'd prefer to use 'static if it works.

Suggested change

) -> CollectionResult<impl Future<Output = CollectionResult<()>> + use<>> {

) -> CollectionResult<impl Future<Output = CollectionResult<()>> + 'static> {

I would argue that use<> is more precise compare to 'static. We declare that future doesn't capture any lifetimes.

Maybe. Though, I think in this case both are equivalent. But also, why do we care? If we accidentally capture non-static lifetime or smth, we would get a compilation error anyway. IMO, rule of thumb is "use strictly necessary, least restrictive bound".

And also 'static is better recognized than use<>. I'd prefer 'static as long as it works.

lib/collection/src/collection/snapshots.rs

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

ffuugoo · 2026-02-10T15:01:56Z

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

+        let local_read = self.local.read().await;

-        let shard_config = ShardConfig::new_replica_set();
-        shard_config.save_to_tar(tar).await?;
-        Ok(())
+        let maybe_local_snapshot_future = if let Some(local) = &*local_read {
+            Some(
+                local
+                    .get_snapshot_creator(&temp_path, &tar, format, manifest, save_wal)
+                    .await?,
+            )
+        } else {
+            None
+        };
+
+        drop(local_read);


🙅‍♀️ no explicit drops 🚫

I'd also just call it create_snapshot instead of maybe_local_snapshot_future

Suggested change

let local_read = self.local.read().await;

let shard_config = ShardConfig::new_replica_set();

shard_config.save_to_tar(tar).await?;

Ok(())

let maybe_local_snapshot_future = if let Some(local) = &*local_read {

Some(

local

.get_snapshot_creator(&temp_path, &tar, format, manifest, save_wal)

.await?,

)

} else {

None

};

drop(local_read);

let maybe_local_snapshot_future = if let Some(local) = &*self.local.read().await {

let create_snapshot = local

.get_snapshot_creator(&temp_path, &tar, format, manifest, save_wal)

.await?;

Some(create_snapshot)

} else {

None

};

this one is actually not necessary, and it will work the same way without drop. But I prefer to keep the drop to make it explicit.

if let Some(_) = local.read() { ... } is still simpler. 🤷‍♀️

ffuugoo · 2026-02-10T15:06:55Z

lib/collection/src/shards/dummy_shard.rs

    pub async fn create_snapshot(
        &self,
        _temp_path: &Path,
        _tar: &tar_ext::BuilderExt,
        _format: SnapshotFormat,
        _manifest: Option<SnapshotManifest>,
        _save_wal: bool,
-    ) -> CollectionResult<()> {
-        self.dummy()
+    ) -> CollectionError {
+        self.dummy_error()
    }


simpler code

Ah, yeah, I see. Then, maybe, remove create_snapshot at all and just do dummy.dummy_error() in that match? This does not make any sense at all.

ffuugoo

Overall, LGTM, I don't see any major issues. Mostly just code style/structuring.

ffuugoo · 2026-02-10T17:05:39Z

lib/collection/src/shards/shard.rs

            Shard::Dummy(dummy_shard) => {
-                dummy_shard
+                return Err(dummy_shard
                    .create_snapshot(temp_path, tar, format, manifest, save_wal)
-                    .await
+                    .await);
            }


Just do return Err(dummy_shard.dummy_error()) and remove DummyShard::create_snapshot method entirely?

* refactor shard.create_snapshot to own everything necessary in the future for snapshotting * do not lock shards_holder and replica_set during snapshot * separate locks and snapshot creation all the way down * separate locks and snapshot creation even further on the local shard level * final touches * minor review fixes

generall added 3 commits February 8, 2026 13:10

refactor shard.create_snapshot to own everything necessary in the fut…

263d299

…ure for snapshotting

do not lock shards_holder and replica_set during snapshot

a764359

separate locks and snapshot creation all the way down

12ce9f5

github-actions bot mentioned this pull request Feb 8, 2026

Flaky test tests::wal_recovery_test::test_wal_replay_loads_pending_to_queue #8057

Closed

generall commented Feb 8, 2026

View reviewed changes

generall added 2 commits February 8, 2026 23:25

separate locks and snapshot creation even further on the local shard …

787dd40

…level

final touches

c304361

generall commented Feb 8, 2026

View reviewed changes

generall requested a review from timvisee February 8, 2026 23:03

timvisee added the release:1.17.0 label Feb 9, 2026

qdrant deleted a comment from coderabbitai bot Feb 9, 2026

tellet-q mentioned this pull request Feb 9, 2026

Add test for consensus' freeze during streaming snapshot #8075

Merged

3 tasks

timvisee requested a review from ffuugoo February 9, 2026 17:37

timvisee approved these changes Feb 9, 2026

View reviewed changes

lib/collection/src/shards/local_shard/snapshot.rs Show resolved Hide resolved

JojiiOfficial approved these changes Feb 10, 2026

View reviewed changes

ffuugoo reviewed Feb 10, 2026

View reviewed changes

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

ffuugoo reviewed Feb 10, 2026

View reviewed changes

lib/collection/src/shards/local_shard/snapshot.rs Outdated Show resolved Hide resolved

ffuugoo reviewed Feb 10, 2026

View reviewed changes

ffuugoo approved these changes Feb 10, 2026

View reviewed changes

minor review fixes

e4f93d2

qdrant deleted a comment from coderabbitai bot Feb 10, 2026

generall merged commit a610710 into dev Feb 10, 2026
15 checks passed

generall deleted the ownable-local-shard branch February 10, 2026 17:03

ffuugoo reviewed Feb 10, 2026

View reviewed changes

This was referenced Feb 13, 2026

Detach snapshot creation from ToC/collections, run in the background #7489

Closed

Bump version to 1.17.0 #8160

Merged

	) -> CollectionResult<impl Future<Output = CollectionResult<()>> + use<>> {
	) -> CollectionResult<impl Future<Output = CollectionResult<()>> + 'static> {

Conversation

generall commented Feb 8, 2026 • edited by timvisee Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ffuugoo Feb 10, 2026 • 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 Feb 10, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

ffuugoo Feb 10, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ffuugoo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

generall commented Feb 8, 2026 •

edited by timvisee

Loading

ffuugoo Feb 10, 2026 •

edited

Loading

ffuugoo Feb 10, 2026 •

edited

Loading

ffuugoo Feb 10, 2026 •

edited

Loading