[fix](cloud) Skip wait for async rowset warmup#62764
Open
bobhan1 wants to merge 4 commits into
Open
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Requesting changes because the new async non-positive-timeout path removes the only producer backpressure on event-driven rowset warm-up submission while the shared warm-up thread pool still has a bounded worker count and an unbounded queue.
Critical checkpoints:
- Goal of current task: The patch does make
sync_wait_timeout_ms <= 0return immediately, but it is not safe as written because the new queueing behavior can overload the warm-up pipeline under load. - Modification size/focus: Small and focused to one function.
- Concurrency: Applicable.
warm_up_rowset()submits onto_thread_pool_token;get_replica_info()/set_event()/clear_job()serialize on_mtx. The new fire-and-forget branch allows many more queued tasks contending on the same state, and the same token is also used byrecycle_cache(). - Lifecycle/static init: Applicable. Copying
RowsetMetaPBavoids the caller-stack lifetime problem forrs_meta; I did not find a new static-init issue. - Configuration: No new config. Existing
warm_up_manager_thread_pool_sizeremains the only concurrency cap, and the queue is still unbounded. - Compatibility/incompatible changes: No FE/BE protocol or storage-format compatibility change.
- Parallel code paths: Applicable. The positive-timeout path still waits; only the non-positive path changed.
- Special conditional checks: The new
sync_wait_timeout_ms <= 0branch is understandable. - Test coverage: No unit/regression test covers the new async lifetime/backpressure behavior.
- Test results changed: None in this PR.
- Observability: Existing logs/metrics are still enough to see failures, but they do not prevent backlog growth.
- Transaction/persistence: No EditLog or persistence change.
- Data writes/modifications: Rowset commit persistence is unchanged; the risk is the post-commit warm-up scheduling path.
- FE-BE variable passing: Unchanged.
- Performance: Lower caller latency is good, but removing backpressure from an unbounded queue can grow memory and delay synchronous
recycle_cache()work on the same token. - Other issues: See inline comment.
User focus points:
- No additional user-provided review focus.
|
|
||
| void CloudWarmUpManager::warm_up_rowset(RowsetMeta& rs_meta, int64_t sync_wait_timeout_ms) { | ||
| if (sync_wait_timeout_ms <= 0) { | ||
| auto rs_meta_pb = std::make_shared<RowsetMetaPB>(rs_meta.get_rowset_pb()); |
Contributor
There was a problem hiding this comment.
warm_up_rowset() is on the commit path for every load-generated rowset. Before this change each caller waited for its submitted task to finish, which implicitly bounded the number of outstanding warm-up jobs. With this fire-and-forget branch, callers can enqueue unlimited RowsetMetaPB copies while CloudWarmUpManagerThreadPool still only has warm_up_manager_thread_pool_size workers and its builder never sets max_queue_size, so the queue stays effectively unbounded. Under an active LOAD warm-up job, a slow FE replica lookup or slow target-BE RPC will now let this backlog grow without bound, and recycle_cache() submissions share the same token, so cache-recycle work can get stuck behind it. Please keep some form of backpressure or add a real queue bound here.
Contributor
Author
|
run beut |
Contributor
Author
|
run buildall |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
Author
|
run p0 |
1 similar comment
Contributor
Author
|
run p0 |
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: CloudWarmUpManager::warm_up_rowset still waited on a bthread condition variable when sync_wait_timeout_ms was non-positive. Submit those warmup tasks asynchronously and return immediately, while keeping the rowset meta alive for the background task and logging if rowset meta initialization fails.
### Release note
None
### Check List (For Author)
- Test: Manual test
- Ran build-support/clang-format.sh
- Ran build-support/check-format.sh
- Ran git diff --check
- Attempted build-support/run-clang-tidy.sh --build-dir be/ut_build_ASAN, blocked by existing compile environment errors including missing /usr/include/stddef.h and pre-existing tidy diagnostics
- Attempted ./build.sh --be, blocked before compilation by sandboxed submodule metadata write and network access
- Behavior changed: Yes. Non-positive warm_up_rowset sync wait timeout now submits the warmup task without waiting on the condition variable.
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary: Synchronous rowset warmup waited on a condition variable without a completion predicate, so a spurious wakeup could let the caller return before the warmup task finished.
### Release note
None
### Check List (For Author)
- Test: Build and style check
- build-support/check-format.sh be/src/cloud/cloud_warm_up_manager.cpp
- ./build.sh --be
- clang-tidy skipped per user request
- Behavior changed: No
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Add BE unit coverage for CloudWarmUpManager rowset warmup wait behavior, including fire-and-forget non-positive timeout ownership of RowsetMetaPB and positive-timeout resistance to spurious condition-variable wakeups.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- ./run-be-ut.sh --run --filter=CloudWarmUpManagerTest.* -j100
- build-support/check-format.sh be/src/cloud/cloud_warm_up_manager.cpp be/test/cloud/cloud_warm_up_manager_test.cpp
- git diff --check
- build-support/run-clang-tidy.sh --build-dir be/ut_build_ASAN (blocked by existing clang-tidy analysis errors: missing stddef.h and core/types.h NOLINTEND)
- Behavior changed: No
- Does this need documentation: No
bobhan1
force-pushed
the
fix-warm-up-sync-wait
branch
from
4f28fd3 to
f33fa2b
Compare
github-actions
Bot
removed
the
approved
Contributor
Author
|
run buildall |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run cloud_p0 |
Contributor
Author
|
run nonConcurrent |
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Add BE unit coverage for asynchronous rowset warmup when RowsetMetaPB initialization fails, ensuring the background task returns without entering rowset warmup.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- ./run-be-ut.sh --run --filter=CloudWarmUpManagerTest.* -j100
- build-support/check-format.sh be/src/cloud/cloud_warm_up_manager.cpp be/test/cloud/cloud_warm_up_manager_test.cpp
- git diff --check
- build-support/run-clang-tidy.sh --build-dir be/ut_build_ASAN (blocked by existing clang-tidy analysis errors: missing stddef.h, unmatched NOLINTEND in be/src/core/types.h, and pre-existing tidy warnings in cloud_warm_up_manager.cpp)
- Behavior changed: No
- Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29444 ms |
Contributor
TPC-DS: Total hot run time: 171066 ms |
Contributor
Author
|
run vault_p0 |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
gavinchou
approved these changes
May 25, 2026
gavinchou
approved these changes
May 25, 2026
Contributor
|
/review |
Contributor
There was a problem hiding this comment.
Review conclusion: request changes. I found a distinct blocking issue in the newly added test: it directly accesses a private CloudWarmUpManager member, so the BE unit test target will fail to compile. I did not duplicate the existing inline thread about unbounded fire-and-forget warm-up queueing; that concern remains already raised in the PR review context.
Critical checkpoint conclusions:
- Goal/test proof: the implementation adds async non-positive timeout warm-up and tests for copy lifetime, init failure, and spurious wakeups, but the new test currently cannot compile.
- Scope/minimality: production changes are focused on CloudWarmUpManager; tests add targeted coverage.
- Concurrency: the synchronous wait now uses a completion predicate; the existing review thread already covers the new async branch's queue/backpressure risk.
- Lifecycle: async rowset metadata lifetime is handled by copying RowsetMetaPB and reconstructing RowsetMeta in the task; manager/thread-pool lifetime appears covered by token shutdown semantics.
- Config/compatibility: no new config, storage format, or protocol compatibility changes.
- Parallel paths: recycle_cache still uses the same token synchronously; no new issue beyond the existing queueing thread.
- Tests: added BE tests are relevant but blocked by private member access.
- Observability: submission/init failures are logged and failed submissions increment the existing metric.
- User focus: no additional user-provided review focus was specified.
| bool release_blocker = false; | ||
|
|
||
| ASSERT_TRUE(manager._thread_pool_token | ||
| ->submit_func([&] { |
Contributor
There was a problem hiding this comment.
This test directly accesses CloudWarmUpManager::_thread_pool_token, but that member is private in cloud_warm_up_manager.h and this file does not use a #define private public test hack or a friend/test hook. BE_TEST only exposes consumer_job(), so the new BE unit test will fail to compile. Please expose a small test hook or drive the behavior through public/sync-point observable state instead of accessing the private token directly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: CloudWarmUpManager::warm_up_rowset still waited on a bthread condition variable when sync_wait_timeout_ms was non-positive. Submit those warmup tasks asynchronously and return immediately, while keeping the rowset meta alive for the background task and logging if rowset meta initialization fails.
Release note
None
Check List (For Author)
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)