core, server/cluster: isolate preparing range size scans - #11098
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesThe PR adds an independently locked, generation-aware
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant checkStores
participant RegionsInfo
participant regionSizeTree
checkStores->>RegionsInfo: request preparing region size
alt bounded range
RegionsInfo->>regionSizeTree: query approximate range size
regionSizeTree-->>RegionsInfo: return size-tree total
else unbounded range
RegionsInfo-->>checkStores: return root-tree total
end
RegionsInfo-->>checkStores: return selected region size
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #11098 +/- ##
==========================================
+ Coverage 79.17% 79.59% +0.41%
==========================================
Files 541 545 +4
Lines 76487 78046 +1559
==========================================
+ Hits 60558 62117 +1559
+ Misses 11629 11608 -21
- Partials 4300 4321 +21
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
f096fac to
3b53c40
Compare
3b53c40 to
cd99024
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@pkg/core/region_size_index.go`:
- Around line 78-90: Prevent stale removals from deleting newer indexed
metadata: in pkg/core/region_size_index.go lines 78-90, update
regionSizeIndex.removeIfCurrent to compare the indexed RegionInfo
version/metadata under i.mu, reject older removal requests, and remove only the
matching indexed state; in pkg/core/region.go lines 1508-1514, preserve the
existing lock-free ordering while pass through the metadata needed so delayed
removal cannot erase an update completed after removeRegionFromSubTree.
In `@pkg/core/region.go`:
- Around line 1203-1206: Update updateSubTreeCacheOrderInsensitive to return
whether the region metadata was accepted, then have the surrounding update flow
call regionSizeIndex.updateIfCurrent only when that result indicates success.
Skip the index write when stale metadata is rejected, while preserving the
existing generation handling and lock-order behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ae712d70-fb1c-4502-b4e7-555291de4805
📒 Files selected for processing (6)
pkg/core/region.gopkg/core/region_size_index.gopkg/core/region_size_index_test.gopkg/core/region_test.goserver/cluster/cluster.goserver/cluster/cluster_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- server/cluster/cluster.go
- server/cluster/cluster_test.go
- pkg/core/region_test.go
ee5d12f to
59e70d1
Compare
c0aebf3 to
bcf971e
Compare
Signed-off-by: lhy1024 <19542290+lhy1024@users.noreply.github.com>
bcf971e to
578c0ac
Compare
Signed-off-by: lhy1024 <19542290+lhy1024@users.noreply.github.com>
Signed-off-by: lhy1024 <19542290+lhy1024@users.noreply.github.com>
Start the asynchronous size index with the primary RaftCluster instead of on the first Preparing query. Use its eventually consistent result directly and defer bounded Preparing checks only until the initial build is ready. Remove root-tree confirmation scans, implicit cache-use state, the one-shot rebuild flag, single-use wrappers, and redundant tests. Signed-off-by: lhy1024 <19542290+lhy1024@users.noreply.github.com>
bufferflies
left a comment
There was a problem hiding this comment.
Follow-up to my earlier review comment, anchoring the specific suggestions to code (see inline comments below).
Signed-off-by: lhy1024 <19542290+lhy1024@users.noreply.github.com>
[LGTM Timeline notifier]Timeline:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bufferflies The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
JmPotato
left a comment
There was a problem hiding this comment.
Focused follow-up review of the Region size index update and freshness path. The root-mutation hook and ID-based reconciliation look sound; the inline comments cover the remaining actionable correctness, coverage, and validation issues.
Non-line items: please rewrap the two over-80-character body lines in commit c97a13c2f, and rerun or clearly attribute the currently failing required checks before merge.
| log.Debug("store preparing threshold", zap.Uint64("store-id", storeID), | ||
| zap.Float64("threshold", threshold), | ||
| zap.Float64("region-size", regionSize)) | ||
| readyToServe = regionSize >= threshold |
There was a problem hiding this comment.
[P2] Do not promote from an index that may still be behind
isReady() only means that the initial build completed; it does not account for pending or activePending. Under backlog, a stale-low range size can make this comparison pass and ReadyToServeLocked commits the one-way Preparing → Serving transition. Later reconciliation fixes the index but cannot undo that transition. Please require a caught-up/fresh generation for this decision, or perform one authoritative root-tree confirmation when the approximate result would promote the store. Add a pending-backlog test that proves the store is not promoted early.
There was a problem hiding this comment.
We are intentionally accepting this behavior as part of the contract: Region-size statistics are eventually consistent and Preparing -> Serving uses an approximate threshold. A stale-low value may advance the one-way transition and a stale-high value may delay it; the existing preparing timeout remains the fallback. We will not add a freshness gate or an authoritative root confirmation here, because that would reintroduce the O(N) root-tree scan and the lock contention this PR is intended to isolate. Backlog/oldest-pending metrics are available for operational visibility.
|
|
||
| func (t *regionSizeTree) getRegionSizeByRange(startKey, endKey []byte) int64 { | ||
| var size int64 | ||
| for { |
There was a problem hiding this comment.
[P1] Validate the CPU objective at the reported scale
This remains an O(N) scan; the change isolates it from the Root/Subtree locks but does not by itself prove that the high Preparing CPU reported in #9574 is resolved. The current single-scan microbenchmark does not cover 50 Preparing stores with concurrent Region heartbeats. Please provide a comparative workload measuring total CPU, heartbeat throughput/p99, Root lock wait, memory, and pending convergence lag at million-Region scale before treating the original performance goal as closed.
There was a problem hiding this comment.
Agreed that a production-scale comparative benchmark would be useful, but this is validation for the broader performance goal rather than a correctness issue in this PR. #11098 specifically decouples bounded range-size reads from the Root/Subtree heartbeat locks; the index query remains O(N), so this PR does not claim to close the total CPU problem from #9574. Million-Region concurrent heartbeat/p99 and pending-lag measurements, together with the O(log N) aggregate design, remain follow-up work tracked in #11082.
Signed-off-by: lhy1024 <19542290+lhy1024@users.noreply.github.com>
|
/test pull-unit-test-next-gen-3 |
|
/test pull-unit-test-next-gen-2 |
|
/test pull-integration-realcluster-test |
| // sizeTree is enabled only by the primary PD service. It never holds the | ||
| // root-tree or subtree lock while updating the size index. | ||
| sizeTreeMu syncutil.Mutex | ||
| sizeTree atomic.Pointer[regionSizeTree] |
There was a problem hiding this comment.
Why use an atomic.Pointer while also having a mutex lock?
There was a problem hiding this comment.
They protect different concerns. sizeTreeMu serializes the compound Start/Stop lifecycle so concurrent lifecycle calls cannot create overlapping workers or stop a worker while it is being started. The atomic.Pointer lets heartbeat mutation, query, reset, and metrics paths observe the optional tree without taking the lifecycle mutex. Using only the mutex would add locking to the heartbeat hot path, while using only the atomic pointer would require a more complex lifecycle state machine.
What problem does this PR solve?
After #11072 removes duplicate scans, each unique non-empty Preparing range still scans the root Region tree. Region heartbeats update the same tree under its write lock, so a large range query can delay routing metadata updates.
Using the existing
overlapTreewould move contention away from the root tree, but would make range queries contend with all subtree readers and writers underst.Issue Number: ref #9574, ref #11082
What is changed and how does it work?
The size tree stores compact ID/start/end/size entries rather than retaining full
RegionInfoobjects. It retains immutable range-key slices captured from root state; a later same-range heartbeat may replace the rootRegionInfowithout refreshing those slices, so the index can retain one older key pair per Region until a range change, reset, or stop. Repeated same-range heartbeats do not accumulate additional copies.No size-tree operation waits for its lock while holding the root-tree lock
tor subtree lockst. A range query can delay the size-tree worker for the duration of that query, but cannot delay the authoritative root or subtree heartbeat update.Observability is provided by:
pd_core_region_size_tree_readypd_core_region_size_tree_pendingpd_core_region_size_tree_oldest_pending_duration_secondspd_core_region_size_tree_rebuild_duration_secondsThe gauges are sampled by the primary RaftCluster's periodic metrics collector, including when Scheduling Service is independent, rather than updated on the Region-heartbeat path.
Check List
Tests
make gotest GOTEST_ARGS='./pkg/core ./server/cluster'make gotest GOTEST_ARGS='./pkg/core ./server/cluster -run "TestRegionSizeTree|TestPreparingRegionSize|TestCheckStoreDefersPreparing" -race -count=3'make gotest GOTEST_ARGS='./pkg/core ./server/cluster -tags=nextgen -run "TestRegionSizeTree|TestPreparingRegionSize|TestCheckStoreDefersPreparing" -count=1'make checkThe tests cover initial build and readiness, batched root access, range-query semantics, context cancellation, reset/rebuild interleavings, bounded pending cancellation, root/subtree lock isolation, pending coalescing, split/merge, delayed notifications, overlap replacement across all root mutation entry points, unavailable Preparing thresholds, and primary-PD metric collection in independent Scheduling Service mode.
Preliminary benchmark
Temporary local benchmark on an AMD Ryzen 7 7840S. The rebuild measurement uses the real batched root
ScanRegionspath and was taken immediately after rebuild, while captured range-key slices still shared backing arrays with the then-current rootRegionInfo.The 1M memory result is about 116 MiB. A linear 10M extrapolation is about 1.13 GiB. After ordinary same-range heartbeats replace root
RegionInfoobjects, the index may additionally retain one previous start/end key backing pair per Region. Production-scale heartbeat p99 and churn-inclusive memory benchmarks remain useful for quantifying the total benefit and cost.Side effects and scope
Related change: this builds on the per-round range cache merged in #11072.
Release note