From 94561888a78ea907ce68cc06f66a29fde4cdb85a Mon Sep 17 00:00:00 2001 From: Boyang Lyu Date: Wed, 23 Oct 2024 16:20:54 +0800 Subject: [PATCH 1/8] check if the query startkey and endkey specify an uncovered region Signed-off-by: Boyang Lyu --- pkg/core/region.go | 23 +++++++++++ server/api/stats_test.go | 88 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+) diff --git a/pkg/core/region.go b/pkg/core/region.go index 53268589c8a..41451b98978 100644 --- a/pkg/core/region.go +++ b/pkg/core/region.go @@ -1789,6 +1789,29 @@ func (r *RegionsInfo) GetRegionCount(startKey, endKey []byte) int { defer r.t.RUnlock() start := ®ionItem{&RegionInfo{meta: &metapb.Region{StartKey: startKey}}} end := ®ionItem{&RegionInfo{meta: &metapb.Region{StartKey: endKey}}} + + // check if both keys are in the uncovered ranges. + startItemt, sit := r.tree.tree.GetWithIndex(start) + endItemt, eit := r.tree.tree.GetWithIndex(end) + if startItemt == nil { + startItemt = r.tree.tree.GetAt(sit - 1) + } + if endItemt == nil { + endItemt = r.tree.tree.GetAt(eit - 1) + } + + startInAnInterval := false + if startItemt != nil { + startInAnInterval = (bytes.Compare(startItemt.GetStartKey(), startKey) <= 0) && (bytes.Compare(startKey, startItemt.GetEndKey()) <= 0) + } + endInAnInterval := false + if endItemt != nil { + endInAnInterval = (bytes.Compare(endItemt.GetStartKey(), endKey) <= 0) && (bytes.Compare(endKey, endItemt.GetEndKey()) <= 0) + } + if sit == eit && (!startInAnInterval) && (!endInAnInterval) { + return 0 + } + // it returns 0 if startKey is nil. _, startIndex := r.tree.tree.GetWithIndex(start) var endIndex int diff --git a/server/api/stats_test.go b/server/api/stats_test.go index 1485f9eb5af..d13d3b9db68 100644 --- a/server/api/stats_test.go +++ b/server/api/stats_test.go @@ -204,3 +204,91 @@ func (suite *statsTestSuite) TestRegionStats() { } } } + +func (suite *statsTestSuite) TestRegionStatsHoles() { + statsURL := suite.urlPrefix + "/stats/region" + epoch := &metapb.RegionEpoch{ + ConfVer: 1, + Version: 1, + } + + re := suite.Require() + + // range holes + regions_withholes := []*core.RegionInfo{ + core.NewRegionInfo( + &metapb.Region{ + Id: 1, + StartKey: []byte(""), + EndKey: []byte("c"), + Peers: []*metapb.Peer{ + {Id: 101, StoreId: 1}, + {Id: 102, StoreId: 2}, + {Id: 103, StoreId: 3}, + }, + RegionEpoch: epoch, + }, + &metapb.Peer{Id: 101, StoreId: 1}, + core.SetApproximateSize(100), + core.SetApproximateKvSize(80), + core.SetApproximateKeys(50), + ), + core.NewRegionInfo( + &metapb.Region{ + Id: 2, + StartKey: []byte("h"), + EndKey: []byte("x"), + Peers: []*metapb.Peer{ + {Id: 104, StoreId: 1}, + {Id: 105, StoreId: 4}, + {Id: 106, StoreId: 5}, + }, + RegionEpoch: epoch, + }, + &metapb.Peer{Id: 105, StoreId: 4}, + core.SetApproximateSize(200), + core.SetApproximateKvSize(180), + core.SetApproximateKeys(150), + ), + core.NewRegionInfo( + &metapb.Region{ + Id: 3, + StartKey: []byte("z"), + EndKey: []byte(""), + Peers: []*metapb.Peer{ + {Id: 106, StoreId: 1}, + {Id: 107, StoreId: 5}, + }, + RegionEpoch: epoch, + }, + &metapb.Peer{Id: 107, StoreId: 5}, + core.SetApproximateSize(1), + core.SetApproximateKvSize(1), + core.SetApproximateKeys(1), + ), + } + + for _, r := range regions_withholes { + mustRegionHeartbeat(re, suite.svr, r) + } + + // holes in between : + // | - c| ... |h - x| ... |z - | + + startKeys := [4]string{"d", "b", "b", "i"} + endKeys := [4]string{"e", "e", "i", "j"} + ans := [4]int{0, 1, 2, 1} + + for i := 0; i < 4; i++ { + startKey := url.QueryEscape(startKeys[i]) + endKey := url.QueryEscape(endKeys[i]) + args_withholes := fmt.Sprintf("?start_key=%s&end_key=%s&count", startKey, endKey) + res, err := testDialClient.Get(statsURL + args_withholes) + re.NoError(err) + stats := &statistics.RegionStats{} + err = apiutil.ReadJSON(res.Body, stats) + res.Body.Close() + re.NoError(err) + re.Equal(ans[i], stats.Count) + } +} From 54de08118dee4bd3bef5843d50eff9bf77d7cd56 Mon Sep 17 00:00:00 2001 From: Boyang Lyu Date: Wed, 23 Oct 2024 17:29:34 +0800 Subject: [PATCH 2/8] reduce duplicate code Signed-off-by: Boyang Lyu --- pkg/core/region.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/pkg/core/region.go b/pkg/core/region.go index 41451b98978..6b8a244430d 100644 --- a/pkg/core/region.go +++ b/pkg/core/region.go @@ -1791,10 +1791,10 @@ func (r *RegionsInfo) GetRegionCount(startKey, endKey []byte) int { end := ®ionItem{&RegionInfo{meta: &metapb.Region{StartKey: endKey}}} // check if both keys are in the uncovered ranges. - startItemt, sit := r.tree.tree.GetWithIndex(start) + startItemt, startIndex := r.tree.tree.GetWithIndex(start) endItemt, eit := r.tree.tree.GetWithIndex(end) if startItemt == nil { - startItemt = r.tree.tree.GetAt(sit - 1) + startItemt = r.tree.tree.GetAt(startIndex - 1) } if endItemt == nil { endItemt = r.tree.tree.GetAt(eit - 1) @@ -1808,12 +1808,10 @@ func (r *RegionsInfo) GetRegionCount(startKey, endKey []byte) int { if endItemt != nil { endInAnInterval = (bytes.Compare(endItemt.GetStartKey(), endKey) <= 0) && (bytes.Compare(endKey, endItemt.GetEndKey()) <= 0) } - if sit == eit && (!startInAnInterval) && (!endInAnInterval) { + if startIndex == eit && (!startInAnInterval) && (!endInAnInterval) { return 0 } - // it returns 0 if startKey is nil. - _, startIndex := r.tree.tree.GetWithIndex(start) var endIndex int var item *regionItem // it should return the length of the tree if endKey is nil. From 0a5763375e54daf10f2f9ce164c1c5ab43b848ac Mon Sep 17 00:00:00 2001 From: Boyang Lyu Date: Wed, 23 Oct 2024 18:01:47 +0800 Subject: [PATCH 3/8] fixed testcases Signed-off-by: Boyang Lyu --- pkg/core/region.go | 20 ++++++++++---------- server/api/stats_test.go | 27 ++++++++++++++++++--------- 2 files changed, 28 insertions(+), 19 deletions(-) diff --git a/pkg/core/region.go b/pkg/core/region.go index 6b8a244430d..60998e90d8a 100644 --- a/pkg/core/region.go +++ b/pkg/core/region.go @@ -1791,22 +1791,22 @@ func (r *RegionsInfo) GetRegionCount(startKey, endKey []byte) int { end := ®ionItem{&RegionInfo{meta: &metapb.Region{StartKey: endKey}}} // check if both keys are in the uncovered ranges. - startItemt, startIndex := r.tree.tree.GetWithIndex(start) - endItemt, eit := r.tree.tree.GetWithIndex(end) - if startItemt == nil { - startItemt = r.tree.tree.GetAt(startIndex - 1) + startItem, startIndex := r.tree.tree.GetWithIndex(start) + endItem, eit := r.tree.tree.GetWithIndex(end) + if startItem == nil { + startItem = r.tree.tree.GetAt(startIndex - 1) } - if endItemt == nil { - endItemt = r.tree.tree.GetAt(eit - 1) + if endItem == nil { + endItem = r.tree.tree.GetAt(eit - 1) } startInAnInterval := false - if startItemt != nil { - startInAnInterval = (bytes.Compare(startItemt.GetStartKey(), startKey) <= 0) && (bytes.Compare(startKey, startItemt.GetEndKey()) <= 0) + if startItem != nil { + startInAnInterval = (bytes.Compare(startItem.GetStartKey(), startKey) <= 0) && (bytes.Compare(startKey, startItem.GetEndKey()) <= 0) } endInAnInterval := false - if endItemt != nil { - endInAnInterval = (bytes.Compare(endItemt.GetStartKey(), endKey) <= 0) && (bytes.Compare(endKey, endItemt.GetEndKey()) <= 0) + if endItem != nil { + endInAnInterval = (bytes.Compare(endItem.GetStartKey(), endKey) <= 0) && (bytes.Compare(endKey, endItem.GetEndKey()) <= 0) } if startIndex == eit && (!startInAnInterval) && (!endInAnInterval) { return 0 diff --git a/server/api/stats_test.go b/server/api/stats_test.go index d13d3b9db68..e60883b8334 100644 --- a/server/api/stats_test.go +++ b/server/api/stats_test.go @@ -20,6 +20,7 @@ import ( "testing" "github.com/pingcap/kvproto/pkg/metapb" + "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" "github.com/tikv/pd/pkg/core" "github.com/tikv/pd/pkg/statistics" @@ -205,17 +206,25 @@ func (suite *statsTestSuite) TestRegionStats() { } } -func (suite *statsTestSuite) TestRegionStatsHoles() { - statsURL := suite.urlPrefix + "/stats/region" +func TestRegionStatsHoles(t *testing.T) { + re := require.New(t) + svr, cleanup := mustNewServer(re) + defer cleanup() + server.MustWaitLeader(re, []*server.Server{svr}) + + addr := svr.GetAddr() + urlPrefix := fmt.Sprintf("%s%s/api/v1", addr, apiPrefix) + + mustBootstrapCluster(re, svr) + + statsURL := urlPrefix + "/stats/region" epoch := &metapb.RegionEpoch{ ConfVer: 1, Version: 1, } - re := suite.Require() - // range holes - regions_withholes := []*core.RegionInfo{ + regionsWithholes := []*core.RegionInfo{ core.NewRegionInfo( &metapb.Region{ Id: 1, @@ -268,8 +277,8 @@ func (suite *statsTestSuite) TestRegionStatsHoles() { ), } - for _, r := range regions_withholes { - mustRegionHeartbeat(re, suite.svr, r) + for _, r := range regionsWithholes { + mustRegionHeartbeat(re, svr, r) } // holes in between : @@ -282,8 +291,8 @@ func (suite *statsTestSuite) TestRegionStatsHoles() { for i := 0; i < 4; i++ { startKey := url.QueryEscape(startKeys[i]) endKey := url.QueryEscape(endKeys[i]) - args_withholes := fmt.Sprintf("?start_key=%s&end_key=%s&count", startKey, endKey) - res, err := testDialClient.Get(statsURL + args_withholes) + argsWithHoles := fmt.Sprintf("?start_key=%s&end_key=%s&count", startKey, endKey) + res, err := testDialClient.Get(statsURL + argsWithHoles) re.NoError(err) stats := &statistics.RegionStats{} err = apiutil.ReadJSON(res.Body, stats) From 6027b94b52cd32c271cbedf3eca77451f02112ae Mon Sep 17 00:00:00 2001 From: Boyang Lyu Date: Mon, 28 Oct 2024 13:48:24 +0800 Subject: [PATCH 4/8] fixed the endkey-being-empty bug Signed-off-by: Boyang Lyu --- pkg/core/region.go | 16 ++++++++++++++-- server/api/stats_test.go | 20 ++++++++++---------- 2 files changed, 24 insertions(+), 12 deletions(-) mode change 100644 => 100755 pkg/core/region.go diff --git a/pkg/core/region.go b/pkg/core/region.go old mode 100644 new mode 100755 index 60998e90d8a..823bf4e45c2 --- a/pkg/core/region.go +++ b/pkg/core/region.go @@ -1249,11 +1249,11 @@ func (r *RegionsInfo) setRegionLocked(region *RegionInfo, withOverlaps bool, ol // UpdateSubTree updates the subtree. func (r *RegionsInfo) UpdateSubTree(region, origin *RegionInfo, overlaps []*RegionInfo, rangeChanged bool) { - failpoint.Inject("UpdateSubTree", func() { + if _, _err_ := failpoint.Eval(_curpkg_("UpdateSubTree")); _err_ == nil { if origin == nil { time.Sleep(time.Second) } - }) + } r.st.Lock() defer r.st.Unlock() if origin != nil { @@ -1795,9 +1795,13 @@ func (r *RegionsInfo) GetRegionCount(startKey, endKey []byte) int { endItem, eit := r.tree.tree.GetWithIndex(end) if startItem == nil { startItem = r.tree.tree.GetAt(startIndex - 1) + } else { + startIndex += 1 } if endItem == nil { endItem = r.tree.tree.GetAt(eit - 1) + } else { + eit += 1 } startInAnInterval := false @@ -1808,6 +1812,13 @@ func (r *RegionsInfo) GetRegionCount(startKey, endKey []byte) int { if endItem != nil { endInAnInterval = (bytes.Compare(endItem.GetStartKey(), endKey) <= 0) && (bytes.Compare(endKey, endItem.GetEndKey()) <= 0) } + + if len(endKey) == 0 { + endItem = r.tree.tree.GetAt(r.tree.tree.Len() - 1) + eit = r.tree.tree.Len() + endInAnInterval = (bytes.Compare(endItem.GetEndKey(), endKey) <= 0) && (bytes.Compare(endKey, endItem.GetEndKey()) <= 0) + } + if startIndex == eit && (!startInAnInterval) && (!endInAnInterval) { return 0 } @@ -1824,6 +1835,7 @@ func (r *RegionsInfo) GetRegionCount(startKey, endKey []byte) int { endIndex-- } } + return endIndex - startIndex + 1 } diff --git a/server/api/stats_test.go b/server/api/stats_test.go index e60883b8334..83a776a1883 100644 --- a/server/api/stats_test.go +++ b/server/api/stats_test.go @@ -228,7 +228,7 @@ func TestRegionStatsHoles(t *testing.T) { core.NewRegionInfo( &metapb.Region{ Id: 1, - StartKey: []byte(""), + StartKey: []byte("-"), EndKey: []byte("c"), Peers: []*metapb.Peer{ {Id: 101, StoreId: 1}, @@ -245,8 +245,8 @@ func TestRegionStatsHoles(t *testing.T) { core.NewRegionInfo( &metapb.Region{ Id: 2, - StartKey: []byte("h"), - EndKey: []byte("x"), + StartKey: []byte("g"), + EndKey: []byte("l"), Peers: []*metapb.Peer{ {Id: 104, StoreId: 1}, {Id: 105, StoreId: 4}, @@ -262,8 +262,8 @@ func TestRegionStatsHoles(t *testing.T) { core.NewRegionInfo( &metapb.Region{ Id: 3, - StartKey: []byte("z"), - EndKey: []byte(""), + StartKey: []byte("o"), + EndKey: []byte("u"), Peers: []*metapb.Peer{ {Id: 106, StoreId: 1}, {Id: 107, StoreId: 5}, @@ -282,13 +282,13 @@ func TestRegionStatsHoles(t *testing.T) { } // holes in between : - // | - c| ... |h - x| ... |z - | + // | - c| ... |g - l| ... |o - u| ... - startKeys := [4]string{"d", "b", "b", "i"} - endKeys := [4]string{"e", "e", "i", "j"} - ans := [4]int{0, 1, 2, 1} + startKeys := [6]string{"d", "b", "b", "i", "", "v"} + endKeys := [6]string{"e", "e", "i", "j", "", ""} + ans := [6]int{0, 1, 2, 1, 3, 0} - for i := 0; i < 4; i++ { + for i := 0; i < len(startKeys); i++ { startKey := url.QueryEscape(startKeys[i]) endKey := url.QueryEscape(endKeys[i]) argsWithHoles := fmt.Sprintf("?start_key=%s&end_key=%s&count", startKey, endKey) From 81bac968f428071914abb8ce798e990f84dc2018 Mon Sep 17 00:00:00 2001 From: Boyang Lyu Date: Mon, 28 Oct 2024 13:56:38 +0800 Subject: [PATCH 5/8] restore failpoint injection code Signed-off-by: Boyang Lyu --- pkg/core/region.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/core/region.go b/pkg/core/region.go index 823bf4e45c2..27948be0f21 100755 --- a/pkg/core/region.go +++ b/pkg/core/region.go @@ -1249,11 +1249,11 @@ func (r *RegionsInfo) setRegionLocked(region *RegionInfo, withOverlaps bool, ol // UpdateSubTree updates the subtree. func (r *RegionsInfo) UpdateSubTree(region, origin *RegionInfo, overlaps []*RegionInfo, rangeChanged bool) { - if _, _err_ := failpoint.Eval(_curpkg_("UpdateSubTree")); _err_ == nil { + failpoint.Inject("UpdateSubTree", func() { if origin == nil { time.Sleep(time.Second) } - } + }) r.st.Lock() defer r.st.Unlock() if origin != nil { From 89c1f57383a6b5ec2a4e7fe9500eafd24f5f7e45 Mon Sep 17 00:00:00 2001 From: Boyang Lyu Date: Mon, 28 Oct 2024 14:02:24 +0800 Subject: [PATCH 6/8] fixed style issue Signed-off-by: Boyang Lyu --- server/api/stats_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/api/stats_test.go b/server/api/stats_test.go index 83a776a1883..3142eef2904 100644 --- a/server/api/stats_test.go +++ b/server/api/stats_test.go @@ -288,7 +288,7 @@ func TestRegionStatsHoles(t *testing.T) { endKeys := [6]string{"e", "e", "i", "j", "", ""} ans := [6]int{0, 1, 2, 1, 3, 0} - for i := 0; i < len(startKeys); i++ { + for i := range startKeys { startKey := url.QueryEscape(startKeys[i]) endKey := url.QueryEscape(endKeys[i]) argsWithHoles := fmt.Sprintf("?start_key=%s&end_key=%s&count", startKey, endKey) From e3792b0f4e5ab2a1978d6ccb18244aaca5b7ed45 Mon Sep 17 00:00:00 2001 From: Boyang Lyu Date: Mon, 28 Oct 2024 14:28:40 +0800 Subject: [PATCH 7/8] seperate sit with StartIndex Signed-off-by: Boyang Lyu --- pkg/core/region.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/pkg/core/region.go b/pkg/core/region.go index 27948be0f21..c1bef53ba34 100755 --- a/pkg/core/region.go +++ b/pkg/core/region.go @@ -1791,12 +1791,12 @@ func (r *RegionsInfo) GetRegionCount(startKey, endKey []byte) int { end := ®ionItem{&RegionInfo{meta: &metapb.Region{StartKey: endKey}}} // check if both keys are in the uncovered ranges. - startItem, startIndex := r.tree.tree.GetWithIndex(start) + startItem, sit := r.tree.tree.GetWithIndex(start) endItem, eit := r.tree.tree.GetWithIndex(end) if startItem == nil { - startItem = r.tree.tree.GetAt(startIndex - 1) + startItem = r.tree.tree.GetAt(sit - 1) } else { - startIndex += 1 + sit += 1 } if endItem == nil { endItem = r.tree.tree.GetAt(eit - 1) @@ -1818,11 +1818,11 @@ func (r *RegionsInfo) GetRegionCount(startKey, endKey []byte) int { eit = r.tree.tree.Len() endInAnInterval = (bytes.Compare(endItem.GetEndKey(), endKey) <= 0) && (bytes.Compare(endKey, endItem.GetEndKey()) <= 0) } - - if startIndex == eit && (!startInAnInterval) && (!endInAnInterval) { + if sit == eit && (!startInAnInterval) && (!endInAnInterval) { return 0 } + _, startIndex := r.tree.tree.GetWithIndex(start) var endIndex int var item *regionItem // it should return the length of the tree if endKey is nil. @@ -1835,7 +1835,6 @@ func (r *RegionsInfo) GetRegionCount(startKey, endKey []byte) int { endIndex-- } } - return endIndex - startIndex + 1 } From 14f545529b568f459e1044ccdb0518badb771513 Mon Sep 17 00:00:00 2001 From: Boyang Lyu Date: Mon, 28 Oct 2024 14:40:34 +0800 Subject: [PATCH 8/8] fixed nullptr dereference bug Signed-off-by: Boyang Lyu --- pkg/core/region.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/core/region.go b/pkg/core/region.go index c1bef53ba34..6408566663c 100755 --- a/pkg/core/region.go +++ b/pkg/core/region.go @@ -1816,7 +1816,10 @@ func (r *RegionsInfo) GetRegionCount(startKey, endKey []byte) int { if len(endKey) == 0 { endItem = r.tree.tree.GetAt(r.tree.tree.Len() - 1) eit = r.tree.tree.Len() - endInAnInterval = (bytes.Compare(endItem.GetEndKey(), endKey) <= 0) && (bytes.Compare(endKey, endItem.GetEndKey()) <= 0) + endInAnInterval = false + if endItem != nil { + endInAnInterval = (bytes.Compare(endItem.GetEndKey(), endKey) <= 0) && (bytes.Compare(endKey, endItem.GetEndKey()) <= 0) + } } if sit == eit && (!startInAnInterval) && (!endInAnInterval) { return 0