[fix](cloud) prevent warm-up job scheduling starvation - #67527
Open
bobhan1 wants to merge 3 commits into
Open
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review status: complete. The third and final review round converged with no new valuable findings; five distinct issues remain and are attached inline.
Summary of findings:
- Sequence-0 arrivals can indefinitely starve an already-RUNNING multi-step job, including its lease renewal and timeout checks.
- A mutable scheduler interval of 0 hot-spins JobDaemon, while a negative value terminates it with an uncaught IllegalArgumentException.
- The bounded heap eagerly allocates the full unbounded concurrency limit every cycle, even when almost no jobs are runnable.
- The tests synthesize rejection with a mock and do not protect the production throwing direct-handoff pool wiring required for rollback.
- Sorted submissions do not enforce ONCE-first destination admission because later worker threads can acquire the per-destination registration first.
Critical checkpoint conclusions:
- Goal and proof: Scheduler-side reservation fixes the original over-submission race, and a fixed finite job set rotates as intended. The PR does not fully achieve starvation-free/priority-correct scheduling because of findings 1 and 5. The unit tests cover bounded fixed-set selection, active reservation, synthesized retry, and positive resize, but not the production admission/rejection paths or the adverse schedules above.
- Scope: The implementation is localized to the manager plus one focused test file. The mutable-interval behavior is part of the stated PR scope.
- Concurrency: JobDaemon is the sole production scheduler/inserter and workers remove exact reservations in finally; no separate over-admission, leak, lock-order, or deadlock defect survived review. The remaining concurrency defect is worker-side destination admission racing after sorted handoff (finding 5).
- Lifecycle: PENDING, RUNNING, periodic wait/resume, event-driven bypass, cancellation, cleanup, replay, and master failover were traced. Cleanup and replay are otherwise consistent, but in-progress work can be denied all later state-machine turns by new arrivals (finding 1).
- Configuration: Positive pool growth/shrink and pause/resume are safe for the core-size-zero cached executor. Non-positive interval values are accepted without validation (finding 2), and an unbounded maximum drives eager per-cycle allocation (finding 3).
- Compatibility and parallel paths: No persisted field, journal opcode, RPC/thrift value, storage format, FE-BE variable, or rolling-upgrade contract changes. ONCE, PERIODIC, EVENT_DRIVEN, table, cancellation, and replay paths were checked; the supported ONCE/PERIODIC same-destination combination exposes finding 5.
- Tests and results: The added assertions are deterministic and restore modified globals. They miss continuous arrivals, invalid intervals, high-capacity allocation, real SynchronousQueue rejection, and delayed-first-worker destination admission. CheckStyle is green on the reviewed head; this review-only runner did not run builds or tests.
- Error handling and observability: Throwing rejection is rolled back and retried, accepted tasks release reservations in finally, and job IDs, destination-lock logs, rejection counts, and pool metrics remain available. The test gap in finding 4 leaves the critical production handler wiring unprotected.
- Persistence, transactions, and data writes: Scheduling history is intentionally in-memory; existing job edit-log/state transitions remain unchanged. No transaction, data-write, visibility, or delete-bitmap path is modified.
- Performance: Apart from finding 3, selection is bounded to O(N log slots) plus sorting retained slots; no additional CPU, memory, or redundant-work issue survived the final sweep.
- User focus: No additional user-provided focus was present, so the complete two-file PR was reviewed without a narrower focus.
Please address the five inline findings and add production-faithful concurrency/configuration coverage before merge.
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16709 ms |
Contributor
TPC-DS: Total hot run time: 81970 ms |
Contributor
ClickBench: Total hot run time: 14.65 s |
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
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:
When cloud warm-up jobs outnumber the configured active slots, the scheduler can over-submit tasks because a job is recorded as active only after its worker starts. The direct-handoff thread pool previously discarded rejected tasks, and unordered map traversal could repeatedly favor the same jobs, leaving other runnable jobs pending indefinitely.
This PR keeps the direct-handoff pool and makes scheduling bounded and retryable:
Release note
Fix cloud warm-up jobs remaining pending when the scheduler thread pool is saturated.
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)