Skip to content

queue: set schedule time for custom queue pushes - #93

Merged
cfzjywxk merged 1 commit into
tikv:masterfrom
lcwangchao:expose_more
Jul 1, 2026
Merged

queue: set schedule time for custom queue pushes#93
cfzjywxk merged 1 commit into
tikv:masterfrom
lcwangchao:expose_more

Conversation

@lcwangchao

@lcwangchao lcwangchao commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

What changed

This PR updates the custom queue adapter to set Extras::schedule_time before forwarding tasks through both custom::TaskInjector::push and custom::LocalQueue::push.

It also exposes read-only getters for multilevel::Config, so callers that build custom queues can reuse the multilevel defaults without duplicating them.

Finally, it relaxes a worker wake test to assert the scheduling guarantee instead of an exact resume count. The exact count is timing-sensitive because a late ensure_workers can wake an already-idle worker again after the task has been handled.

Why

Built-in queues set schedule_time when a task is pushed, and tracked runners rely on it to report queue wait time. Custom queues already reuse the same tracked runner metrics, but their adapter did not initialize this timestamp before handing tasks to the user-provided queue.

Without this, custom queues can miss schedule-wait accounting even when the task itself uses multilevel-compatible extras.

Tests

  • cargo fmt -- --check
  • RUSTFLAGS="-D warnings" cargo clippy --all --all-features -- -D clippy::all
  • RUSTFLAGS="-D warnings" cargo test --all --all-features -- --nocapture
  • RUSTFLAGS="-D warnings" cargo bench --all --all-features -- --test

Signed-off-by: Chao Wang <cclcwangchao@hotmail.com>
@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Custom queue task adapters (TaskInjector::push, LocalQueue::push) now require T: TaskCell and stamp schedule_time on tasks before enqueueing, with corresponding test updates. multilevel::Config gains public getter methods for name, level time thresholds, level0 proportion, and cleanup interval. A worker test assertion was relaxed from exact equality to a lower bound.

Changes

Queue scheduling metadata and config accessors

Layer / File(s) Summary
Schedule time stamping on push
src/queue/custom.rs
TaskInjector::push and LocalQueue::push now require T: TaskCell, set extras.schedule_time = Some(now()) before forwarding tasks, and tests assert schedule_time.is_some() for both injector and local pushes.
Multilevel Config getters
src/queue/multilevel.rs
Config adds get_name, get_level_time_threshold, get_level0_proportion_target, and get_cleanup_interval getters, validated by an expanded test_config_getters.
Worker resume metric assertion relaxed
src/pool/worker.rs
Test assertion for metrics.resume changed from == 1 to >= 1.

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant TaskInjector
  participant LocalQueue
  participant TaskQueue

  Caller->>TaskInjector: push(task_cell)
  TaskInjector->>TaskInjector: set extras.schedule_time = now()
  TaskInjector->>TaskQueue: forward stamped task_cell

  Caller->>LocalQueue: push(task_cell)
  LocalQueue->>LocalQueue: set extras.schedule_time = now()
  LocalQueue->>TaskQueue: forward stamped task_cell
Loading

Possibly related PRs

  • tikv/yatp#92: Overlaps directly with the schedule_time stamping in custom queue push and the worker wake/resume assertion changes.

Suggested reviewers: jiadebin, cfzjywxk

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures the main custom-queue change, though it omits the additional multilevel config getter updates.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/queue/custom.rs (1)

142-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Minor duplication across push implementations.

The stamp-then-forward body (task_cell.mut_extras().schedule_time = Some(now()); self.queue.push(task_cell);) is now identical in TaskInjector::push, LocalQueue::push here, and multilevel::LocalQueue::push. Could extract a tiny shared helper (e.g. a free function or trait default) to avoid triple maintenance if the stamping logic ever changes.

Also applies to: 174-180

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/queue/custom.rs` around lines 142 - 148, The push bodies in
TaskInjector::push, LocalQueue::push, and multilevel::LocalQueue::push are
duplicated; extract the shared “stamp schedule_time then forward to queue.push”
logic into a small common helper or trait default method. Update
LocalQueue::push and the other push implementations to call that shared helper
so the scheduling stamp stays consistent in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/queue/custom.rs`:
- Around line 142-148: The push bodies in TaskInjector::push, LocalQueue::push,
and multilevel::LocalQueue::push are duplicated; extract the shared “stamp
schedule_time then forward to queue.push” logic into a small common helper or
trait default method. Update LocalQueue::push and the other push implementations
to call that shared helper so the scheduling stamp stays consistent in one
place.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: db9a04b8-f84b-4157-afba-8c0266892965

📥 Commits

Reviewing files that changed from the base of the PR and between c2815a5 and d44069e.

📒 Files selected for processing (3)
  • src/pool/worker.rs
  • src/queue/custom.rs
  • src/queue/multilevel.rs

Comment thread src/queue/custom.rs
use super::{
multilevel::{MultiLevelMetrics, TrackedRunnerBuilder},
PopResult,
multilevel::{now, MultiLevelMetrics, TrackedRunnerBuilder},

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move these crates to some common lib? Introducing multilevel here is a bit strange.
Non-block

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can do it in another PR

@cfzjywxk
cfzjywxk merged commit 64ccad6 into tikv:master Jul 1, 2026
22 checks passed
@lcwangchao
lcwangchao deleted the expose_more branch July 1, 2026 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants