lib/collection/src/shards/local_shard/scroll.rs (1)

156-156: Hold update_operation_lock for less time to avoid blocking snapshots/updates

Currently the read guard is kept through payload/vector retrieval, which can be slow and will block snapshot attempts (write) and updates longer than necessary. Release it once the candidate IDs are computed.

Apply diffs below in each method.

scroll_by_id:

@@
-        let with_payload = WithPayload::from(with_payload_interface);
-        // update timeout
-        let timeout = timeout.saturating_sub(start.elapsed());
-        let mut records_map = tokio::time::timeout(
+        let with_payload = WithPayload::from(with_payload_interface);
+        // update timeout
+        let timeout = timeout.saturating_sub(start.elapsed());
+        // Release update lock before fetching payload/vector data to reduce contention.
+        drop(scroll_lock);
+        let mut records_map = tokio::time::timeout(
@@
-        .map_err(|_: Elapsed| CollectionError::timeout(timeout.as_secs() as usize, "retrieve"))??;
-
-        drop(scroll_lock);
+        .map_err(|_: Elapsed| CollectionError::timeout(timeout.as_secs() as usize, "retrieve"))??;

scroll_by_field:

@@
-        let with_payload = WithPayload::from(with_payload_interface);
-
-        // update timeout
-        let timeout = timeout.saturating_sub(start.elapsed());
-
-        // Fetch with the requested vector and payload
-        let records_map = tokio::time::timeout(
+        let with_payload = WithPayload::from(with_payload_interface);
+        // update timeout
+        let timeout = timeout.saturating_sub(start.elapsed());
+        // Release update lock before fetching payload/vector data to reduce contention.
+        drop(scroll_lock);
+        // Fetch with the requested vector and payload
+        let records_map = tokio::time::timeout(
@@
-        .map_err(|_| CollectionError::timeout(timeout.as_secs() as usize, "retrieve"))??;
-
-        drop(scroll_lock);
+        .map_err(|_| CollectionError::timeout(timeout.as_secs() as usize, "retrieve"))??;

scroll_randomly:

@@
-        let with_payload = WithPayload::from(with_payload_interface);
-        // update timeout
-        let timeout = timeout.saturating_sub(start.elapsed());
-        let records_map = tokio::time::timeout(
+        let with_payload = WithPayload::from(with_payload_interface);
+        // update timeout
+        let timeout = timeout.saturating_sub(start.elapsed());
+        // Release update lock before fetching payload/vector data to reduce contention.
+        drop(scroll_lock);
+        let records_map = tokio::time::timeout(
@@
-        .map_err(|_: Elapsed| CollectionError::timeout(timeout.as_secs() as usize, "retrieve"))??;
-
-        drop(scroll_lock);
+        .map_err(|_: Elapsed| CollectionError::timeout(timeout.as_secs() as usize, "retrieve"))??;

Also applies to: 241-241, 340-340

lib/collection/src/shards/local_shard/snapshot_tests.rs (1)

71-75: Explicitly drop tar builder and assert count dynamically

Make intent explicit by dropping the builder before reading; assert the expected archive count from the holder to reduce fixture coupling.

Apply:

-    let mut tar = tar::Archive::new(File::open(&snapshot_file).unwrap());
-    let archive_count = tar.entries_with_seek().unwrap().count();
-    // one archive produced per concrete segment in the SegmentHolder
-    assert_eq!(archive_count, 2);
+    // Ensure writer is fully flushed before reading
+    drop(tar);
+    let mut archive = tar::Archive::new(File::open(&snapshot_file).unwrap());
+    let archive_count = archive.entries_with_seek().unwrap().count();
+    // one archive per concrete segment in the SegmentHolder
+    assert_eq!(archive_count, before_ids.len());

lib/shard/Cargo.toml (1)

12-14: Move serde_json to dev-dependencies
All serde_json usages are inside #[cfg(test)] modules; relocate it to [dev-dependencies] or gate it under the testing feature to avoid adding it to production builds.

lib/shard/src/segment_holder/snapshot.rs (2)

63-64: Avoid unwrap on get(); make the invariant explicit (or return an error)

Using unwrap can panic if the mapping changed unexpectedly. Prefer expect with a message, or return an OperationError.

-            let segment = segments_lock.get(segment_id).unwrap();
+            let segment = segments_lock
+                .get(segment_id)
+                .expect("segment_id obtained from segment_ids() must exist while holding upgradable read lock");

---

49-56: Clean up tmp segment on early failure

If index replication fails before swap, tmp_segment is left behind. Add a cleanup path to drop tmp_segment data on error before returning.

Example approach: wrap creation in a small guard that calls tmp_segment.drop_data() unless “committed”, and mark committed right before returning Ok((proxies, tmp_segment, ...)).

Also applies to: 57-78, 80-89

lib/collection/src/shards/local_shard/mod.rs (4)

208-209: Rename local var to match field and intent

Use update_operation_lock consistently to avoid confusion.

-        let scroll_read_lock = Arc::new(tokio::sync::RwLock::new(()));
+        let update_operation_lock = Arc::new(tokio::sync::RwLock::new(()));
@@
-            scroll_read_lock.clone(),
+            update_operation_lock.clone(),
@@
-            update_operation_lock: scroll_read_lock,
+            update_operation_lock,

Also applies to: 226-228, 261-262

---

122-124: Use imported alias for consistency

Type already aliased as TokioRwLock above.

-    pub(super) update_operation_lock: Arc<tokio::sync::RwLock<()>>,
+    pub(super) update_operation_lock: Arc<TokioRwLock<()>>,

---

289-293: Prefer explicit From over Into per guidelines

-        let wal: SerdeWal<OperationWithClockTag> = SerdeWal::new(
-            wal_path.to_str().unwrap(),
-            (&collection_config_read.wal_config).into(),
-        )
+        let wal: SerdeWal<OperationWithClockTag> = SerdeWal::new(
+            wal_path.to_str().unwrap(),
+            WalOptions::from(&collection_config_read.wal_config),
+        )

---

575-577: Prefer explicit From over Into per guidelines

-        let wal: SerdeWal<OperationWithClockTag> =
-            SerdeWal::new(wal_path.to_str().unwrap(), (&config.wal_config).into())?;
+        let wal: SerdeWal<OperationWithClockTag> =
+            SerdeWal::new(wal_path.to_str().unwrap(), WalOptions::from(&config.wal_config))?;

lib/collection/src/shards/local_shard/snapshot.rs (1)

327-341: Use HashSet for unproxied IDs to avoid O(n²) retain

Switch from Vec to HashSet for membership tests.

-    let mut unproxied_segment_ids = Vec::with_capacity(proxies.len());
+    let mut unproxied_segment_ids = HashSet::with_capacity(proxies.len());
@@
-                Ok(lock) => {
-                    segments_lock = lock;
-                    unproxied_segment_ids.push(*segment_id);
-                }
+                Ok(lock) => {
+                    segments_lock = lock;
+                    unproxied_segment_ids.insert(*segment_id);
+                }
@@
-    proxies.retain(|(id, _)| !unproxied_segment_ids.contains(id));
+    proxies.retain(|(id, _)| !unproxied_segment_ids.contains(id));

Also applies to: 343-344

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: generall
PR: qdrant/qdrant#6635
File: lib/segment/src/index/field_index/full_text_index/text_index.rs:784-832
Timestamp: 2025-06-14T20:35:10.603Z
Learning: Functions gated with `#[cfg(feature = "testing")]` within `#[cfg(test)]` modules are safe from compilation issues since both the function and its call sites are restricted to test builds. The additional feature gate is often used for optional heavy test utilities.

Learnt from: IvanPleshkov
PR: qdrant/qdrant#7043
File: lib/segment/src/vector_storage/quantized/quantized_mmap_storage.rs:86-90
Timestamp: 2025-08-15T11:42:00.297Z
Learning: In lib/segment/src/vector_storage/quantized/quantized_mmap_storage.rs, overflow protection for encoded_storage_size computation (quantized_vector_size * vectors_count) is implemented in PR #7048, using checked_mul with u64 casting to prevent silent overflow.

lib/collection/src/shards/local_shard/snapshot.rs (1)

123-161: Minor: error message wording.

Nit: “Error while create empty WAL” → “Error while creating empty WAL”.

-            CollectionError::service_error(format!("Error while create empty WAL: {err}"))
+            CollectionError::service_error(format!("Error while creating empty WAL: {err}"))

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: timvisee
PR: qdrant/qdrant#7157
File: lib/shard/src/segment_holder/mod.rs:808-814
Timestamp: 2025-09-01T11:42:06.964Z
Learning: In Qdrant's segment holder, panicking when no segments exist during flush_all is intentional and preferred over graceful error handling, as having zero segments could permanently corrupt the WAL by acknowledging u64::MAX. The maintainers consider this condition impossible and use the panic as a fail-fast safety mechanism to prevent data corruption.