Uh oh!
There was an error while loading. Please reload this page.
[opt](cloud) Add warm up job count metric on BE - #64734
Conversation
hello-stephen
commented
Jun 23, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
liaoxin01
commented
Jun 23, 2026
/review |
liaoxin01
commented
Jun 23, 2026
run buildall |
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds a new BE-local bvar metric to track how many file-cache warm-up jobs are currently resident in backend memory, incrementing/decrementing on state transitions so retries/dedup don’t skew counts.
Changes:
- Introduces
file_cache_warm_up_job_numas a process-localbvar::Adder<int64_t>. - Increments the metric when a job is first accepted (regular SET_JOB/SET_BATCH and event-driven LOAD add).
- Decrements the metric when a job is actually cleared (regular CLEAR_JOB and event-driven LOAD clear on successful erase).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
I found one correctness issue in the new metric lifecycle accounting.
Critical checkpoint conclusions:
- Goal/test: the PR adds a BE-local bvar for warm-up jobs currently held in memory, but the event-driven lazy cancellation path can remove a held job without decrementing the metric. Existing tests cover the lazy cleanup behavior, but not this new bvar value.
- Scope: the code change is small and focused on
CloudWarmUpManagermetric bookkeeping. - Concurrency/lifecycle: the touched state is protected by
_mtx; the issue is lifecycle completeness, not a lock race. Event-driven jobs can be cleared both by explicit CLEAR_JOB and by lazy FECANCELLEDcleanup, and only the explicit path updates the new metric. - Config/protocol/compatibility/storage: no new config, protocol, storage format, or rolling-upgrade compatibility concern found.
- Parallel paths: regular once/periodic job accounting and explicit event clear were checked; the missed parallel path is event-driven lazy cancellation.
- Observability: the added metric is useful, but currently misleading after missed/expired event-driven clears.
- Tests/CI:
git diff --checkis clean for the changed file. GitHub formatting/checkstyle and TeamCity compile checks are passing; the macOS BE UT job failed before tests because the runner has JDK 25 instead of required JDK 17, which appears unrelated. Several TeamCity regression/BE UT jobs are still pending.
User focus: no additional user-provided focus was supplied.
Subagent conclusions: TSC-1 and OR-1 were verified and merged into the single inline comment below. OR-2 was recorded as a duplicate. The existing Copilot comment on the redundant _cur_job_id != 0 guard was treated as already-known and not duplicated. Convergence round 1 ended with both subagents replying NO_NEW_VALUABLE_FINDINGS for the same final ledger/comment set.
Uh oh!
There was an error while loading. Please reload this page.
liaoxin01
commented
Jun 23, 2026
run buildall |
hello-stephen
commented
Jun 23, 2026
TPC-H: Total hot run time: 29496 ms |
hello-stephen
commented
Jun 23, 2026
TPC-DS: Total hot run time: 172553 ms |
hello-stephen
commented
Jun 23, 2026
ClickBench: Total hot run time: 25.26 s |
hello-stephen
commented
Jun 23, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Jun 26, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
gavinchou
commented
Jun 30, 2026
/review |
PR approved by at least one committer and no changes requested. |
PR approved by anyone and no changes requested. |
There was a problem hiding this comment.
I found one issue: the new BE warm-up job count metric is added without direct coverage for the lifecycle balance it depends on.
Critical checkpoint conclusions:
- Goal/test: the PR adds
file_cache_warm_up_job_numto report warm-up jobs currently held in BE memory, but no test directly proves the new counter balances across its lifecycle paths. - Scope: the implementation is small and focused on
CloudWarmUpManagermetric bookkeeping. - Concurrency/lifecycle: the touched manager state is protected by
_mtx; the reviewed risk is lifecycle completeness across the regular job slot, event-driven cache, and FE-CANCELLED lazy cleanup. - Config/protocol/compatibility/storage: no new config, protocol, storage format, persistence, or rolling-upgrade compatibility concern found.
- Parallel paths: regular SET_JOB/SET_BATCH/CLEAR_JOB, event SET/CLEAR, duplicate event updates, and lazy cancellation were inspected. The current head addresses the previously raised lazy-cleanup decrement path, but tests do not lock that contract down.
- Observability: the new bvar is useful, but it becomes misleading if any future cleanup path forgets to decrement.
- Tests/CI:
git diff --check HEAD -- be/src/cloud/cloud_warm_up_manager.cppis clean. Live PR checks show compile, formatter, main BE UT, cloud regression, and P0 regression passing; macOS BE UT and coverage are currently failing in external checks and were not diagnosed here. Local BE tests were not run because this checkout is missingthirdparty/installed.
User focus: no additional user-provided review focus was supplied.
Subagent conclusions: optimizer-rewrite reported NO_NEW_VALUABLE_FINDINGS. tests-session-config proposed TSC-001, which was accepted as main-001 and submitted as the inline comment. Convergence round 1 ended with both live subagents reporting NO_NEW_VALUABLE_FINDINGS for the same final ledger/comment set.
Uh oh!
There was an error while loading. Please reload this page.
Add a `file_cache_warm_up_job_num` bvar that tracks the number of warm up jobs currently held in a BE's memory. It is incremented when FE dispatches a new job to the BE (SET_JOB / SET_BATCH / event-driven SET_JOB) and decremented when the job is cleared (CLEAR_JOB / event-driven CLEAR_JOB). The increment/decrement are gated on actual state transitions (`_cur_job_id` going 0 -> job_id, and `_tablet_replica_cache.erase()` returning > 0), so retries, FE failover replays, and duplicate CLEAR_JOB requests do not skew the count.
Address review: the event-driven `_tablet_replica_cache` can also be erased in `get_replica_info()` when FE reports a warm up job as CANCELLED (lazy cleanup used when an explicit CLEAR_JOB is missed). That erase did not decrement `g_file_cache_warm_up_job_num`, so the metric could stay at 1 after the job is no longer held. Decrement the metric when this erase actually removes an entry, and erase the matching `_event_driven_filters` entry to stay consistent with the explicit CLEAR_JOB path in set_event().
5baed1b to
98978fbComparePR approved by at least one committer and no changes requested. |
liaoxin01
commented
Jul 9, 2026
/review |
There was a problem hiding this comment.
Automated review summary for PR #64734.
I reviewed the single changed file, be/src/cloud/cloud_warm_up_manager.cpp, plus the BE service dispatch and FE warm-up request paths. I am not adding new inline comments: the only substantiated concern found in this pass is already covered by existing inline thread discussion_r3496071818, which requests direct lifecycle tests for file_cache_warm_up_job_num. The lazy cancellation decrement added in get_replica_info() matches the explicit event CLEAR path and addresses the earlier metric-leak thread at discussion_r3459320733.
Critical checkpoint conclusions:
- Goal/test: the change adds a BE metric for warm-up jobs held in memory. The code gates increments on
_cur_job_idgoing from 0 to a real job or event cache insertion, and gates decrements on clear/cache erase/lazy cancel. Direct metric lifecycle coverage is still missing, already tracked by the existing inline thread. - Scope: the implementation is narrowly scoped to one BE cloud file and follows the existing bvar/manager-state pattern.
- Concurrency/lifecycle: changed regular and event-driven state transitions are under
_mtx; no new lock order or shutdown lifecycle issue was found. - Compatibility/config/session: no persisted format, thrift schema, config, or session propagation change is introduced.
- Parallel paths: regular SET_JOB/SET_BATCH/CLEAR_JOB, event SET/CLEAR, retry/duplicate clear, and FE-CANCELLED lazy cleanup paths were checked.
- Observability/performance: this is observability-only; no meaningful hot-path cost beyond bvar updates on existing state transitions was found.
- Tests: not run in this checkout because
.worktree_initializedandthirdparty/installedare missing. Validation was static.
Subagent conclusions:
optimizer-rewrite: no new valuable findings; no optimizer, join, aggregate, or Nereids rewrite paths are touched.tests-session-config: recordedTSC-DUP-001, a duplicate of existing threaddiscussion_r3496071818; no new non-duplicate finding.- Convergence round 1 ended with both live subagents returning
NO_NEW_VALUABLE_FINDINGSfor the same ledger/comment set.
User focus: .code-review.KbTfY8/review_focus.txt contains no additional user-provided focus.
hello-stephen
commented
Jul 9, 2026
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Jul 9, 2026
TPC-H: Total hot run time: 29635 ms |
hello-stephen
commented
Jul 9, 2026
TPC-DS: Total hot run time: 179709 ms |
hello-stephen
commented
Jul 9, 2026
ClickBench: Total hot run time: 25.17 s |
hello-stephen
commented
Jul 9, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
liaoxin01
commented
Jul 27, 2026
run buildall |
hello-stephen
commented
Jul 27, 2026
TPC-H: Total hot run time: 29895 ms |
hello-stephen
commented
Jul 27, 2026
TPC-DS: Total hot run time: 178048 ms |
hello-stephen
commented
Jul 27, 2026
ClickBench: Total hot run time: 25.07 s |
hello-stephen
commented
Jul 27, 2026
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Jul 27, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Uh oh!
There was an error while loading. Please reload this page.
Backport #64734 to branch-4.1. ### What problem does this PR solve? Issue Number: None Related PR: #64734 Problem Summary: Add the per-BE `file_cache_warm_up_job_num` bvar so operators can see how many warm-up jobs are currently held in BE memory. The count changes only on real job state transitions, so retry/replay and duplicate clear requests do not skew it. The lazy cancellation cleanup path also decrements the metric and removes the matching event filter. ### Release note Add the `file_cache_warm_up_job_num` BE metric. ### Check List (For Author) - Test: Manual test - clang-format 16 dry-run check passed for `be/src/cloud/cloud_warm_up_manager.cpp` - `git diff --check` passed - Local build was not run because this target-branch worktree does not provide `hooks/setup_worktree.sh` or `thirdparty/installed`; CI is pending - Behavior changed: Yes. A new per-BE warm-up job count metric is exposed. - Does this need documentation: No
## Proposed changes
Add a `file_cache_warm_up_job_num` bvar metric that tracks the number of
warm up jobs currently held in a BE's memory. This gives operators
per-BE visibility into how many warm up jobs each backend is currently
holding.
### What changed
In `be/src/cloud/cloud_warm_up_manager.cpp`:
- New `bvar::Adder<int64_t>
g_file_cache_warm_up_job_num("file_cache_warm_up_job_num")`.
- **+1** when FE dispatches a new job to this BE:
- `check_and_set_job_id` — regular (cluster/table) warm up `SET_JOB`, on
`_cur_job_id` transition `0 -> job_id`.
- `check_and_set_batch_id` — defensive, same `0 -> job_id` transition
(e.g. if a `SET_BATCH` were to arrive first).
- `set_event` — event-driven `SET_JOB`, when a new `job_id` is inserted
into `_tablet_replica_cache`.
- **-1** when the job is cleared:
- `clear_job` — regular `CLEAR_JOB`, only when a live job actually
existed.
- `set_event` (clear) — event-driven `CLEAR_JOB`, only when
`_tablet_replica_cache.erase()` actually removed an entry.
Proposed changes
Add a
file_cache_warm_up_job_numbvar metric that tracks the number of warm up jobs currently held in a BE's memory. This gives operators per-BE visibility into how many warm up jobs each backend is currently holding.What changed
In
be/src/cloud/cloud_warm_up_manager.cpp:bvar::Adder<int64_t> g_file_cache_warm_up_job_num("file_cache_warm_up_job_num").check_and_set_job_id— regular (cluster/table) warm upSET_JOB, on_cur_job_idtransition0 -> job_id.check_and_set_batch_id— defensive, same0 -> job_idtransition (e.g. if aSET_BATCHwere to arrive first).set_event— event-drivenSET_JOB, when a newjob_idis inserted into_tablet_replica_cache.clear_job— regularCLEAR_JOB, only when a live job actually existed.set_event(clear) — event-drivenCLEAR_JOB, only when_tablet_replica_cache.erase()actually removed an entry.Why it is dedup-safe
All increments/decrements are gated on real state transitions rather than on RPC arrival:
_cur_job_idis a single slot whose only reset to0is inclear_job(which carries the matching-1), so a regular job contributes exactly one+1and one-1per lifecycle. RepeatedSET_JOB(retry, FE failover replay) hits the_cur_job_id != 0guard and does not double count.!_tablet_replica_cache.contains(job_id)(add) anderase(job_id) > 0(clear), so duplicateSET_JOB/CLEAR_JOBare no-ops.The metric is process-local: on BE restart it resets to
0along with_cur_job_id/_tablet_replica_cache, so an abandoned-but-not-cleared job (e.g. BE down duringCLEAR_JOB) self-heals on restart.The value is exposed via the BE
/varsendpoint asfile_cache_warm_up_job_num.