Notify optimization handles to stop on shard drop#7765
Conversation
d741627 to
55d984d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/collection/src/shards/local_shard/drop.rs(1 hunks)lib/collection/src/update_handler.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/review-rules.md)
**/*.rs: Prefer explicitSomeType::from(x)over implicitx.into()in Rust
In new code, don't usetransmute_from_u8,transmute_to_u8,transmute_from_u8_to_slice,transmute_from_u8_to_mut_slice,transmute_to_u8_slice- usebytemuckorzerocopycrates instead
Prefer explicit exhaustive matches over catch-all_arm in Rust match expressions, so we don't miss handling new enum variants when they are added later (exceptions: tests and benchmarks, or when sure new variants won't affect logic)
Prefer explicit field ignoring using: _over using..in Rust struct destructuring (exceptions: tests and benchmarks, or when sure new fields won't affect logic)
Files:
lib/collection/src/shards/local_shard/drop.rslib/collection/src/update_handler.rs
🪛 GitHub Actions: Codespell
lib/collection/src/update_handler.rs
[error] 238-238: codespell: misspelling detected. 'hanldes' should be 'handles'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: rust-tests (ubuntu-latest)
- GitHub Check: test-consensus-compose
- GitHub Check: e2e-tests
- GitHub Check: rust-tests (macos-latest)
- GitHub Check: rust-tests (windows-latest)
- GitHub Check: test-consistency
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: integration-tests-consensus
- GitHub Check: integration-tests
- GitHub Check: Build Qdrant Edge Python bindings
- GitHub Check: lint
🔇 Additional comments (2)
lib/collection/src/shards/local_shard/drop.rs (1)
50-50: LGTM! Proper placement in shutdown sequence.The call to
notify_optimization_handles_to_stop()is correctly positioned after stopping the flush worker and before sending the Stop signal. This non-blocking notification aligns with the drop context constraints where awaiting is not possible.lib/collection/src/update_handler.rs (1)
241-246: LGTM! Clean implementation of non-blocking stop notification.The method correctly acquires a blocking lock and iterates through all optimization handles to signal them to stop. The blocking lock usage is appropriate for this synchronous context, and the approach aligns with the fire-and-forget requirement of the drop routine.
This PR is a fix to restore a proper shutdown on ctr+c.
Currently the running optimizations are preventing the Qdrant process to shutdown.
The fix is to notify the optimization handles to flip the cancellation flag internally.
No need to wait for an acknowledgement, the optimization loops will eventually stop.
Manually tested with
bfb -n 10M -d 256 --keywords 100 --float-payloads true --int-payloads 2 --uuid-payloads --bool-payloads --geo-payloads --text-payloads