Repository navigation
statistics: filter stale write samples after leader transfers - #11446
Conversation
Detect leader transfers from Region metadata before write tasks run. Skip the first stale sample even when an inactive Region has no hot-cache entry, so historical writes cannot raise adaptive thresholds and cool continuously hot peers. Signed-off-by: Ryan Leung <rleungx@gmail.com>
ti-chi-bot
Bot
added
release-note
Walkthrough
Merge Risk:
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title "statistics: filter stale write samples after leader transfers" matches the main change. The raw summary and objectives show that write samples after a leader transfer are now skipped for un… Description check The description follows the repository template. It includes the problem statement, an issue link ("close #4481, ref#11409"), a commit-message block, a checklist with unit tests and manual verificati…Linked Issues check Issue [ #4481] is open and directly linked. It reports a large cache-entry increase after leader transfers, but it has no detailed acceptance criteria. The PR addresses this behavior. `newRegionFlowTas…Out of Scope Changes check The changes remain within issue [ #4481].HotCache,HotPeerCache, and heartbeat-handler changes carry transfer state into asynchronous write-statistics processing.pkg/core/region.gostores a ter…Full details: Title check
-
Explanation
The title "statistics: filter stale write samples after leader transfers" matches the main change. The raw summary and objectives show that write samples after a leader transfer are now skipped for uncached peers and the transfer guard is reused for cached peers. The title follows the repository's "pkg: what's changed" format and names the affected package.
Full details: Description check
-
Explanation
The description follows the repository template. It includes the problem statement, an issue link ("close
#4481, ref#11409"), a commit-message block, a checklist with unit tests and manual verification noted, and a release note. The commit-message block is filled in. The checklist items are not explicitly ticked and the code-change and side-effect items are not addressed, but these are non-critical gaps. The problem, the fix, the tests, and the release note are all clearly described.
Full details: Linked Issues check
-
Explanation
Issue [
#4481] is open and directly linked. It reports a large cache-entry increase after leader transfers, but it has no detailed acceptance criteria. The PR addresses this behavior.newRegionFlowTaskscaptures leader changes and term increases from the heartbeat origin.checkPeerFlowForRegionskips the first write sample for transferred regions before admission and threshold calculation. Cached peers retain their existing state, and later fresh samples are admitted. The heartbeat handlers pass the origin snapshot before publication. Tests cover cold and cached peers, returning leaders, term changes, threshold stability, and later samples.
Full details: Out of Scope Changes check
-
Explanation
The changes remain within issue [
#4481].HotCache,HotPeerCache, and heartbeat-handler changes carry transfer state into asynchronous write-statistics processing.pkg/core/region.gostores a term-only change so later heartbeats receive updated origin metadata. The added tests verify this transfer and term handling. No unrelated production behavior or unrelated test scope is established.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 10 functions across 8 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
ti-chi-bot
Bot
added
the
size/L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11446 +/- ##
==========================================
+ Coverage 85.01% 85.03% +0.02%
==========================================
Files 555 555
Lines 71856 71870 +14
==========================================
+ Hits 61085 61114 +29
+ Misses 10770 10755 -15
Partials 1 1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Pass the previous Region snapshot from heartbeat validation through the PD and scheduling-service statistics paths. Keep existing entry points available, and copy the task snapshot inside the worker so it stays captured by value. Move transfer regressions into hot_cache_test.go and cover both entry points, keeping the PR free of new files. Signed-off-by: Ryan Leung <rleungx@gmail.com>
| } | ||
| if origin != nil { | ||
| previousLeader := origin.GetLeader().GetStoreId() | ||
| regionInfo.leaderTransferred = previousLeader != 0 && regionInfo.leaderStoreID != 0 && previousLeader != regionInfo.leaderStoreID |
There was a problem hiding this comment.
Could we also detect a leadership change when the leader returns to the same store between the heartbeats received by PD?
For example, PD last saw leader A at term 10, the Region went A -> B -> A, and B's heartbeat never reached PD. The next heartbeat reports A at term 12. This condition treats it as unchanged because the store IDs match, so the returning leader's first sample can still admit accumulated historical writes into TopN. The higher term is available in both snapshots but is not considered here.
I added the two focused tests below. The first checks initial cold-cache admission and also verifies that a subsequent fresh sample is accepted. The second checks the actual threshold/scheduling-stat impact using 64 cold Regions reporting 2 MiB / 500 s and one continuously hot Region at 2400 B/s:
Case on 3fc81b29a6 |
Historical hot-peer entries in the first test | Byte threshold in the second test | Steady Region entries returned with minHotDegree=3 |
|---|---|---|---|
| Observed A -> B, term 10 -> 11 | 0 | 1024 B/s | 3 |
| A -> B -> A, PD sees term 10 -> 12 on A | 3 | 3355.2 B/s | 0 |
Both cases fail on the PR base 4ab9d1679e; the ordinary A -> B case passes on this head. The returning-leader case is therefore a remaining gap in the fix, rather than a new regression.
The threshold test expires only the cached threshold timestamp inside the owning write worker so the real calcHotThresholds() recalculates it without a wall-clock wait. Production code and the threshold formula are unchanged.
Save this complete file as pkg/statistics/hot_write_transfer_review_test.go; it uses the existing newBenchmarkRegion fixture. I reran both added tests on 3fc81b29a6, with failpoints enabled and disabled afterward. No existing tests or CI were run.
make gotest GOTEST_ARGS='./pkg/statistics -run TestAudit -count=1 -v'package statistics
import (
"fmt"
"testing"
"time"
"github.com/stretchr/testify/require"
"github.com/pingcap/kvproto/pkg/metapb"
"github.com/pingcap/kvproto/pkg/pdpb"
"github.com/tikv/pd/pkg/core"
"github.com/tikv/pd/pkg/statistics/utils"
)
func TestAuditColdWriteTransferSkipsHistoricalSample(t *testing.T) {
for _, tc := range []struct {
name string
leaderStore uint64
term uint64
}{
{name: "observed-new-leader", leaderStore: 2, term: 11},
{name: "leader-returns-between-observed-heartbeats", leaderStore: 1, term: 12},
} {
t.Run(tc.name, func(t *testing.T) {
cluster := core.NewBasicCluster()
region := newBenchmarkRegion(1, 3).Clone(core.SetRegionVersion(10), core.SetRegionConfVer(10), core.SetWrittenBytes(0), core.SetWrittenKeys(0))
withTerm := func(r *core.RegionInfo, term uint64) *core.RegionInfo {
return core.RegionFromHeartbeat(&pdpb.RegionHeartbeatRequest{
Region: r.GetMeta(), Leader: r.GetLeader(), Term: term,
Interval: r.GetInterval(), BytesWritten: r.GetBytesWritten(), KeysWritten: r.GetKeysWritten(),
}, 0)
}
region = withTerm(region, 10)
for _, peer := range region.GetPeers() {
cluster.PutStore(core.NewStoreInfo(&metapb.Store{Id: peer.GetStoreId()}))
}
cluster.PutRegion(region)
cache := NewHotCache(t.Context(), cluster)
cache.CheckRegionFlowAsync(region)
require.Empty(t, cache.GetHotPeerStats(utils.Write, 0), "the region starts cold")
// In the second case, the intermediate leader's heartbeat never
// reached PD; the returning store reports writes from its last stint.
historical := withTerm(region.Clone(core.WithLeader(region.GetStorePeer(tc.leaderStore)), core.SetWrittenBytes(2<<20), core.SetReportInterval(0, 500)), tc.term)
_, _, err := cluster.PreCheckPutRegion(historical)
require.NoError(t, err)
cache.CheckRegionFlowAsync(historical)
_, err = cluster.CheckAndPutRootTree(&core.MetaProcessContext{Tracer: core.NewNoopHeartbeatProcessTracer()}, historical)
require.NoError(t, err)
afterHistorical := cache.GetHotPeerStats(utils.Write, 0)
count := 0
for _, stats := range afterHistorical {
count += len(stats)
}
t.Logf("root leader=%d term=%d; historical sample created %d write-hot peers", cluster.GetRegion(region.GetID()).GetLeader().GetStoreId(), cluster.GetRegion(region.GetID()).GetTerm(), count)
// A normal subsequent report must still be admitted.
fresh := historical.Clone(core.SetWrittenBytes(8000*60), core.SetReportInterval(500, 560))
cache.CheckRegionFlowAsync(fresh)
cluster.PutRegion(fresh)
afterFresh := cache.GetHotPeerStats(utils.Write, 0)
require.Len(t, afterFresh, 3)
require.Zero(t, count, "the first historical sample must not populate TopN after a leadership term change")
})
}
}
func TestAuditReturningLeaderDoesNotRaiseWriteThreshold(t *testing.T) {
for _, returnToSameStore := range []bool{false, true} {
t.Run(fmt.Sprintf("return-to-same-store=%t", returnToSameStore), func(t *testing.T) {
cluster := core.NewBasicCluster()
for id := uint64(1); id <= 3; id++ {
cluster.PutStore(core.NewStoreInfo(&metapb.Store{Id: id}))
}
cache := NewHotCache(t.Context(), cluster)
newRegion := func(id, term, leader, bytes, start, end uint64) *core.RegionInfo {
r := newBenchmarkRegion(id, 3).Clone(core.SetRegionVersion(10), core.SetRegionConfVer(10), core.WithStartKey([]byte(fmt.Sprintf("%08d", id))), core.WithEndKey([]byte(fmt.Sprintf("%08d", id+1))))
return core.RegionFromHeartbeat(&pdpb.RegionHeartbeatRequest{
Region: r.GetMeta(), Leader: r.GetStorePeer(leader), Term: term,
BytesWritten: bytes, Interval: &pdpb.TimeInterval{StartTimestamp: start, EndTimestamp: end},
}, 0)
}
report := func(r *core.RegionInfo) {
cache.CheckRegionFlowAsync(r)
cluster.PutRegion(r)
}
steady := newRegion(10000, 10, 1, 2400*60, 0, 60)
for range 12 {
report(steady)
}
require.Len(t, cache.GetHotPeerStats(utils.Write, 3), 3)
for id := uint64(1); id <= 64; id++ {
report(newRegion(id, 10, 1, 0, 0, 60))
}
_ = cache.GetHotPeerStats(utils.Write, 0)
leader, term := uint64(2), uint64(11)
if returnToSameStore {
leader, term = 1, 12
}
for id := uint64(1); id <= 64; id++ {
report(newRegion(id, term, leader, 2<<20, 0, 500))
}
threshold := make(chan float64, 1)
require.True(t, cache.CheckWriteAsync(func(c *HotPeerCache) {
for _, value := range c.thresholdsOfStore {
value.updatedTime = time.Now().Add(-2 * ThresholdsUpdateInterval)
}
threshold <- c.calcHotThresholds(1)[utils.ByteDim]
}))
rate := <-threshold
report(steady)
visible := 0
for _, stats := range cache.GetHotPeerStats(utils.Write, 3) {
for _, stat := range stats {
if stat.RegionID == steady.GetID() {
visible++
}
}
}
t.Logf("return-to-same-store=%t: threshold=%.4f B/s, schedulable steady peers=%d", returnToSameStore, rate, visible)
require.Equal(t, 3, visible, "historical samples must not hide continuously hot peers after the leader returns")
require.Equal(t, 1024.0, rate)
})
}
}Observed on 3fc81b29a6:
observed-new-leader: historical sample created 0 write-hot peers
leader-returns-between-observed-heartbeats: historical sample created 3 write-hot peers
return-to-same-store=false: threshold=1024.0000 B/s, schedulable steady peers=3
return-to-same-store=true: threshold=3355.2000 B/s, schedulable steady peers=0
Treat a higher known raft term as a leadership change even when PD sees the same leader store on both heartbeats. Preserve the fallback for unknown terms and refresh region metadata on term increases so subsequent fresh write samples are admitted normally. Extend transfer regressions for cached and uncached peers, threshold stability, both entry points, and term-only heartbeat publication. Signed-off-by: rleungx <rleungx@gmail.com>
ti-chi-bot
Bot
added
size/XL
|
/retest |
|
/test pull-integration-realcluster-test |
lhy1024
left a comment
lhy1024
left a comment
There was a problem hiding this comment.
Reviewed commit 376708978e82a43ec22ff088dc8a8c6a11ade8f8 and the monolithic PD and scheduling-service call paths.
The independent reproducers for ordinary leader transfers and returning leaders pass: the first historical write sample creates no hot-cache entries, the byte threshold remains at 1,024 B/s, and the steady hot peers remain visible. I found no additional blocking issue in this review.
Validation was limited to focused regression reproducers; I did not run a test suite or inspect CI results.
ti-chi-bot
Bot
added
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lhy1024, YuhaoZhang00 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 |
[LGTM Timeline notifier]Timeline:
|
What problem does this PR solve?
A cold Region's first write sample after a leader transfer bypasses the
existing transfer guard when no hot-cache entry remains. Historical writes
can then enter TopN and raise adaptive thresholds, excluding continuously
hot peers from scheduling.
Issue Number: close #4481, ref #11409
What is changed and how does it work?
Check List
Tests
filtered out; the fix preserves all 96 and keeps the threshold at 1,024 B/s.
pkg/statistics/...basic tests and NextGen race/deadlock tests pass.make checkpasses with statistics-scoped lint and root-module checks,including the repository-wide modernize scan. Generated files are unchanged.
Release note
Summary by CodeRabbit