From b0207a6a234adda38ef67c26e5382b6b2353fbc6 Mon Sep 17 00:00:00 2001 From: Yongbo Jiang Date: Tue, 26 Sep 2023 11:36:15 +0800 Subject: [PATCH] =?UTF-8?q?Revert=20"statistics:=20fix=20empty=20region=20?= =?UTF-8?q?count=20when=20resuming=20(#7009)=20(#70=E2=80=A6=20(#7150)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ref tikv/pd#7148 Signed-off-by: Cabinfever_B --- server/api/stats_test.go | 4 +-- server/cluster/cluster.go | 4 +-- server/core/region.go | 26 +++++++++-------- server/core/region_test.go | 31 +++++++++++++++++++-- server/statistics/region.go | 6 ++-- server/statistics/region_collection.go | 4 +-- server/statistics/region_collection_test.go | 5 ++-- tests/pdctl/scheduler/scheduler_test.go | 4 +-- 8 files changed, 53 insertions(+), 31 deletions(-) diff --git a/server/api/stats_test.go b/server/api/stats_test.go index 19a27ab0c53..bf92634ae58 100644 --- a/server/api/stats_test.go +++ b/server/api/stats_test.go @@ -137,7 +137,7 @@ func (suite *statsTestSuite) TestRegionStats() { statsAll := &statistics.RegionStats{ Count: 4, EmptyCount: 1, - StorageSize: 350, + StorageSize: 351, StorageKeys: 221, StoreLeaderCount: map[uint64]int{1: 1, 4: 2, 5: 1}, StorePeerCount: map[uint64]int{1: 3, 2: 1, 3: 1, 4: 2, 5: 2}, @@ -150,7 +150,7 @@ func (suite *statsTestSuite) TestRegionStats() { stats23 := &statistics.RegionStats{ Count: 2, EmptyCount: 1, - StorageSize: 200, + StorageSize: 201, StorageKeys: 151, StoreLeaderCount: map[uint64]int{4: 1, 5: 1}, StorePeerCount: map[uint64]int{1: 2, 4: 1, 5: 2}, diff --git a/server/cluster/cluster.go b/server/cluster/cluster.go index 631dd85129d..688bbcc4941 100644 --- a/server/cluster/cluster.go +++ b/server/cluster/cluster.go @@ -843,9 +843,7 @@ func (c *RaftCluster) processRegionHeartbeat(region *core.RegionInfo) error { if err != nil { return err } - if c.GetStoreConfig().IsEnableRegionBucket() { - region.InheritBuckets(origin) - } + region.Inherit(origin, c.storeConfigManager.GetStoreConfig().IsEnableRegionBucket()) c.hotStat.CheckWriteAsync(statistics.NewCheckExpiredItemTask(region)) c.hotStat.CheckReadAsync(statistics.NewCheckExpiredItemTask(region)) diff --git a/server/core/region.go b/server/core/region.go index 7a1fb2c87c2..07b522e47ca 100644 --- a/server/core/region.go +++ b/server/core/region.go @@ -144,9 +144,8 @@ 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 } @@ -189,9 +188,19 @@ func RegionFromHeartbeat(heartbeat *pdpb.RegionHeartbeatRequest, opts ...RegionC return region } -// InheritBuckets inherits the buckets from the parent region if bucket enabled. -func (r *RegionInfo) InheritBuckets(origin *RegionInfo) { - if origin != nil && r.buckets == nil { +// 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 { r.buckets = origin.buckets } } @@ -469,13 +478,6 @@ 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 -} - // GetApproximateKeys returns the approximate keys of the region. func (r *RegionInfo) GetApproximateKeys() int64 { return r.approximateKeys diff --git a/server/core/region_test.go b/server/core/region_test.go index 8f63125e5d6..d6030997fa3 100644 --- a/server/core/region_test.go +++ b/server/core/region_test.go @@ -186,9 +186,35 @@ func TestSortedEqual(t *testing.T) { } } -func TestInheritBuckets(t *testing.T) { +func TestInherit(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 @@ -201,11 +227,12 @@ func TestInheritBuckets(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.InheritBuckets(origin) + r.Inherit(origin, true) 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/server/statistics/region.go b/server/statistics/region.go index 60e2f19935d..2822bb64ead 100644 --- a/server/statistics/region.go +++ b/server/statistics/region.go @@ -57,12 +57,10 @@ func (s *RegionStats) Observe(r *core.RegionInfo) { s.Count++ approximateKeys := r.GetApproximateKeys() approximateSize := r.GetApproximateSize() - if approximateSize == core.EmptyRegionApproximateSize { + if approximateSize <= core.EmptyRegionApproximateSize { s.EmptyCount++ } - if !r.IsEmptyRegion() { - s.StorageSize += approximateSize - } + s.StorageSize += approximateSize s.StorageKeys += approximateKeys leader := r.GetLeader() if leader != nil { diff --git a/server/statistics/region_collection.go b/server/statistics/region_collection.go index 56d5516c8b7..ab5e6b22d9c 100644 --- a/server/statistics/region_collection.go +++ b/server/statistics/region_collection.go @@ -198,7 +198,7 @@ func (r *RegionStatistics) Observe(region *core.RegionInfo, stores []*core.Store DownPeer: len(region.GetDownPeers()) > 0, PendingPeer: len(region.GetPendingPeers()) > 0, LearnerPeer: len(region.GetLearners()) > 0, - EmptyRegion: region.IsEmptyRegion(), + EmptyRegion: region.GetApproximateSize() <= core.EmptyRegionApproximateSize, OversizedRegion: region.IsOversized( int64(r.storeConfigManager.GetStoreConfig().GetRegionMaxSize()), int64(r.storeConfigManager.GetStoreConfig().GetRegionMaxKeys()), @@ -206,7 +206,7 @@ func (r *RegionStatistics) Observe(region *core.RegionInfo, stores []*core.Store UndersizedRegion: region.NeedMerge( int64(r.opt.GetMaxMergeRegionSize()), int64(r.opt.GetMaxMergeRegionKeys()), - ) && region.GetApproximateSize() >= core.EmptyRegionApproximateSize, + ), } for typ, c := range conditions { diff --git a/server/statistics/region_collection_test.go b/server/statistics/region_collection_test.go index 179d78b539f..7c686f1c9ce 100644 --- a/server/statistics/region_collection_test.go +++ b/server/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], core.SetApproximateSize(1)) + region1 := core.NewRegionInfo(r1, peers[0]) region2 := core.NewRegionInfo(r2, peers[0]) regionStats := NewRegionStatistics(opt, manager, nil) regionStats.Observe(region1, stores) @@ -103,8 +103,7 @@ 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], 0) - re.Len(regionStats.stats[EmptyRegion], 0) + re.Len(regionStats.stats[UndersizedRegion], 1) re.Len(regionStats.offlineStats[ExtraPeer], 1) re.Empty(regionStats.offlineStats[MissPeer]) re.Len(regionStats.offlineStats[DownPeer], 1) diff --git a/tests/pdctl/scheduler/scheduler_test.go b/tests/pdctl/scheduler/scheduler_test.go index c904e80940b..1fc69c89e79 100644 --- a/tests/pdctl/scheduler/scheduler_test.go +++ b/tests/pdctl/scheduler/scheduler_test.go @@ -24,7 +24,6 @@ import ( "github.com/stretchr/testify/require" "github.com/tikv/pd/pkg/testutil" "github.com/tikv/pd/server/config" - "github.com/tikv/pd/server/core" "github.com/tikv/pd/server/versioninfo" "github.com/tikv/pd/tests" "github.com/tikv/pd/tests/pdctl" @@ -129,8 +128,7 @@ func TestScheduler(t *testing.T) { pdctl.MustPutStore(re, leaderServer.GetServer(), store) } - // note: because pdqsort is a unstable sort algorithm, set ApproximateSize for this region. - pdctl.MustPutRegion(re, cluster, 1, 1, []byte("a"), []byte("b"), core.SetApproximateSize(10)) + pdctl.MustPutRegion(re, cluster, 1, 1, []byte("a"), []byte("b")) time.Sleep(3 * time.Second) // scheduler show command