From 74ead5cbdf17df955ad7075810be3a724033ca0f Mon Sep 17 00:00:00 2001 From: Yongbo Jiang Date: Thu, 7 Sep 2023 16:48:44 +0800 Subject: [PATCH] statistics: fix empty region count when resuming (#7009) close tikv/pd#7008 Signed-off-by: Cabinfever_B Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com> --- pkg/core/region.go | 26 +++++++++----------- pkg/core/region_test.go | 31 ++---------------------- pkg/statistics/region.go | 6 +++-- pkg/statistics/region_collection.go | 4 +-- pkg/statistics/region_collection_test.go | 5 ++-- server/api/stats_test.go | 4 +-- server/cluster/cluster.go | 4 ++- 7 files changed, 28 insertions(+), 52 deletions(-) diff --git a/pkg/core/region.go b/pkg/core/region.go index bc1464a71b3..57ba5cb3db0 100644 --- a/pkg/core/region.go +++ b/pkg/core/region.go @@ -147,8 +147,9 @@ const ( func RegionFromHeartbeat(heartbeat *pdpb.RegionHeartbeatRequest, opts ...RegionCreateOption) *RegionInfo { // Convert unit to MB. // If region isn't empty and less than 1MB, use 1MB instead. - // The size of empty region will be correct by the previous RegionInfo. regionSize := heartbeat.GetApproximateSize() / units.MiB + // Due to https://github.com/tikv/tikv/pull/11170, if region size is not initialized, + // approximate size will be zero, and region size is zero not EmptyRegionApproximateSize if heartbeat.GetApproximateSize() > 0 && regionSize < EmptyRegionApproximateSize { regionSize = EmptyRegionApproximateSize } @@ -193,19 +194,9 @@ func RegionFromHeartbeat(heartbeat *pdpb.RegionHeartbeatRequest, opts ...RegionC return region } -// Inherit inherits the buckets and region size from the parent region if bucket enabled. -// correct approximate size and buckets by the previous size if here exists a reported RegionInfo. -// See https://github.com/tikv/tikv/issues/11114 -func (r *RegionInfo) Inherit(origin *RegionInfo, bucketEnable bool) { - // regionSize should not be zero if region is not empty. - if r.GetApproximateSize() == 0 { - if origin != nil { - r.approximateSize = origin.approximateSize - } else { - r.approximateSize = EmptyRegionApproximateSize - } - } - if bucketEnable && origin != nil && r.buckets == nil { +// InheritBuckets inherits the buckets from the parent region if bucket enabled. +func (r *RegionInfo) InheritBuckets(origin *RegionInfo) { + if origin != nil && r.buckets == nil { r.buckets = origin.buckets } } @@ -515,6 +506,13 @@ func (r *RegionInfo) GetApproximateSize() int64 { return r.approximateSize } +// IsEmptyRegion returns whether the region is empty. +func (r *RegionInfo) IsEmptyRegion() bool { + // When cluster resumes, the region size may be not initialized, but region heartbeat is send. + // So use `==` here. + return r.approximateSize == EmptyRegionApproximateSize +} + // GetStorePeerApproximateKeys returns the approximate keys of the peer on the specified store. func (r *RegionInfo) GetStorePeerApproximateKeys(storeID uint64) int64 { peer := r.GetStorePeer(storeID) diff --git a/pkg/core/region_test.go b/pkg/core/region_test.go index 5588d9190ec..1e6b43fbf96 100644 --- a/pkg/core/region_test.go +++ b/pkg/core/region_test.go @@ -186,35 +186,9 @@ func TestSortedEqual(t *testing.T) { } } -func TestInherit(t *testing.T) { +func TestInheritBuckets(t *testing.T) { re := require.New(t) - // size in MB - // case for approximateSize - testCases := []struct { - originExists bool - originSize uint64 - size uint64 - expect uint64 - }{ - {false, 0, 0, 1}, - {false, 0, 2, 2}, - {true, 0, 2, 2}, - {true, 1, 2, 2}, - {true, 2, 0, 2}, - } - for _, testCase := range testCases { - var origin *RegionInfo - if testCase.originExists { - origin = NewRegionInfo(&metapb.Region{Id: 100}, nil) - origin.approximateSize = int64(testCase.originSize) - } - r := NewRegionInfo(&metapb.Region{Id: 100}, nil) - r.approximateSize = int64(testCase.size) - r.Inherit(origin, false) - re.Equal(int64(testCase.expect), r.approximateSize) - } - // bucket data := []struct { originBuckets *metapb.Buckets buckets *metapb.Buckets @@ -227,12 +201,11 @@ func TestInherit(t *testing.T) { for _, d := range data { origin := NewRegionInfo(&metapb.Region{Id: 100}, nil, SetBuckets(d.originBuckets)) r := NewRegionInfo(&metapb.Region{Id: 100}, nil) - r.Inherit(origin, true) + r.InheritBuckets(origin) re.Equal(d.originBuckets, r.GetBuckets()) // region will not inherit bucket keys. if origin.GetBuckets() != nil { newRegion := NewRegionInfo(&metapb.Region{Id: 100}, nil) - newRegion.Inherit(origin, false) re.NotEqual(d.originBuckets, newRegion.GetBuckets()) } } diff --git a/pkg/statistics/region.go b/pkg/statistics/region.go index f39c58ed81c..f78c8c6add3 100644 --- a/pkg/statistics/region.go +++ b/pkg/statistics/region.go @@ -59,10 +59,12 @@ func (s *RegionStats) Observe(r *core.RegionInfo) { approximateKeys := r.GetApproximateKeys() approximateSize := r.GetApproximateSize() approximateKvSize := r.GetApproximateKvSize() - if approximateSize <= core.EmptyRegionApproximateSize { + if approximateSize == core.EmptyRegionApproximateSize { s.EmptyCount++ } - s.StorageSize += approximateSize + if !r.IsEmptyRegion() { + s.StorageSize += approximateSize + } s.UserStorageSize += approximateKvSize s.StorageKeys += approximateKeys leader := r.GetLeader() diff --git a/pkg/statistics/region_collection.go b/pkg/statistics/region_collection.go index 6d3ac3f1760..c24a1581d26 100644 --- a/pkg/statistics/region_collection.go +++ b/pkg/statistics/region_collection.go @@ -198,7 +198,7 @@ func (r *RegionStatistics) Observe(region *core.RegionInfo, stores []*core.Store return false }(), LearnerPeer: len(region.GetLearners()) > 0, - EmptyRegion: region.GetApproximateSize() <= core.EmptyRegionApproximateSize, + EmptyRegion: region.IsEmptyRegion(), OversizedRegion: region.IsOversized( int64(r.conf.GetRegionMaxSize()), int64(r.conf.GetRegionMaxKeys()), @@ -206,7 +206,7 @@ func (r *RegionStatistics) Observe(region *core.RegionInfo, stores []*core.Store UndersizedRegion: region.NeedMerge( int64(r.conf.GetMaxMergeRegionSize()), int64(r.conf.GetMaxMergeRegionKeys()), - ), + ) && region.GetApproximateSize() >= core.EmptyRegionApproximateSize, WitnessLeader: region.GetLeader().GetIsWitness(), } // Check if the region meets any of the conditions and update the corresponding info. diff --git a/pkg/statistics/region_collection_test.go b/pkg/statistics/region_collection_test.go index 78cbd840934..232fb8b73d8 100644 --- a/pkg/statistics/region_collection_test.go +++ b/pkg/statistics/region_collection_test.go @@ -63,7 +63,7 @@ func TestRegionStatistics(t *testing.T) { stores[3] = store3 r1 := &metapb.Region{Id: 1, Peers: peers, StartKey: []byte("aa"), EndKey: []byte("bb")} r2 := &metapb.Region{Id: 2, Peers: peers[0:2], StartKey: []byte("cc"), EndKey: []byte("dd")} - region1 := core.NewRegionInfo(r1, peers[0]) + region1 := core.NewRegionInfo(r1, peers[0], core.SetApproximateSize(1)) region2 := core.NewRegionInfo(r2, peers[0]) regionStats := NewRegionStatistics(nil, opt, manager) regionStats.Observe(region1, stores) @@ -97,7 +97,8 @@ func TestRegionStatistics(t *testing.T) { re.Len(regionStats.stats[PendingPeer], 1) re.Len(regionStats.stats[LearnerPeer], 1) re.Len(regionStats.stats[OversizedRegion], 1) - re.Len(regionStats.stats[UndersizedRegion], 1) + re.Len(regionStats.stats[UndersizedRegion], 0) + re.Len(regionStats.stats[EmptyRegion], 0) re.Len(regionStats.stats[OfflinePeer], 1) region1 = region1.Clone(core.WithRemoveStorePeer(7)) diff --git a/server/api/stats_test.go b/server/api/stats_test.go index 4003f53ed9e..7281f8ec985 100644 --- a/server/api/stats_test.go +++ b/server/api/stats_test.go @@ -142,7 +142,7 @@ func (suite *statsTestSuite) TestRegionStats() { statsAll := &statistics.RegionStats{ Count: 4, EmptyCount: 1, - StorageSize: 351, + StorageSize: 350, UserStorageSize: 291, StorageKeys: 221, StoreLeaderCount: map[uint64]int{1: 1, 4: 2, 5: 1}, @@ -156,7 +156,7 @@ func (suite *statsTestSuite) TestRegionStats() { stats23 := &statistics.RegionStats{ Count: 2, EmptyCount: 1, - StorageSize: 201, + StorageSize: 200, UserStorageSize: 181, StorageKeys: 151, StoreLeaderCount: map[uint64]int{4: 1, 5: 1}, diff --git a/server/cluster/cluster.go b/server/cluster/cluster.go index d48d9df7afd..b16de73f84e 100644 --- a/server/cluster/cluster.go +++ b/server/cluster/cluster.go @@ -1090,7 +1090,9 @@ func (c *RaftCluster) processRegionHeartbeat(region *core.RegionInfo) error { if err != nil { return err } - region.Inherit(origin, c.GetStoreConfig().IsEnableRegionBucket()) + if c.GetStoreConfig().IsEnableRegionBucket() { + region.InheritBuckets(origin) + } if !c.isAPIServiceMode { c.hotStat.CheckWriteAsync(statistics.NewCheckExpiredItemTask(region))