Conversation
a36ea32 to
c77273d
Compare
|
The root cause is again that the proxy's write segments are shared and modified without respecting the locks hierarchy, qdrant/lib/shard/src/segment_holder/mod.rs Lines 749 to 764 in 20db14f The delete operation lands exactly between the The point is supposed to be present because it passed the filter a nano second previously. Grabbing the |
c77273d to
936484d
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 (1)
lib/collection/tests/integration/continuous_snapshot_test.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (.github/review-rules.md)
**/*.rs: Prefer explicit SomeType::from(x) over implicit x.into() in Rust code
Do not use transmute_from_u8, transmute_to_u8, transmute_from_u8_to_slice, transmute_from_u8_to_mut_slice, transmute_to_u8_slice in new code; use bytemuck or zerocopy instead
Files:
lib/collection/tests/integration/continuous_snapshot_test.rs
**/{tests,benches}/**/*.rs
📄 CodeRabbit inference engine (.github/review-rules.md)
Using .unwrap() and panic!() in tests and benchmarks is acceptable and should not be flagged
Files:
lib/collection/tests/integration/continuous_snapshot_test.rs
⏰ 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). (9)
- GitHub Check: rust-tests (windows-latest)
- GitHub Check: rust-tests (macos-latest)
- GitHub Check: e2e-tests
- GitHub Check: rust-tests (ubuntu-latest)
- GitHub Check: test-consensus-compose
- GitHub Check: rust-tests-no-rocksdb (ubuntu-latest)
- GitHub Check: integration-tests
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: integration-tests-consensus
🔇 Additional comments (1)
lib/collection/tests/integration/continuous_snapshot_test.rs (1)
128-128: LGTM!The comment clarifies the behavior of the following code block.
|
|
||
| // Retrieve one point at a time again with payload | ||
| for i in 0..points_count { | ||
| let retrieve_point = PointRequestInternal { | ||
| ids: vec![i.into()], | ||
| with_payload: Some(true.into()), | ||
| with_vector: WithVector::Bool(false), | ||
| }; | ||
| let hw_counter = HwMeasurementAcc::disposable(); | ||
| let retrieve_result = collection | ||
| .retrieve( | ||
| retrieve_point, | ||
| None, | ||
| &ShardSelectorInternal::All, | ||
| None, | ||
| hw_counter, | ||
| ) | ||
| .await?; | ||
| assert_eq!(retrieve_result.len(), 1); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use explicit type conversion instead of .into().
Line 200 uses true.into() which violates the coding guideline: "Prefer explicit SomeType::from(x) over implicit x.into() in Rust code". Use an explicit conversion to improve code clarity.
As per coding guidelines.
Apply this diff to use explicit conversion:
let retrieve_point = PointRequestInternal {
ids: vec![i.into()],
- with_payload: Some(true.into()),
+ with_payload: Some(WithPayloadInterface::from(true)),
with_vector: WithVector::Bool(false),
};Note: The new retrieval loop appropriately exercises the code path with payload requests, which is essential for this regression test. The lack of explicit payload verification is acceptable since the race condition manifests as retrieval errors rather than incorrect data.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Retrieve one point at a time again with payload | |
| for i in 0..points_count { | |
| let retrieve_point = PointRequestInternal { | |
| ids: vec![i.into()], | |
| with_payload: Some(true.into()), | |
| with_vector: WithVector::Bool(false), | |
| }; | |
| let hw_counter = HwMeasurementAcc::disposable(); | |
| let retrieve_result = collection | |
| .retrieve( | |
| retrieve_point, | |
| None, | |
| &ShardSelectorInternal::All, | |
| None, | |
| hw_counter, | |
| ) | |
| .await?; | |
| assert_eq!(retrieve_result.len(), 1); | |
| } | |
| // Retrieve one point at a time again with payload | |
| for i in 0..points_count { | |
| let retrieve_point = PointRequestInternal { | |
| ids: vec![i.into()], | |
| with_payload: Some(WithPayloadInterface::from(true)), | |
| with_vector: WithVector::Bool(false), | |
| }; | |
| let hw_counter = HwMeasurementAcc::disposable(); | |
| let retrieve_result = collection | |
| .retrieve( | |
| retrieve_point, | |
| None, | |
| &ShardSelectorInternal::All, | |
| None, | |
| hw_counter, | |
| ) | |
| .await?; | |
| assert_eq!(retrieve_result.len(), 1); | |
| } |
🤖 Prompt for AI Agents
In lib/collection/tests/integration/continuous_snapshot_test.rs around lines 195
to 214, replace the implicit conversion Some(true.into()) with an explicit
conversion using the concrete payload type's from method (e.g.,
Some(WithPayload::from(true))) so the code follows the guideline "Prefer
explicit SomeType::from(x) over implicit x.into()"; update the loop to call the
concrete_type::from(true) for with_payload and keep the rest of the retrieval
logic unchanged.
| let retrieve_point = PointRequestInternal { | ||
| ids: vec![i.into()], | ||
| with_payload: Some(true.into()), | ||
| with_vector: WithVector::Bool(false), |
There was a problem hiding this comment.
Nit: can we retrieve it including vectors and also assert we got both a vector and payload?
📝 WalkthroughWalkthroughThe test lib/collection/tests/integration/continuous_snapshot_test.rs now performs a retrieval after each point insertion. The retrieval requests both payload and vector data (with_payload=true, with_vector enabled) and asserts that exactly one result is returned and that both the vector and payload are present. No public or exported declarations were changed. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.rs📄 CodeRabbit inference engine (.github/review-rules.md)
Files:
**/{tests,benches}/**/*.rs📄 CodeRabbit inference engine (.github/review-rules.md)
Files:
⏰ 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
* Continuous snapshot data loss * assert vector & payload are present
This test used to be broken with:
It is now fixed after reworking how proxy segments work in #7345
The PR can now be merged to act as non regression test.