Allow updating WAL config in collection after creation#7032
Allow updating WAL config in collection after creation#7032
Conversation
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
lib/storage/src/content_manager/collection_meta_ops.rs (1)
249-251: WAL diff field wiring looks correct; document blocking semanticsThe field is correctly added and validated as nested. Please clarify that applying WAL config diffs is blocking and may trigger optimizer recreation to set expectations similar to
optimizers_config.Suggested doc tweak:
- /// WAL parameters for the collection. If none - it is left unchanged. + /// WAL parameters for the collection. If provided, the update is blocking and may trigger optimizer recreation; if none - it is left unchanged.Side note: while you are in this area,
optimizers_configin UpdateCollection lacks#[validate(nested)]unlike other nested fields (CreateCollection has it). Consider adding it for consistency in a follow-up.lib/storage/src/content_manager/conversions.rs (1)
180-197: Maps wal_config to internal type — consistent with other diffsUsing wal_config.map(WalConfigDiff::from) mirrors the HNSW mapping style and is correct. Optional nit: for consistency with CreateCollection above, you could also use |v| v.into().
lib/collection/src/collection/collection_ops.rs (1)
60-66: Doc nit: clarify wording to “WAL config” instead of “params”Minor clarity improvement in the comment block.
Apply this diff:
- /// Updates WAL config: - /// Saves new params on disk + /// Updates WAL config: + /// Saves new WAL config on disklib/storage/src/content_manager/toc/collection_meta_ops.rs (1)
157-160: Applies WAL config diff and triggers optimizer recreation — confirm necessityThe call to update_wal_config_from_diff is correct. Setting recreate_optimizers = true may be unnecessary unless optimizers depend on WAL config; recreating them is comparatively heavy. Please confirm the dependency and, if not required, avoid optimizer restarts on WAL-only changes.
Additionally, note that each diff handler persists the config independently, resulting in multiple disk writes during a multi-field update. If that’s a bottleneck for frequent updates, consider batching multiple diffs into a single write in the future.
Would you like me to propose a small refactor to collect changes and persist once per UpdateCollection call?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lib/api/src/grpc/proto/collections.proto(1 hunks)lib/api/src/grpc/qdrant.rs(1 hunks)lib/collection/src/collection/collection_ops.rs(1 hunks)lib/storage/src/content_manager/collection_meta_ops.rs(2 hunks)lib/storage/src/content_manager/conversions.rs(3 hunks)lib/storage/src/content_manager/mod.rs(1 hunks)lib/storage/src/content_manager/toc/collection_meta_ops.rs(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
lib/storage/src/content_manager/collection_meta_ops.rs (4)
lib/collection/src/operations/config_diff.rs (1)
validate(377-384)lib/collection/src/operations/types.rs (4)
validate(679-685)validate(1832-1837)validate(1964-1966)validate(1997-1999)lib/collection/src/operations/point_ops.rs (2)
validate(110-116)validate(255-261)lib/segment/src/types.rs (5)
validate(778-784)validate(1574-1584)validate(1904-1915)validate(1946-1953)validate(2996-3007)
⏰ 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). (12)
- GitHub Check: test-consistency
- GitHub Check: e2e-tests
- GitHub Check: integration-tests-consensus
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: integration-tests
- GitHub Check: test-consensus-compose
- GitHub Check: rust-tests (macos-latest)
- GitHub Check: rust-tests-no-rocksdb (ubuntu-latest)
- GitHub Check: storage-compat-test
- GitHub Check: rust-tests (ubuntu-latest)
- GitHub Check: rust-tests (windows-latest)
- GitHub Check: lint
🔇 Additional comments (7)
lib/storage/src/content_manager/collection_meta_ops.rs (1)
279-279: Initialize new field in empty updates (LGTM)Initializing
wal_configtoNoneinnew_emptypreserves the “no-op unless provided” semantics. Good catch.lib/storage/src/content_manager/mod.rs (1)
136-136: wal_config confirmed in all UpdateCollection constructorsI audited every
UpdateCollection { … }literal and foundwal_configexplicitly set in each case:
- lib/storage/src/content_manager/mod.rs (lines 131–140)
- lib/storage/src/content_manager/conversions.rs (lines 179–187)
- lib/storage/src/content_manager/collection_meta_ops.rs (lines 276–285)
No further constructors are missing the
wal_configfield.lib/api/src/grpc/qdrant.rs (1)
753-756: Wire-compatibility: field tag 10 is safe and append-onlyAdding optional
wal_configwith tag 10 at the end ofUpdateCollectionmaintains backward compatibility on the wire.lib/storage/src/content_manager/conversions.rs (2)
4-7: Wires WalConfigDiff into conversions — looks correctImport of WalConfigDiff under config_diff aligns with other diff imports and is used below. No concerns.
165-176: UpdateCollectionwal_configVerified and Clients Updated
- In
lib/api/src/grpc/proto/collections.proto, theUpdateCollectionmessage includes
optional WalConfigDiff wal_config = 10;with a unique tag number.- Generated Rust client in
lib/api/src/grpc/qdrant.rscorrectly has
#[prost(message, optional, tag = "10")] pub wal_config: Option<WalConfigDiff>.- OpenAPI/REST schemas and all client SDKs (gRPC and REST) include the new
wal_configfield.No further changes are needed.
lib/collection/src/collection/collection_ops.rs (1)
60-76: Adds update_wal_config_from_diff — implementation follows established patternMethod mirrors update_hnsw_config_from_diff: write-lock mutate, then persist via read-lock. This is consistent with the file’s conventions.
lib/storage/src/content_manager/toc/collection_meta_ops.rs (1)
131-139: Destructures wal_config in UpdateCollection — good integration pointBringing wal_config into the UpdateCollection flow is correct and matches other config diffs.
| optional QuantizationConfigDiff quantization_config = 7; // Quantization configuration of vector | ||
| optional SparseVectorConfig sparse_vectors_config = 8; // New sparse vector parameters | ||
| optional StrictModeConfig strict_mode_config = 9; // New strict mode configuration | ||
| optional WalConfigDiff wal_config = 10; // New WAL parameters for the collection |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Good addition; clarify blocking behavior and client JSON types
Adding an optional field with a new tag (10) is backward compatible in proto3. To reduce surprises for clients, please document that applying WAL diffs may recreate optimizers and be blocking. Also, the PR description shows string values for numeric fields; the API expects numbers. Consider updating examples/docs to use numbers or add relaxed deserialization on the REST side.
Proposed inline proto comment tweak:
- optional WalConfigDiff wal_config = 10; // New WAL parameters for the collection
+ optional WalConfigDiff wal_config = 10; // New WAL parameters for the collection. May trigger optimizer recreation; update is blocking until applied.To verify end-to-end wiring and docs, you can run:
🏁 Script executed:
#!/bin/bash
# Verify UpdateCollection carries wal_config in generated code and conversions, and that it's applied in the TOC path.
set -euo pipefail
echo "[proto] UpdateCollection includes wal_config:"
rg -n "message UpdateCollection" lib/api/src/grpc/proto/collections.proto -A 20
echo "[generated grpc] UpdateCollection struct includes wal_config (if generated is checked in):"
rg -n "pub struct UpdateCollection" lib/api/src/grpc/qdrant.rs -A 20 || echo "generated file not found (ok if not checked in)"
echo "[conversions] wal_config mapping present:"
rg -n "wal_config" lib/storage/src/content_manager/conversions.rs -n -A 3 || echo "mapping not found"
echo "[apply path] wal_config diff applied to collection:"
rg -n "update_wal_config_from_diff" lib/storage/src/content_manager/toc/collection_meta_ops.rs -A 5 || echo "apply call not found"Length of output: 7820
Confirm end-to-end WAL config support; update proto comment and JSON numeric types
Verified that wal_config is correctly handled in:
lib/api/src/grpc/proto/collections.proto→UpdateCollectionmessage- Generated Rust gRPC struct in
qdrant.rs - Conversion mappings in
conversions.rs - Application path via
update_wal_config_from_diffincollection_meta_ops.rs
Please address the remaining documentation tweaks:
• In lib/api/src/grpc/proto/collections.proto (line 421), update the inline comment to surface blocking behavior:
- optional WalConfigDiff wal_config = 10; // New WAL parameters for the collection
+ optional WalConfigDiff wal_config = 10; // New WAL parameters for the collection. May trigger optimizer recreation; update is blocking until applied.• Review client-facing examples/docs that show string literals for numeric fields (e.g., "timeout": "30"). Either:
- Change them to use numeric values (e.g.,
"timeout": 30), or - Implement relaxed deserialization on the REST side to accept strings as numbers.
📝 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.
| optional WalConfigDiff wal_config = 10; // New WAL parameters for the collection | |
| optional WalConfigDiff wal_config = 10; // New WAL parameters for the collection. May trigger optimizer recreation; update is blocking until applied. |
| /// New WAL parameters for the collection | ||
| #[prost(message, optional, tag = "10")] | ||
| pub wal_config: ::core::option::Option<WalConfigDiff>, | ||
| } |
There was a problem hiding this comment.
Add nested validation for wal_config in UpdateCollection
wal_config is missing #[validate(nested)], so the validator derive won't enforce WalConfigDiff constraints (e.g., min ranges) when this field is provided. This is inconsistent with CreateCollection.wal_config and other nested fields in UpdateCollection (e.g., hnsw_config, quantization_config, strict_mode_config), which all have nested validation.
Apply:
/// New WAL parameters for the collection
#[prost(message, optional, tag = "10")]
+ #[validate(nested)]
pub wal_config: ::core::option::Option<WalConfigDiff>,🤖 Prompt for AI Agents
In lib/api/src/grpc/qdrant.rs around lines 753 to 756, the
UpdateCollection.wal_config field lacks #[validate(nested)] so WalConfigDiff
constraints are not enforced; add #[validate(nested)] immediately above the pub
wal_config field (matching other nested fields), ensure the validator derive
still applies to the parent struct, and run cargo test/cargo check to verify
validation is compiled and functioning.
💡 Verification agent
🧩 Analysis chain
Confirm the proto source also declares validation
If this file is generated, ensure the proto definition for UpdateCollection.wal_config is annotated so codegen emits #[validate(nested)] (same as CreateCollection.wal_config). Otherwise, regeneration will drop the fix.
🏁 Script executed:
#!/bin/bash
# Locate the proto and inspect UpdateCollection.wal_config for validation hints/options
set -euo pipefail
fd -a collections.proto | while read -r f; do
echo ">>> File: $f"
rg -n -A 10 -B 10 $'message\\s+UpdateCollection\\s*\\{' "$f" || true
rg -n -A 2 -B 2 $'wal_config\\s*=\\s*10' "$f" || true
doneLength of output: 2639
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect CreateCollection in the proto for wal_config and any validation options
echo "=== Proto: CreateCollection ==="
rg -n -A 5 -B 2 'message\s+CreateCollection\s*\{' lib/api/src/grpc/proto/collections.proto || true
rg -n -A 2 -B 2 'wal_config\s*=' lib/api/src/grpc/proto/collections.proto || true
# Inspect generated Rust for CreateCollection.wal_config annotations
echo "=== Rust: CreateCollection struct ==="
rg -n -A 5 -B 2 'struct\s+CreateCollection' lib/api/src/grpc/qdrant.rs || true
rg -n -A 2 -B 2 'wal_config' lib/api/src/grpc/qdrant.rs || trueLength of output: 3007
Annotate UpdateCollection.wal_config for nested validation
The generated UpdateCollection struct is missing #[validate(nested)] on its wal_config field. To preserve nested validation, add the same Protobuf option used in CreateCollection:
Points to address:
- lib/api/src/grpc/proto/collections.proto → in
message UpdateCollection, fieldwal_config = 10 - After regenerating, verify lib/api/src/grpc/qdrant.rs
UpdateCollectionstruct (around line 753) includes#[validate(nested)]
Suggested proto diff:
message UpdateCollection {
// ...
- optional WalConfigDiff wal_config = 10; // New WAL parameters for the collection
+ optional WalConfigDiff wal_config = 10 [(validate.rules).nested = true]; // New WAL parameters for the collection
}Run cargo run --bin prost-build (or your regen command) and confirm the generated Rust has:
#[prost(message, optional, tag = "10")]
#[validate(nested)]
pub wal_config: Option<WalConfigDiff>,🤖 Prompt for AI Agents
In lib/api/src/grpc/qdrant.rs around lines 753–756 the
UpdateCollection.wal_config field is missing the #[validate(nested)] attribute;
update lib/api/src/grpc/proto/collections.proto in the message UpdateCollection
to annotate the wal_config = 10 field with the same Protobuf option used in
CreateCollection (so the field carries the validate(nested) option), then re-run
the protobuf codegen (e.g., cargo run --bin prost-build) and confirm the
regenerated lib/api/src/grpc/qdrant.rs contains the wal_config field with both
#[prost(message, optional, tag = "10")] and #[validate(nested)] attributes.
| &self, | ||
| wal_config_diff: WalConfigDiff, | ||
| ) -> CollectionResult<()> { | ||
| { | ||
| let mut config = self.collection_config.write().await; | ||
| config.wal_config = wal_config_diff.update(&config.wal_config)?; | ||
| } | ||
| self.collection_config.read().await.save(&self.path)?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm whether runtime WAL needs reconfiguration on-the-fly
This updates and persists config, but does not touch any in-memory WAL components. If wal_capacity_mb, wal_segments_ahead, or wal_retain_closed affect WAL runtime behavior (segment rotation, retention, preallocation), you may need to propagate changes to the WAL subsystem so they take effect without restart. If the design is “persist now, apply on next startup,” please document that behavior.
Run this script to find potential WAL runtime update hooks and current usage of WalConfig to assess whether an in-memory reconfiguration step exists:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Search for WAL-related runtime update hooks:"
rg -n -A2 -B2 $'on_.*wal.*update|wal.*on_.*update|update.*wal'
echo
echo "Search for WalConfig/WalConfigDiff usage across the codebase:"
rg -n -A2 -B2 $'\\bWalConfig(Diff)?\\b|\\bwal_segments_ahead\\b|\\bwal_retain_closed\\b|\\bwal_capacity_mb\\b'Length of output: 212258
Runtime WAL parameters aren’t applied in‐memory—only persisted
The call to update_wal_config_from_diff in collection_ops.rs merely writes the new values to disk. We found no code path that re‐configures the live WAL:
- No “on_wal_update” or similar hook in
local_shard,wal_delta, or elsewhere. WalOptionsinconfig.rsis built once fromWalConfig(on startup) and never refreshed.- State‐management never invokes a WAL reconfiguration, only optimizer recreation.
Action items:
- Either implement a runtime hook that updates the in-memory WAL (e.g. expose a
wal.reconfigure(opts)method on your WAL handle and call it after persisting), or - Clearly document in
update_wal_config_from_diffand in the public API that WAL changes are only applied on next startup.
e51c0e1 to
886470d
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@generall suggested that this is a bit risky especially when user wants to decrease the WAL capacity. We can lose some data (if it's not applied yet). Also, nodes must be restarted for changes to be effective, and restarts can take significant time. Possible improvements:
We have seen this issue only with one large customer so we can postpone this PR till it's actually required in more real world clusters. cc @timvisee |
5d6f272 to
2d11af5
Compare
|
Merging #7475 introduced merge conflicts in this branch, so I rebased it. |
📝 WalkthroughWalkthroughThis PR adds support for WAL (Write-Ahead Log) configuration updates to collections. A new optional Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/api/src/grpc/qdrant.rs (1)
1247-1249: Add nested validation forUpdateCollection.wal_configvia proto options
UpdateCollection.wal_configis missing#[validate(nested)], soWalConfigDiff’s range constraints (e.g.wal_capacity_mb >= 1,wal_retain_closed >= 1) are not enforced on update requests, unlikeCreateCollection.wal_config. This is an API‑level validation gap on a risky config surface and should be aligned with the create path.Because
lib/api/src/grpc/qdrant.rsis prost‑generated, don’t edit this file directly. Instead, update the proto and regenerate:// lib/api/src/grpc/proto/collections.proto message UpdateCollection { // ... - optional WalConfigDiff wal_config = 11; // New WAL parameters for the collection + optional WalConfigDiff wal_config = 11 [(validate.rules).nested = true]; // New WAL parameters for the collection }After regenerating,
UpdateCollection.wal_configshould have both#[prost(message, optional, tag = "11")]and#[validate(nested)], matching other nested configs.
🧹 Nitpick comments (1)
docs/redoc/master/openapi.json (1)
10326-10336: WAL update field doc looks good; consider adding an explicit safety warningThe new
wal_configfield onUpdateCollectionis wired and described clearly, including the “effective only on Qdrant restart” behavior and the fact thatnullleaves it unchanged. From an API-surface perspective this looks correct.Given the earlier discussion about the risk of changing WAL settings (especially decreasing capacity/segments), it would be helpful to call that out explicitly in the description, e.g. that this is an advanced knob and that aggressive reductions can increase risk of data loss or failed recovery. That keeps the API powerful while nudging users toward safe usage.
Also, since
docs/redoc/master/openapi.jsonis generated, any wording tweaks should be done inlib/api/src/rest/schema.rs(theUpdateCollectionREST schema) and then re-generated. Based on learnings, ...
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/redoc/master/openapi.json(1 hunks)lib/api/src/grpc/proto/collections.proto(1 hunks)lib/api/src/grpc/qdrant.rs(1 hunks)lib/collection/src/collection/collection_ops.rs(1 hunks)lib/storage/src/content_manager/collection_meta_ops.rs(2 hunks)lib/storage/src/content_manager/conversions.rs(3 hunks)lib/storage/src/content_manager/mod.rs(1 hunks)lib/storage/src/content_manager/toc/collection_meta_ops.rs(2 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/collection/collection_ops.rslib/storage/src/content_manager/collection_meta_ops.rslib/storage/src/content_manager/conversions.rslib/api/src/grpc/qdrant.rslib/storage/src/content_manager/toc/collection_meta_ops.rslib/storage/src/content_manager/mod.rs
🧠 Learnings (7)
📚 Learning: 2025-09-01T11:42:06.964Z
Learnt from: timvisee
Repo: qdrant/qdrant PR: 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.
Applied to files:
lib/collection/src/collection/collection_ops.rs
📚 Learning: 2025-10-12T22:22:11.435Z
Learnt from: generall
Repo: qdrant/qdrant PR: 7388
File: lib/shard/src/segment_holder/flush.rs:167-191
Timestamp: 2025-10-12T22:22:11.435Z
Learning: In Qdrant's SegmentHolder flush logic, when calculating the minimum unsaved version for WAL acknowledgment, the code intentionally returns `segment_persisted_version` (not `segment_persisted_version + 1`) when there are unsaved changes. This conservative approach assumes the last persisted version might not have been fully applied and allows the system to safely retry operations when versions match, ensuring WAL consistency.
Applied to files:
lib/collection/src/collection/collection_ops.rs
📚 Learning: 2025-08-10T18:26:12.443Z
Learnt from: generall
Repo: qdrant/qdrant PR: 7006
File: docs/redoc/master/openapi.json:13645-13652
Timestamp: 2025-08-10T18:26:12.443Z
Learning: In qdrant, docs/redoc/master/openapi.json is auto-generated from the REST schemas. To change field docs, edit lib/api/src/rest/schema.rs (e.g., add doc comments or #[schemars(description = ...)]). Specifically, UpdateVectors.update_filter lacked a description and should state: "If specified, only update vectors for points that match this filter; points not matching the filter are left unchanged."
Applied to files:
lib/api/src/grpc/proto/collections.protodocs/redoc/master/openapi.jsonlib/api/src/grpc/qdrant.rs
📚 Learning: 2025-09-16T19:14:17.614Z
Learnt from: generall
Repo: qdrant/qdrant PR: 7183
File: lib/api/src/grpc/qdrant.rs:4263-4273
Timestamp: 2025-09-16T19:14:17.614Z
Learning: In qdrant, lib/api/src/grpc/qdrant.rs is auto-generated by prost-build; do not edit it directly. Make changes in lib/api/src/grpc/proto/points.proto (e.g., add [deprecated=true], doc comments, or encoding options), then regenerate the Rust code.
Applied to files:
lib/api/src/grpc/qdrant.rs
📚 Learning: 2025-08-10T18:30:02.986Z
Learnt from: generall
Repo: qdrant/qdrant PR: 7006
File: lib/collection/src/operations/verification/update.rs:158-174
Timestamp: 2025-08-10T18:30:02.986Z
Learning: In Qdrant's strict mode verification code (lib/collection/src/operations/verification/update.rs), exhaustive pattern matching without `..` is intentionally used for structs like PointsBatch and PointsList. This design pattern ensures compilation fails when new fields are added, forcing developers to explicitly consider how new fields should be handled in the indexed_filter_write method. This provides visibility and compile-time safety for struct evolution.
Applied to files:
lib/api/src/grpc/qdrant.rs
📚 Learning: 2025-08-15T15:56:36.821Z
Learnt from: coszio
Repo: qdrant/qdrant PR: 7065
File: lib/collection/src/operations/universal_query/shard_query.rs:0-0
Timestamp: 2025-08-15T15:56:36.821Z
Learning: Internal gRPC APIs in Qdrant don't require additional runtime validation when proto-level validation is already configured via build.rs range constraints.
Applied to files:
lib/api/src/grpc/qdrant.rs
📚 Learning: 2025-10-16T13:00:46.809Z
Learnt from: coszio
Repo: qdrant/qdrant PR: 7368
File: lib/shard/src/query/mod.rs:313-341
Timestamp: 2025-10-16T13:00:46.809Z
Learning: In the Qdrant codebase, for the `Feedback` query variant in `lib/shard/src/query/mod.rs`, validation for sparse vectors should happen down the line rather than early in the `query_enum_from_grpc_raw_query` function. The `Nearest` variant is treated as a special case where early validation with better error messages is acceptable.
Applied to files:
lib/api/src/grpc/qdrant.rs
🧬 Code graph analysis (1)
lib/collection/src/collection/collection_ops.rs (3)
lib/segment/src/index/sparse_index/sparse_vector_index.rs (1)
config(60-62)lib/segment/src/segment/entry.rs (1)
config(606-608)lib/segment/src/vector_storage/quantized/quantized_vectors.rs (1)
config(190-192)
⏰ 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). (10)
- GitHub Check: integration-tests
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: integration-tests-consensus
- GitHub Check: e2e-tests
- GitHub Check: test-consistency
- GitHub Check: rust-tests (ubuntu-latest)
- GitHub Check: test-consensus-compose
- GitHub Check: rust-tests (macos-latest)
- GitHub Check: rust-tests (windows-latest)
- GitHub Check: lint
🔇 Additional comments (7)
lib/storage/src/content_manager/mod.rs (1)
137-137: LGTM!The initialization of the new
wal_configfield toNoneis correct and consistent with the existing pattern. Sinceremove_replicais specifically for removing replicas and not for updating collection configuration,Noneis the appropriate value.lib/storage/src/content_manager/toc/collection_meta_ops.rs (2)
167-169: Correct handling of WAL config without optimizer recreation.The implementation correctly applies the WAL config diff without setting
recreate_optimizers = true, which aligns with the design that WAL configuration changes don't require optimizer recreation and only take effect after restart.
142-142: LGTM: WAL config properly destructured.The
wal_configfield is correctly destructured from theUpdateCollectionpayload, consistent with other config update fields.lib/storage/src/content_manager/conversions.rs (2)
7-7: LGTM!The import of
WalConfigDiffis consistent with other config diff types already imported.
171-192: LGTM!The WAL config conversion is properly wired through the
UpdateCollectionflow. The code follows the coding guidelines by using explicitWalConfigDiff::frominstead of implicit.into(), and the destructuring explicitly lists all fields includingtimeout: _, avoiding the catch-all..pattern. The implementation is consistent with other config diff conversions (e.g.,hnsw_configat line 191).lib/storage/src/content_manager/collection_meta_ops.rs (2)
245-247: LGTM!The
wal_configfield is properly declared with appropriate validation and documentation. The comment clearly states that changes take effect only after restart, which aligns with the PR objectives. The#[validate(nested)]attribute ensures proper validation of the nested structure.
273-289: LGTM!The
wal_configfield is properly initialized toNonein thenew_empty()method, consistent with other optional config fields.
| // New WAL parameters for the collection | ||
| optional WalConfigDiff wal_config = 11; |
There was a problem hiding this comment.
Document that WAL config changes require restart to take effect.
The comment should inform users that WAL configuration changes are persisted but only become effective after restarting the Qdrant node, as stated in the PR description.
Apply this diff to improve the documentation:
- // New WAL parameters for the collection
+ // New WAL parameters for the collection.
+ // Changes are persisted but only take effect after restarting the Qdrant node.
optional WalConfigDiff wal_config = 11;📝 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.
| // New WAL parameters for the collection | |
| optional WalConfigDiff wal_config = 11; | |
| // New WAL parameters for the collection. | |
| // Changes are persisted but only take effect after restarting the Qdrant node. | |
| optional WalConfigDiff wal_config = 11; |
🤖 Prompt for AI Agents
In lib/api/src/grpc/proto/collections.proto around lines 508 to 509, update the
comment above the wal_config field to state that while WAL configuration changes
are persisted, they only take effect after restarting the Qdrant node; modify
the comment to clearly mention persistence and the required restart so users
know changes are not applied dynamically.
| /// Updates WAL config: | ||
| /// Saves new params on disk | ||
| pub async fn update_wal_config_from_diff( | ||
| &self, | ||
| wal_config_diff: WalConfigDiff, | ||
| ) -> CollectionResult<()> { | ||
| { | ||
| let mut config = self.collection_config.write().await; | ||
| config.wal_config = config.wal_config.update(&wal_config_diff); | ||
| } | ||
| self.collection_config.read().await.save(&self.path)?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Document that WAL config changes require restart to take effect.
The method documentation correctly omits the optimizer recreation note (per previous review feedback), but should explicitly state that changes only become effective after restarting the Qdrant node.
Apply this diff to clarify the restart requirement:
/// Updates WAL config:
/// Saves new params on disk
+ ///
+ /// Changes only take effect after restarting the Qdrant node.
pub async fn update_wal_config_from_diff(🤖 Prompt for AI Agents
In lib/collection/src/collection/collection_ops.rs around lines 62 to 74, update
the triple-slash doc comment for update_wal_config_from_diff to explicitly state
that changes to the WAL config are persisted to disk but will only take effect
after the Qdrant node is restarted; modify the doc string to add a clear
single-line note about the restart requirement (no code changes needed).
|
We think it's better to do it like #7834 and hence closing this PR. |
Allow directly modifying WAL config via API call even after collection is created:
This makes it more likely for Qdrant to do WAL delta transfers (which is faster and preferred for large clusters)
Note that changes are effectively only after restarting Qdrant node.
Related to #6976 (review)