Register trackers only after successfull start by xzfc · Pull Request #7766 · qdrant/qdrant

xzfc · 2025-12-15T17:26:13Z

Problem

The optimizers log is polluted with empty cancelled trackers. Consequently, the optimizations endpoint added in #7625 includes many entries like this:

{
  "name": "Segment Optimizing",
  "started_at": "2025-12-09T16:06:18.594754171Z",
  "finished_at": "2025-12-09T16:06:18.944208155Z",
  "duration_sec": 0.349453884
},
{
  "name": "Segment Optimizing",
  "started_at": "2025-12-09T16:06:18.592540546Z",
  "finished_at": "2025-12-09T16:06:18.944175295Z",
  "duration_sec": 0.351634589
}

Solution

Postpone TrackerLog::register() after checking for all_segments_ok inside SegmentOptimizer::optimize.

coderabbitai · 2025-12-15T17:30:31Z

📝 Walkthrough

Walkthrough

This pull request adds a new on_successful_start callback parameter of type Box<dyn FnOnce()> to the SegmentOptimizer::optimize trait method signature. The callback is invoked once at the beginning of the optimization workflow after segment validation. All optimizer implementations' test calls are updated to pass a no-op closure as this parameter. In optimization_worker.rs, the tracker registration timing is adjusted to occur within the callback rather than before the optimize call, deferring it until optimization successfully starts.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

optimization_worker.rs — Requires careful attention to understand the timing implications of moving tracker registration from pre-call to the callback invocation and how this affects early-cancellation scenarios.
segment_optimizer.rs — Trait signature change with new parameter addition and callback invocation point; verify the callback is invoked at the correct stage in the workflow.
Test updates across multiple optimizer files — While homogeneous in pattern, verify all call sites consistently pass the new parameter without omission.

Possibly related PRs

add flag to break optimization loop on panic #6787 — Modifies the same SegmentOptimizer::optimize signature in segment_optimizer.rs, introducing potential conflicts or interdependencies.
Indexing progress #7625 — Also updates the SegmentOptimizer::optimize API and associated call sites across optimizer implementations.

Suggested reviewers

generall
timvisee

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: deferring tracker registration until after successful optimization start.
Description check	✅ Passed	The description is directly related to the changeset, explaining the problem of empty cancelled trackers and the solution of postponing tracker registration.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch tracker-register

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4e91702 and c3c22ea.

📒 Files selected for processing (6)

lib/collection/src/collection_manager/optimizers/config_mismatch_optimizer.rs (6 hunks)
lib/collection/src/collection_manager/optimizers/indexing_optimizer.rs (6 hunks)
lib/collection/src/collection_manager/optimizers/merge_optimizer.rs (1 hunks)
lib/collection/src/collection_manager/optimizers/segment_optimizer.rs (3 hunks)
lib/collection/src/collection_manager/optimizers/vacuum_optimizer.rs (3 hunks)
lib/collection/src/update_workers/optimization_worker.rs (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (.github/review-rules.md)

**/*.rs: Prefer explicit SomeType::from(x) over implicit x.into() in Rust
In new code, don't use transmute_from_u8, transmute_to_u8, transmute_from_u8_to_slice, transmute_from_u8_to_mut_slice, transmute_to_u8_slice - use bytemuck or zerocopy crates 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_manager/optimizers/indexing_optimizer.rs
lib/collection/src/collection_manager/optimizers/vacuum_optimizer.rs
lib/collection/src/collection_manager/optimizers/merge_optimizer.rs
lib/collection/src/collection_manager/optimizers/config_mismatch_optimizer.rs
lib/collection/src/collection_manager/optimizers/segment_optimizer.rs
lib/collection/src/update_workers/optimization_worker.rs

🧠 Learnings (2)

📚 Learning: 2025-07-11T11:35:21.549Z

Learnt from: generall
Repo: qdrant/qdrant PR: 6854
File: lib/segment/src/index/query_estimator.rs:320-327
Timestamp: 2025-07-11T11:35:21.549Z
Learning: In test code for Qdrant's query estimator (lib/segment/src/index/query_estimator.rs), simplified ID resolution logic using `id.to_string().parse().unwrap()` is acceptable for testing purposes and doesn't need to match production code's `id_tracker.internal_id()` approach. Test code can use mock implementations that serve the testing goals.

Applied to files:

lib/collection/src/collection_manager/optimizers/indexing_optimizer.rs

📚 Learning: 2025-10-13T22:58:03.121Z

Learnt from: generall
Repo: qdrant/qdrant PR: 7400
File: lib/segment/src/id_tracker/simple_id_tracker.rs:234-241
Timestamp: 2025-10-13T22:58:03.121Z
Learning: SimpleIdTracker in lib/segment/src/id_tracker/simple_id_tracker.rs is being deprecated and should not receive fixes related to version tracking or recovery logic, as it has a different version storage structure that is incompatible with newer trackers.

Applied to files:

lib/collection/src/update_workers/optimization_worker.rs

🧬 Code graph analysis (1)

lib/collection/src/collection_manager/optimizers/config_mismatch_optimizer.rs (3)

lib/collection/src/collection_manager/optimizers/indexing_optimizer.rs (1)

new (38-59)

lib/collection/src/collection_manager/optimizers/merge_optimizer.rs (1)

new (39-60)

lib/collection/src/collection_manager/optimizers/vacuum_optimizer.rs (1)

new (42-65)

⏰ 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: e2e-tests
GitHub Check: Build Qdrant Edge Python bindings
GitHub Check: rust-tests (macos-latest)
GitHub Check: test-consistency
GitHub Check: test-consensus-compose
GitHub Check: lint
GitHub Check: integration-tests
GitHub Check: integration-tests-consensus
GitHub Check: test-shard-snapshot-api-s3-minio
GitHub Check: rust-tests (ubuntu-latest)
GitHub Check: rust-tests (windows-latest)

🔇 Additional comments (19)

lib/collection/src/collection_manager/optimizers/merge_optimizer.rs (1)

261-261: LGTM!

The test correctly provides a no-op callback for the new on_successful_start parameter, which is appropriate since tests don't need tracker registration.

lib/collection/src/update_workers/optimization_worker.rs (1)

354-358: LGTM! Clean solution for deferring tracker registration.

The callback correctly captures the tracker and optimizers_log, deferring registration until after the optimization successfully starts. The tracker_handle obtained at line 344 remains valid for updating status (Done/Cancelled/Error) regardless of whether the callback is invoked.

lib/collection/src/collection_manager/optimizers/vacuum_optimizer.rs (3)

359-359: LGTM!

Test correctly updated with no-op callback for the new parameter.

513-513: LGTM!

Test correctly updated with no-op callback.

631-631: LGTM!

Test correctly updated with no-op callback.

lib/collection/src/collection_manager/optimizers/config_mismatch_optimizer.rs (6)

367-367: LGTM!

Test correctly updated with no-op callback.

401-401: LGTM!

Test correctly updated with no-op callback.

532-532: LGTM!

Test correctly updated with no-op callback.

574-574: LGTM!

Test correctly updated with no-op callback.

711-711: LGTM!

Test correctly updated with no-op callback.

756-756: LGTM!

Test correctly updated with no-op callback.

lib/collection/src/collection_manager/optimizers/indexing_optimizer.rs (6)

403-403: LGTM!

Test correctly updated with no-op callback.

554-554: LGTM!

Test correctly updated with no-op callback.

572-572: LGTM!

Test correctly updated with no-op callback.

703-703: LGTM!

Test correctly updated with no-op callback.

825-825: LGTM!

Test correctly updated with no-op callback.

996-996: LGTM!

Test correctly updated with no-op callback.

lib/collection/src/collection_manager/optimizers/segment_optimizer.rs (2)

605-614: LGTM! Well-designed callback interface.

The on_successful_start callback is appropriately typed as Box<dyn FnOnce()> since it should only be invoked once. The #[expect(clippy::too_many_arguments)] attribute acknowledges the parameter count while maintaining a clean API.

661-662: Callback placement is correct.

The callback is invoked at the right point in the optimization flow:

After check_process_stopped confirms no cancellation

After all_segments_ok validation passes (line 654-657)

Before creating temp segments and proxies

This ensures trackers are only registered for optimizations that will actually proceed.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Register trackers only after successfull start

c3c22ea

xzfc requested a review from generall December 15, 2025 17:26

xzfc mentioned this pull request Dec 15, 2025

Optimization panic status cleanup #7783

Merged

generall approved these changes Dec 16, 2025

View reviewed changes

generall merged commit 97fa802 into dev Dec 16, 2025
15 checks passed

generall deleted the tracker-register branch December 16, 2025 18:38

timvisee pushed a commit that referenced this pull request Dec 18, 2025

Register trackers only after successfull start (#7766)

188bd3a

This was referenced Jan 9, 2026

Prevent unoptimized updates #7643

Merged

OptimizationPlanner #7752

Merged

Expose planned optimizations in API #7784

Merged

coderabbitai bot mentioned this pull request Jan 21, 2026

Fix optimizer error if segment is already under optimization #7960

Merged

3 tasks

coderabbitai bot mentioned this pull request Feb 19, 2026

refactor/opt #8065

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Register trackers only after successfull start#7766

Register trackers only after successfull start#7766
generall merged 1 commit intodevfrom
tracker-register

xzfc commented Dec 15, 2025

Uh oh!

coderabbitai bot commented Dec 15, 2025

Walkthrough

Estimated code review effort

Possibly related PRs

Suggested reviewers

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

xzfc commented Dec 15, 2025

Problem

Solution

Uh oh!

coderabbitai bot commented Dec 15, 2025

Walkthrough

Estimated code review effort

Possibly related PRs

Suggested reviewers

Pre-merge checks and finishing touches

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants