Skip to content
Merged
1 change: 1 addition & 0 deletions pkg/core/basic_cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ type RegionSetInformer interface {
RandWitnessRegions(storeID uint64, ranges []keyutil.KeyRange) []*RegionInfo
RandPendingRegions(storeID uint64, ranges []keyutil.KeyRange) []*RegionInfo
GetAverageRegionSize() int64
GetNonEmptyAverageRegionSize() int64
GetStoreRegionCount(storeID uint64) int
GetRegion(id uint64) *RegionInfo
GetAdjacentRegions(region *RegionInfo) (*RegionInfo, *RegionInfo)
Expand Down
13 changes: 13 additions & 0 deletions pkg/core/region.go
Original file line number Diff line number Diff line change
Expand Up @@ -2350,6 +2350,19 @@ func (r *RegionsInfo) GetAverageRegionSize() int64 {
return r.tree.TotalSize() / int64(r.tree.length())
}

// GetNonEmptyAverageRegionSize returns the average approximate size of
// non-empty regions only. Empty regions (e.g. freshly split, unwritten
// regions) are excluded so a cluster with many of them doesn't get this
// average diluted toward noise levels.
func (r *RegionsInfo) GetNonEmptyAverageRegionSize() int64 {
r.t.RLock()
defer r.t.RUnlock()
if r.tree.nonEmptyRegionsCnt == 0 {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning 0 when the cluster or selected range contains no non-empty region changes the existing tolerance to zero. This is observable in scatter-range, which intentionally allows 1 MiB empty regions and sets the tolerant ratio to 2: rangeCluster.GetNonEmptyAverageRegionSize() now returns 0, so source gets no tolerance and target only gets the candidate's 1 MiB. The configured ratio is therefore no longer honored and empty-region balancing becomes more aggressive. Please define a fallback, such as retaining GetAverageRegionSize() when the non-empty count is zero, and add an all-empty range regression test.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ec0fec6. GetNonEmptyAverageRegionSize() now falls back to the plain all-regions average (matching GetAverageRegionSize()'s value) when there are no non-empty regions at all, instead of returning 0. Verified: in an all-empty scenario the two methods now return identical values, so scatter-range's configured tolerant-size-ratio is no longer silently zeroed out.

Comment thread
bufferflies marked this conversation as resolved.
Outdated
return 0
}
return r.tree.nonEmptyTotalSize / int64(r.tree.nonEmptyRegionsCnt)
}

// ValidRegion is used to decide if the region is valid.
func (r *RegionsInfo) ValidRegion(region *metapb.Region) error {
startKey := region.GetStartKey()
Expand Down
33 changes: 32 additions & 1 deletion pkg/core/region_tree.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,14 @@ const (
type regionTree struct {
tree *btree.BTreeG[*regionItem]
// Statistics
totalSize int64
totalSize int64
// nonEmptyTotalSize and nonEmptyRegionsCnt mirror totalSize/length but
// exclude empty regions (approximateSize <= EmptyRegionApproximateSize),
// so GetAverageRegionSize can reflect only regions that actually hold

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment names GetAverageRegionSize, but that method still includes empty regions; these counters are consumed by GetNonEmptyAverageRegionSize. The debug log in pkg/schedule/schedulers/utils.go:174 likewise still reports the old average while tolerantResource uses the new one. Please correct the comment and expose the non-empty average, or rename the log field, so diagnostics match the calculation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8203c39. Corrected the comment to name GetNonEmptyAverageRegionSize (the method these counters actually back — GetAverageRegionSize itself was reverted to its original behavior earlier in this PR). Also added a non-empty-average-region-size field to the debug log alongside the existing average-region-size, so both are visible regardless of which one actually fed into tolerantResource for a given schedule kind.

// data instead of being diluted by a large number of freshly-split,
// unwritten regions.
nonEmptyTotalSize int64
nonEmptyRegionsCnt int
totalWriteBytesRate float64
totalWriteKeysRate float64
// count the number of regions that not loaded from storage.
Expand All @@ -75,6 +82,8 @@ func newRegionTree() *regionTree {
return &regionTree{
tree: btree.NewG[*regionItem](defaultBTreeDegree),
totalSize: 0,
nonEmptyTotalSize: 0,
nonEmptyRegionsCnt: 0,
totalWriteBytesRate: 0,
totalWriteKeysRate: 0,
notFromStorageRegionsCnt: 0,
Expand All @@ -85,6 +94,8 @@ func newRegionTreeWithCountRef() *regionTree {
return &regionTree{
tree: btree.NewG[*regionItem](defaultBTreeDegree),
totalSize: 0,
nonEmptyTotalSize: 0,
nonEmptyRegionsCnt: 0,
totalWriteBytesRate: 0,
totalWriteKeysRate: 0,
notFromStorageRegionsCnt: 0,
Expand Down Expand Up @@ -174,6 +185,10 @@ func (t *regionTree) updateRef(origin, region *RegionInfo) {
func (t *regionTree) update(item *regionItem, withOverlaps bool, overlaps ...*RegionInfo) []*RegionInfo {
region := item.RegionInfo
t.totalSize += region.approximateSize
if region.approximateSize > EmptyRegionApproximateSize {
t.nonEmptyTotalSize += region.approximateSize
t.nonEmptyRegionsCnt++
}
regionWriteBytesRate, regionWriteKeysRate := region.GetWriteRate()
t.totalWriteBytesRate += regionWriteBytesRate
t.totalWriteKeysRate += regionWriteKeysRate
Expand Down Expand Up @@ -201,6 +216,10 @@ func (t *regionTree) update(item *regionItem, withOverlaps bool, overlaps ...*Re
logutil.ZapRedactStringer("delete-region", RegionToHexMeta(old.GetMeta())),
logutil.ZapRedactStringer("update-region", RegionToHexMeta(region.GetMeta())))
t.totalSize -= old.approximateSize
if old.approximateSize > EmptyRegionApproximateSize {
t.nonEmptyTotalSize -= old.approximateSize
t.nonEmptyRegionsCnt--
}
regionWriteBytesRate, regionWriteKeysRate = old.GetWriteRate()
t.totalWriteBytesRate -= regionWriteBytesRate
t.totalWriteKeysRate -= regionWriteKeysRate
Expand All @@ -218,11 +237,19 @@ func (t *regionTree) update(item *regionItem, withOverlaps bool, overlaps ...*Re
// updateStat is used to update statistics when RegionInfo is directly replaced.
func (t *regionTree) updateStat(origin *RegionInfo, region *RegionInfo) {
t.totalSize += region.approximateSize
if region.approximateSize > EmptyRegionApproximateSize {
t.nonEmptyTotalSize += region.approximateSize
t.nonEmptyRegionsCnt++
}
regionWriteBytesRate, regionWriteKeysRate := region.GetWriteRate()
t.totalWriteBytesRate += regionWriteBytesRate
t.totalWriteKeysRate += regionWriteKeysRate

t.totalSize -= origin.approximateSize
if origin.approximateSize > EmptyRegionApproximateSize {
t.nonEmptyTotalSize -= origin.approximateSize
t.nonEmptyRegionsCnt--
}
regionWriteBytesRate, regionWriteKeysRate = origin.GetWriteRate()
t.totalWriteBytesRate -= regionWriteBytesRate
t.totalWriteKeysRate -= regionWriteKeysRate
Expand Down Expand Up @@ -252,6 +279,10 @@ func (t *regionTree) remove(region *RegionInfo) {
}

t.totalSize -= result.GetApproximateSize()
if result.GetApproximateSize() > EmptyRegionApproximateSize {
t.nonEmptyTotalSize -= result.GetApproximateSize()
t.nonEmptyRegionsCnt--
}
regionWriteBytesRate, regionWriteKeysRate := result.GetWriteRate()
t.totalWriteBytesRate -= regionWriteBytesRate
t.totalWriteKeysRate -= regionWriteKeysRate
Expand Down
79 changes: 77 additions & 2 deletions pkg/schedule/schedulers/balance_region_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,13 @@ package schedulers
import (
"fmt"
"testing"
"time"

"github.com/docker/go-units"
"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/core/constant"
Expand Down Expand Up @@ -57,7 +60,7 @@ func TestInfluenceAmp(t *testing.T) {

// It will schedule if the diff region count is greater than the sum
// of TolerantSizeRatio and influenceAmp*2.
tc.AddRegionStore(1, int(100+influenceAmp+3))
tc.AddRegionStore(1, int(100+influenceAmp+4))
tc.AddRegionStore(2, int(100-influenceAmp))
tc.AddLeaderRegion(1, 1, 2)
region := tc.GetRegion(1).Clone(core.SetApproximateSize(R))
Expand All @@ -70,13 +73,85 @@ func TestInfluenceAmp(t *testing.T) {

// It will not schedule if the diff region count is greater than the sum
// of TolerantSizeRatio and influenceAmp*2.
tc.AddRegionStore(1, int(100+influenceAmp+2))
tc.AddRegionStore(1, int(100+influenceAmp+3))
solver.Source = tc.GetStore(1)
solver.sourceScore, solver.targetScore = solver.sourceStoreScore(""), solver.targetStoreScore("")
re.False(solver.shouldBalance(""))
re.Less(solver.sourceScore-solver.targetScore, float64(1))
}

// TestSingleRegionOnLargeEmptyDiskDoesNotMigrate verifies that when a store's
// disk is mostly empty except for a single small region (e.g. 6TiB capacity
// with only a 10MiB region), balance-region does NOT move that region to an
// otherwise-identical, entirely empty peer store. Moving it would not fix any
// real imbalance (10MiB vs 6TiB capacity is a negligible utilization
// difference either way) and would just relocate the same "which store holds
// the only real data" state onto a different empty store — inviting the kind
// of pointless churn reported in #11135, since every other empty store looks
// equally attractive as a target on the next scheduling pass.
func TestSingleRegionOnLargeEmptyDiskDoesNotMigrate(t *testing.T) {
cancel, _, tc, oc := prepareSchedulersTest()
defer cancel()
re := require.New(t)

const (
capacity = 6 * units.TiB
regionSizeMB = 10 // MiB, matches core.StoreInfo.GetRegionSize()'s unit
)

mkStore := func(id uint64, usedMB int64) *core.StoreInfo {
usedBytes := uint64(usedMB) * units.MiB
stats := &pdpb.StoreStats{
Capacity: capacity,
UsedSize: usedBytes,
Available: capacity - usedBytes,
}
return core.NewStoreInfo(
&metapb.Store{Id: id, State: metapb.StoreState_Up},
core.SetStoreStats(stats),
core.SetRegionCount(10),
core.SetRegionSize(usedMB),
core.SetLastHeartbeatTS(time.Now()),
)
}

// storeA holds the cluster's only non-empty region; storeB is otherwise
// identical (same capacity, same region count) but has no data at all,
// and does not already hold a peer of region 1 — a real Schedule() run
// would still consider it a legitimate, unfiltered candidate target.
tc.PutStore(mkStore(1, regionSizeMB))
tc.PutStore(mkStore(2, 0))

tc.AddLeaderRegion(1, 1)
region := tc.GetRegion(1).Clone(core.SetApproximateSize(regionSizeMB))
tc.PutRegion(region)

// Mirror the real-world setup that motivated this test: each store already
// holds 10 regions (region *count* is balanced), but region 1 above is the
// only one with any data — the other 19 are freshly-split, empty regions.
var nextID uint64 = 2
for range 9 {
tc.AddLeaderRegion(nextID, 1)
empty := tc.GetRegion(nextID).Clone(core.SetApproximateSize(0))
tc.PutRegion(empty)
nextID++
}
for range 10 {
tc.AddLeaderRegion(nextID, 2)
empty := tc.GetRegion(nextID).Clone(core.SetApproximateSize(0))
tc.PutRegion(empty)
nextID++
}

kind := constant.NewScheduleKind(constant.RegionKind, constant.BySize)
influence := oc.GetOpInfluence(tc.GetBasicCluster())
basePlan := plan.NewBalanceSchedulerPlan()
solver := newSolver(basePlan, kind, tc, influence)
solver.Source, solver.Target, solver.Region = tc.GetStore(1), tc.GetStore(2), tc.GetRegion(1)
solver.sourceScore, solver.targetScore = solver.sourceStoreScore(""), solver.targetStoreScore("")
re.False(solver.shouldBalance(""))
}

func TestShouldBalance(t *testing.T) {
// store size = 100GiB
// region size = 96MiB
Expand Down
5 changes: 5 additions & 0 deletions pkg/schedule/schedulers/range_cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,11 @@ func (r *rangeCluster) GetAverageRegionSize() int64 {
return r.subCluster.GetAverageRegionSize()
}

// GetNonEmptyAverageRegionSize returns the average approximate size of non-empty regions.
func (r *rangeCluster) GetNonEmptyAverageRegionSize() int64 {
return r.subCluster.GetNonEmptyAverageRegionSize()
}

// GetAvgNetworkSlowScore returns the average network slow score.
func (r *rangeCluster) GetAvgNetworkSlowScore(id uint64) uint64 {
return r.subCluster.GetAvgNetworkSlowScore(id)
Expand Down
11 changes: 9 additions & 2 deletions pkg/schedule/schedulers/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,11 @@ func (p *solver) targetStoreScore(scheduleName string) float64 {
targetDelta := influence + tolerantResource
score = p.Target.LeaderScore(p.kind.Policy, targetDelta)
case constant.RegionKind:
targetDelta := influence*influenceAmp + tolerantResource
// account for the candidate region's own size, so a target doesn't
// look artificially light just because this move hasn't landed yet.
// Unlike opInfluence (other, already-pending operators), this is not
// amplified: it is the literal size the target is about to receive.
targetDelta := influence*influenceAmp + tolerantResource + p.Region.GetApproximateSize()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the added 10 MiB/6 TiB regression scenario, GetAverageRegionSize() truncates to zero; the populated source's v2 score is slightly above 10 while an empty target's projected score is exactly 10, so this comparison still schedules the peer. After it lands on any empty store, another empty target recreates the same state, allowing the peer to rotate indefinitely and leaving the reported churn unresolved.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8532b76. getTolerantResource() now uses a new GetNonEmptyAverageRegionSize() (excludes empty regions from the average instead of GetAverageRegionSize()), so it no longer collapses to zero in this scenario. Re-verified the exact case you described: tolerantResource computes to the candidate region's own size instead of truncating to zero, and the target's projected score now exceeds the source's, so shouldBalance() returns false — the region stays put instead of rotating. TestSingleRegionOnLargeEmptyDiskDoesNotMigrate (renamed/inverted from the original test) asserts this directly.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tolerantResource already contributes one average region to the target margin. Adding the candidate again rejects legitimate moves when the candidate equals the average: with three 96 MiB regions on the source and an empty target (v1, ratio=1), this head compares 192 vs 192 and schedules nothing, although moving one region leaves 192 vs 96. The base head schedules this move, so this introduces a balance-region regression for ordinary equal-sized regions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in dafffd2. You're right that tolerantResource already represents about one region's worth of margin, so adding the candidate's size on top double-counted it. Switched to max(tolerantResource, p.Region.GetApproximateSize()) instead of summing them — this falls back to the candidate's real size only when it exceeds the average-based margin, matching the (previously unimplemented) intent in shouldBalance()'s own comment about max(regionSize, averageRegionSize). Re-ran your exact reproduction (three 96MiB regions on the source, empty target, v1, ratio=1): now scores 192 vs 96 and schedules the move, matching pre-PR/base behavior. Added TestBalanceRegionOrdinaryMoveNotBlockedByCandidateSize as a permanent regression test for this case, since no existing test previously covered balancing between several ordinary, similarly-sized regions.

score = p.Target.RegionScore(p.GetSchedulerConfig().GetRegionScoreFormulaVersion(), p.GetSchedulerConfig().GetHighSpaceRatio(), p.GetSchedulerConfig().GetLowSpaceRatio(), targetDelta)
case constant.WitnessKind:
targetDelta := influence + tolerantResource
Expand Down Expand Up @@ -178,7 +182,10 @@ func (p *solver) getTolerantResource() int64 {
if (p.kind.Resource == constant.LeaderKind || p.kind.Resource == constant.WitnessKind) && p.kind.Policy == constant.ByCount {
p.tolerantSource = int64(p.tolerantSizeRatio)
} else {
regionSize := p.GetAverageRegionSize()
// Use the non-empty average so a cluster full of freshly-split,
// unwritten regions doesn't collapse the tolerant margin toward
// noise levels.
regionSize := p.GetNonEmptyAverageRegionSize()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch is shared by all non-count schedule kinds, not just balance-region. In particular, balance-leader with BySize also reaches this call, so a cluster with one large data region and many empty regions will use that large region as the leader tolerance and can suppress legitimate leader transfers. Please either limit this behavior to RegionKind, or document the intended leader behavior and add a regression test.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ec0fec6. Scoped GetNonEmptyAverageRegionSize() to RegionKind specifically in getTolerantResource(); LeaderKind/WitnessKind now unconditionally fall through to the original GetAverageRegionSize() regardless of policy, so leader-schedule-policy=size is unaffected by this PR. Re-ran TestBalanceLeader*/TestShouldBalance to confirm no behavior change on that path.

p.tolerantSource = int64(float64(regionSize) * p.tolerantSizeRatio)
Comment thread
bufferflies marked this conversation as resolved.
Outdated
}
return p.tolerantSource
Expand Down
Loading