[improvement](fe) Presize global cloud tablet route sets - #66447
Open
deardeng wants to merge 3 commits into
Open
[improvement](fe) Presize global cloud tablet route sets#66447deardeng wants to merge 3 commits into
deardeng wants to merge 3 commits into
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#66378, apache#66389 Problem Summary: Cloud tablet route rebuilding creates new current and future global per-backend tablet sets on every `statRouteInfo()` round. Even when route cardinalities are stable, each `ConcurrentHashMap`-backed set starts without a sizing hint and repeatedly replaces its backing table while growing. Use the corresponding previous-generation per-backend tablet count as the initial-capacity hint for each rebuilt global set. The previous maps already remain live until the temporary routes are complete, so the hint does not extend their lifetime. First-round and newly seen backends retain the default unsized `newKeySet()` behavior. Table- and partition-level indexes, routing contents, scheduling decisions, and incremental update semantics are unchanged. A single-threaded JDK 17 multi-scale allocation model representing 4 million tablets across 4 clusters and both current/future global memberships estimates cumulative allocation on this path at 1.94 GiB before and 1.62 GiB after the change, a 16.14% reduction. Approximate post-GC retained heap is unchanged in the stable-cardinality model; these are path-level estimates, not production RSS measurements. ### Release note None ### Check List (For Author) - Test: Unit Test - `./run-fe-ut.sh --run org.apache.doris.cloud.catalog.CloudTabletRebalancerTest` (17 tests passed) - `mvn checkstyle:check -pl fe-core` (0 violations) - Single-threaded JDK 17 multi-scale allocation model - Behavior changed: No - Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 28828 ms |
Contributor
TPC-DS: Total hot run time: 166633 ms |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review status: changes requested. The review converged after two rounds: all normal full-coverage and risk-focused reviewers returned NO_NEW_VALUABLE_FINDINGS against the same frozen ledger, inline wording, and anchor. One P2 issue is reported inline: an unbounded previous-generation size hint can turn an abrupt route shrink into obsolete large allocations and repeated rebalancer OOME retries.
Critical checkpoint conclusions:
- Goal, correctness, and proof: Reusing prior cardinality does avoid resize churn when cardinality is stable, and the tests verify current/future hint selection plus unchanged memberships. The implementation does not safely accomplish that goal across a normal large-to-small catalog transition; the missing sharp-shrink oracle is the inline blocker.
- Scope and clarity: The production change is otherwise focused on the two global route sets. The helper split is clear and table/partition allocation behavior remains untouched.
- Concurrency: Route-set rebuild, transfer, preheat, and failed-task rollback mutations run on the rebalancer daemon thread. Warmup RPC workers only enqueue failures, while the checker updates inflight/replica state rather than the captured route maps. No new race, lock-order issue, or heavy operation under a new lock was found.
- Lifecycle: The two factories retain the matching prior current/future generation only for the synchronous rebuild and are not retained by
computeIfAbsent. No reference leak or circular/static-initialization issue was found. The stale-capacity failure/retry lifecycle is covered by the inline issue. - Configuration and compatibility: No configuration, persistence format, FE-BE protocol, function symbol, or rolling-upgrade contract changes are introduced.
- Parallel paths and conditions: Only full rebuilds use the sizing factories; direct transfer, preheat, rollback, and public/package helper paths preserve the default allocator. Null, first-round, absent-BE, and zero-size cases remain safe. The unbounded positive-size branch is the reported defect.
- Tests and results: The new unit tests are deterministic and their membership/capacity assertions are correct, but the tracking override bypasses the production allocator and therefore masks the shrink hazard. Per the runner contract, I performed a static-only review and did not run builds or tests; the author reports the targeted FE unit test and Checkstyle pass.
- Observability: No new steady-state metric or log requirement was identified. Existing daemon exception logging does not prevent the repeated allocation failure, so observability is not a substitute for the requested fix.
- Transactions, persistence, and data writes: This patch changes only in-memory scheduling indexes; it adds no EditLog path, transaction boundary, metadata persistence, or user-data write.
- Performance: Stable-cardinality allocation reduction is plausible, but the unbounded stale hint creates a material peak-memory/liveness regression during sharp shrink. No other hot-path allocation, repeated lookup, or route-content regression was found.
- Other issues: No additional substantiated correctness, lifecycle, compatibility, error-handling, or test-result issue remains after duplicate suppression and final sweep.
User focus: .code-review.Bbg2kk/review_focus.txt contains no additional focus, so the full PR was reviewed without narrowing scope.
Contributor
ClickBench: Total hot run time: 23.75 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: None Related PR: apache#66447 Problem Summary: Global cloud tablet route rebuilding uses the previous per-backend tablet count as an allocation hint. Add a focused unit-test reproducer showing that a stale count of two million is currently forwarded unchanged even when the rebuilt current and future routes each contain only one tablet. The test expects the hint to be bounded at 1,048,576 while preserving the rebuilt memberships. ### Release note None ### Check List (For Author) - Test: Unit Test - `./run-fe-ut.sh --run org.apache.doris.cloud.catalog.CloudTabletRebalancerTest` (expected RED: 18 tests run, the new sharp-shrink assertion is the only failure) - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66447 Problem Summary: Cloud tablet route rebuilding uses the previous generation per-backend tablet count to presize current and future global route sets. After a sharp catalog shrink, an obsolete multi-million-tablet count could force both replacement sets to allocate oversized ConcurrentHashMap backing tables even when they retain only a few tablets. Bound the previous-generation hint at 1,048,576 entries while preserving exact hints below that threshold and unchanged route memberships. A single-threaded JDK 17 model matching the production uncompressed-oops mode estimates that rebuilding two one-tablet sets from a four-million-tablet previous count reduces allocation from 128.01 MiB to 32.01 MiB (75.00%) and approximate post-full-GC retained heap from 130.00 MiB to 34.00 MiB (73.85%); these are path-level model estimates, not production RSS measurements. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-fe-ut.sh --run org.apache.doris.cloud.catalog.CloudTabletRebalancerTest (18 tests passed) - cd fe && mvn checkstyle:check -pl fe-core (pass) - Single-threaded JDK 17 allocation model with compressed and uncompressed object pointers - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-08T03:32:00Z. Please trigger /review again after that time. |
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
2 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.
Related PR: #66378, #66389
Problem Summary: Cloud tablet route rebuilding creates new current and future global per-backend tablet sets on every
statRouteInfo()round. Even when route cardinalities are stable, eachConcurrentHashMap-backed set starts without a sizing hint and repeatedly replaces its backing table while growing.Use the corresponding previous-generation per-backend tablet count as the initial-capacity hint for each rebuilt global set. The previous maps already remain live until the temporary routes are complete, so the hint does not extend their lifetime. First-round and newly seen backends retain the default unsized
newKeySet()behavior. Table- and partition-level indexes, routing contents, scheduling decisions, and incremental update semantics are unchanged.A single-threaded JDK 17 multi-scale allocation model representing 4 million tablets across 4 clusters and both current/future global memberships estimates cumulative allocation on this path at 1.94 GiB before and 1.62 GiB after the change, a 16.14% reduction. Approximate post-GC retained heap is unchanged in the stable-cardinality model; these are path-level estimates, not production RSS measurements.
Release note
None
Check List (For Author)
./run-fe-ut.sh --run org.apache.doris.cloud.catalog.CloudTabletRebalancerTest(17 tests passed)mvn checkstyle:check -pl fe-core(0 violations)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)