statistics: fix empty region count when resuming #7009

CabinfeverB · 2023-08-30T06:41:05Z

What problem does this PR solve?

Issue Number: Close #7008 #5273

What is changed and how does it work?

We only save meta when save regions, so the approximate size of the original region is zero when restarting and Inherit is useless.

Check List

Tests

Manual test (add detailed scripts or steps below)

Code changes

Has the configuration change
Has HTTP API interfaces changed (Don't forget to add the declarative for the new API)
Has persistent data change

Side effects

Possible performance regression
Increased code complexity
Breaking backward compatibility

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
PR to update pingcap/tiup:
Need to cherry-pick to the release branch

Release note

None.

Signed-off-by: Cabinfever_B <[email protected]>

ti-chi-bot · 2023-08-30T06:41:06Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

Connor1996
rleungx

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

rleungx · 2023-08-30T06:52:50Z

pkg/statistics/region_collection.go

@@ -198,7 +198,9 @@ func (r *RegionStatistics) Observe(region *core.RegionInfo, stores []*core.Store
 			return false
 		}(),
 		LearnerPeer: len(region.GetLearners()) > 0,
-		EmptyRegion: region.GetApproximateSize() <= core.EmptyRegionApproximateSize,
+		// When cluster resumes, the region size may be not initialized, but region heartbeat is send.


If the region size is not initialized, will it inherit from the original one?

It will not because the approximate size of original region is zero after pause.

Signed-off-by: Cabinfever_B <[email protected]>

codecov · 2023-08-30T10:44:05Z

Codecov Report

Merging #7009 (511ca0e) into master (41eae1a) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #7009      +/-   ##
==========================================
- Coverage   74.38%   74.36%   -0.03%     
==========================================
  Files         436      436              
  Lines       46437    46435       -2     
==========================================
- Hits        34544    34530      -14     
- Misses       8869     8875       +6     
- Partials     3024     3030       +6

Flag	Coverage Δ
unittests	`74.36% <100.00%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Connor1996 · 2023-08-31T03:01:42Z

pkg/statistics/region.go

@@ -59,7 +59,7 @@ 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


approximateSize == 1 means empty, should we add it to storage size?

Do u mean current storage size? If so, it is calculated from the used size of store

I mean

if approximateSize > core.EmptyRegionApproximateSize { s.StorageSize += approximateSize }

Connor1996 · 2023-08-31T06:26:44Z

pkg/statistics/region.go

@@ -59,7 +59,7 @@ 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


I mean

if approximateSize > core.EmptyRegionApproximateSize { s.StorageSize += approximateSize }

Connor1996 · 2023-08-31T06:28:58Z

pkg/statistics/region_collection.go

-		EmptyRegion: region.GetApproximateSize() <= core.EmptyRegionApproximateSize,
+		// When cluster resumes, the region size may be not initialized, but region heartbeat is send.
+		// So use `==` here.
+		EmptyRegion: region.GetApproximateSize() == core.EmptyRegionApproximateSize,


can we extract a function IsEmptyRegion() for regionInfo

Signed-off-by: Cabinfever_B <[email protected]>

Connor1996

LGTM

CabinfeverB · 2023-09-07T08:10:02Z

/merge

ti-chi-bot · 2023-09-07T08:10:04Z

@CabinfeverB: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2023-09-07T08:10:06Z

This pull request has been accepted and is ready to merge.

Commit hash: c1e170e

ti-chi-bot · 2023-09-07T08:10:19Z

@CabinfeverB: Your PR was out of date, I have automatically updated it for you.

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2023-09-07T08:49:24Z

In response to a cherrypick label: new pull request created to branch release-6.5: #7055.

close tikv#7008 Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2023-09-07T08:50:02Z

In response to a cherrypick label: new pull request created to branch release-7.1: #7056.

close tikv#7008 Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2023-09-07T08:50:40Z

In response to a cherrypick label: new pull request created to branch release-6.1: #7057.

close tikv#7008 Signed-off-by: ti-chi-bot <[email protected]>

close #7008 Signed-off-by: ti-chi-bot <[email protected]> Signed-off-by: Cabinfever_B <[email protected]> Co-authored-by: Yongbo Jiang <[email protected]> Co-authored-by: Cabinfever_B <[email protected]>

This reverts commit 74ead5c. Signed-off-by: Cabinfever_B <[email protected]>

…ikv#7055)" This reverts commit 8c5859e. Signed-off-by: Cabinfever_B <[email protected]>

) close #7148 Signed-off-by: Cabinfever_B <[email protected]>

#7150) ref #7148 Signed-off-by: Cabinfever_B <[email protected]>

fix

941cd35

Signed-off-by: Cabinfever_B <[email protected]>

CabinfeverB requested a review from rleungx August 30, 2023 06:41

ti-chi-bot bot requested a review from lhy1024 August 30, 2023 06:41

CabinfeverB requested review from Connor1996 and removed request for lhy1024 August 30, 2023 06:41

ti-chi-bot bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 30, 2023

rleungx reviewed Aug 30, 2023

View reviewed changes

remove

0c692ac

Signed-off-by: Cabinfever_B <[email protected]>

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 30, 2023

CabinfeverB requested a review from bufferflies August 30, 2023 09:21

CabinfeverB added 2 commits August 30, 2023 17:49

fix

9a4c15e

Signed-off-by: Cabinfever_B <[email protected]>

fix undersized

703d845

Signed-off-by: Cabinfever_B <[email protected]>

rleungx approved these changes Aug 31, 2023

View reviewed changes

ti-chi-bot bot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 31, 2023

Connor1996 reviewed Aug 31, 2023

View reviewed changes

address comment

c1e170e

Signed-off-by: Cabinfever_B <[email protected]>

CabinfeverB requested a review from Connor1996 September 4, 2023 09:07

Connor1996 approved these changes Sep 6, 2023

View reviewed changes

ti-chi-bot bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 6, 2023

rleungx mentioned this pull request Sep 6, 2023

Identify regions of unknown size as empty-region and undersized-region. #5273

Closed

ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 7, 2023

Merge branch 'master' into fix_empty

511ca0e

ti-chi-bot bot merged commit 74ead5c into tikv:master Sep 7, 2023
23 checks passed

ti-chi-bot mentioned this pull request Sep 7, 2023

statistics: fix empty region count when resuming (#7009) #7055

Merged

ti-chi-bot pushed a commit to ti-chi-bot/pd that referenced this pull request Sep 7, 2023

This is an automated cherry-pick of tikv#7009

3c933dc

close tikv#7008 Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Sep 7, 2023

statistics: fix empty region count when resuming (#7009) #7056

Closed

ti-chi-bot pushed a commit to ti-chi-bot/pd that referenced this pull request Sep 7, 2023

This is an automated cherry-pick of tikv#7009

4fb2327

close tikv#7008 Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Sep 7, 2023

statistics: fix empty region count when resuming (#7009) #7057

Closed

ti-chi-bot pushed a commit to ti-chi-bot/pd that referenced this pull request Sep 7, 2023

This is an automated cherry-pick of tikv#7009

2ce73d5

close tikv#7008 Signed-off-by: ti-chi-bot <[email protected]>

CabinfeverB mentioned this pull request Sep 11, 2023

test: fix unstable test TestScheduler #7074

Merged

CabinfeverB added a commit to CabinfeverB/pd that referenced this pull request Sep 25, 2023

Revert "statistics: fix empty region count when resuming (tikv#7009)"

962fd74

This reverts commit 74ead5c. Signed-off-by: Cabinfever_B <[email protected]>

CabinfeverB added a commit to CabinfeverB/pd that referenced this pull request Sep 25, 2023

Revert "statistics: fix empty region count when resuming (tikv#7009) (t…

727ab1e

…ikv#7055)" This reverts commit 8c5859e. Signed-off-by: Cabinfever_B <[email protected]>

ti-chi-bot bot pushed a commit that referenced this pull request Sep 25, 2023

Revert "statistics: fix empty region count when resuming (#7009)" (#7149

eac55a7

) close #7148 Signed-off-by: Cabinfever_B <[email protected]>

ti-chi-bot bot pushed a commit that referenced this pull request Sep 26, 2023

Revert "statistics: fix empty region count when resuming (#7009) (#70… (

b0207a6

#7150) ref #7148 Signed-off-by: Cabinfever_B <[email protected]>

CabinfeverB mentioned this pull request Sep 26, 2023

region is not balanced during br restore #7148

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

statistics: fix empty region count when resuming #7009

statistics: fix empty region count when resuming #7009

CabinfeverB commented Aug 30, 2023 •

edited

Loading

ti-chi-bot bot commented Aug 30, 2023 •

edited

Loading

rleungx Aug 30, 2023

CabinfeverB Aug 30, 2023

codecov bot commented Aug 30, 2023 •

edited

Loading

Connor1996 Aug 31, 2023

CabinfeverB Aug 31, 2023

Connor1996 Aug 31, 2023

Connor1996 Aug 31, 2023

Connor1996 Aug 31, 2023

Connor1996 left a comment

CabinfeverB commented Sep 7, 2023

ti-chi-bot bot commented Sep 7, 2023

ti-chi-bot bot commented Sep 7, 2023

ti-chi-bot bot commented Sep 7, 2023

ti-chi-bot commented Sep 7, 2023

ti-chi-bot commented Sep 7, 2023

ti-chi-bot commented Sep 7, 2023

statistics: fix empty region count when resuming #7009

statistics: fix empty region count when resuming #7009

Conversation

CabinfeverB commented Aug 30, 2023 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Aug 30, 2023 • edited Loading

rleungx Aug 30, 2023

Choose a reason for hiding this comment

CabinfeverB Aug 30, 2023

Choose a reason for hiding this comment

codecov bot commented Aug 30, 2023 • edited Loading

Codecov Report

Connor1996 Aug 31, 2023

Choose a reason for hiding this comment

CabinfeverB Aug 31, 2023

Choose a reason for hiding this comment

Connor1996 Aug 31, 2023

Choose a reason for hiding this comment

Connor1996 Aug 31, 2023

Choose a reason for hiding this comment

Connor1996 Aug 31, 2023

Choose a reason for hiding this comment

Connor1996 left a comment

Choose a reason for hiding this comment

CabinfeverB commented Sep 7, 2023

ti-chi-bot bot commented Sep 7, 2023

ti-chi-bot bot commented Sep 7, 2023

ti-chi-bot bot commented Sep 7, 2023

ti-chi-bot commented Sep 7, 2023

ti-chi-bot commented Sep 7, 2023

ti-chi-bot commented Sep 7, 2023

CabinfeverB commented Aug 30, 2023 •

edited

Loading

ti-chi-bot bot commented Aug 30, 2023 •

edited

Loading

codecov bot commented Aug 30, 2023 •

edited

Loading