*: fix region stats check #7748

CabinfeverB · 2024-01-23T08:30:50Z

What problem does this PR solve?

Issue Number: Close #7728

What is changed and how does it work?

isNew is useless because region source is inexact when leader transfer and no sync. Now, no need to hold isNew because saveCache must be true if the region in cache does not have leader.
check whether the region is observed by its own map

Check List

Tests

Unit test
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 <cabinfeveroier@gmail.com>

ti-chi-bot · 2024-01-23T08:30:53Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

HuSharp
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.

CabinfeverB · 2024-01-23T08:33:51Z

/run-check-issue-triage-complete

pkg/core/region.go

rleungx · 2024-01-23T08:59:53Z

pkg/core/region.go

-				if origin.GetLeader().GetId() == 0 {
-					isNew = true
-				} else if log.GetLevel() <= zap.InfoLevel {
+				if log.GetLevel() <= zap.InfoLevel {
 					info("leader changed",


Will it print leader 0?

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

codecov · 2024-01-24T03:42:14Z

Codecov Report

Merging #7748 (43cc64d) into master (37be34e) will decrease coverage by 0.04%.
Report is 2 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7748      +/-   ##
==========================================
- Coverage   73.47%   73.44%   -0.04%     
==========================================
  Files         432      432              
  Lines       47842    47843       +1     
==========================================
- Hits        35151    35136      -15     
- Misses       9648     9666      +18     
+ Partials     3043     3041       -2

Flag	Coverage Δ
unittests	`73.44% <100.00%> (-0.04%)`	⬇️

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

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

pkg/core/region.go

server/cluster/cluster.go

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

HuSharp · 2024-02-01T09:04:17Z

pkg/statistics/region_collection.go

@@ -240,10 +250,10 @@ func (r *RegionStatistics) Observe(region *core.RegionInfo, stores []*core.Store
 		}
 	}
 	// Remove the info if any of the conditions are not met any more.
-	if oldIndex, ok := r.index[regionID]; ok {
-		deleteIndex = oldIndex &^ peerTypeIndex
+	if oldIndex, ok := r.index[regionID]; ok && oldIndex > 0 {


for oldIndex > 0

0 for region state feels a bit weird, do you think we need to add a state name to 0? Like notRecord or something.
and maybe better != 0?

For bit operations. != doesn't seem to be used very often

HuSharp · 2024-02-01T09:05:20Z

pkg/core/region.go

@@ -718,19 +724,16 @@ func GenerateRegionGuideFunc(enableLog bool) RegionGuideFunc {
 	}
 	// Save to storage if meta is updated.
 	// Save to cache if meta or leader is updated, or contains any down/pending peer.
-	// Mark isNew if the region in cache does not have leader.
-	return func(region, origin *RegionInfo) (isNew, saveKV, saveCache, needSync bool) {
+	// Now, no need to hold `isNew` because `saveCache` must be true if the region in cache does not have leader.


maybe can move this statement to pr description

Suggested change

// Now, no need to hold `isNew` because `saveCache` must be true if the region in cache does not have leader.

Got it. But I think we can keep this comment in code.

Oh 😯 I mean, after your code merged, isNew is a word which doesn't exist and could be confused others.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

CabinfeverB · 2024-02-06T03:27:12Z

/merge

ti-chi-bot · 2024-02-06T03:27:14Z

@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 · 2024-02-06T03:27:16Z

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

Commit hash: cf03d8c

ti-chi-bot · 2024-02-06T08:15:36Z

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

close tikv#7728 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

close #7728 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io> Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> Co-authored-by: Yongbo Jiang <cabinfeveroier@gmail.com> Co-authored-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot · 2024-02-22T08:10:59Z

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

close tikv#7728 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

close #7728 Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> Co-authored-by: Yongbo Jiang <cabinfeveroier@gmail.com> Co-authored-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot · 2024-03-25T05:36:04Z

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

close tikv#7728 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

close tikv#7728 Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

close #7728 Signed-off-by: husharp <jinhao.hu@pingcap.com> Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> Co-authored-by: husharp <jinhao.hu@pingcap.com> Co-authored-by: Yongbo Jiang <cabinfeveroier@gmail.com> Co-authored-by: Cabinfever_B <cabinfeveroier@gmail.com>

fix region stats check

62c5424

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

CabinfeverB requested review from AndreMouche, rleungx and HuSharp January 23, 2024 08:30

ti-chi-bot bot added do-not-merge/needs-triage-completed release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 23, 2024

ti-chi-bot bot removed the do-not-merge/needs-triage-completed label Jan 23, 2024

rleungx reviewed Jan 23, 2024

View reviewed changes

fix region stats check

2d055ed

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

address comment

c545ac8

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

HuSharp reviewed Jan 24, 2024

View reviewed changes

pkg/core/region.go Outdated Show resolved Hide resolved

server/cluster/cluster.go Outdated Show resolved Hide resolved

address comment

77dfa24

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

CabinfeverB requested review from rleungx and HuSharp January 31, 2024 15:19

HuSharp reviewed Feb 1, 2024

View reviewed changes

CabinfeverB added 3 commits February 1, 2024 17:36

address comment

dfa0a64

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

Merge branch 'master' into fix_stats_region

73f62c4

address comment

cf03d8c

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

HuSharp approved these changes Feb 1, 2024

View reviewed changes

ti-chi-bot bot added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 1, 2024

rleungx approved these changes Feb 6, 2024

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 Feb 6, 2024

ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 6, 2024

Merge branch 'master' into fix_stats_region

43cc64d

ti-chi-bot bot merged commit cce1464 into tikv:master Feb 6, 2024
25 of 26 checks passed

ti-chi-bot added the needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. label Feb 6, 2024

ti-chi-bot pushed a commit to ti-chi-bot/pd that referenced this pull request Feb 6, 2024

This is an automated cherry-pick of tikv#7748

032e31d

close tikv#7728 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Feb 6, 2024

*: fix region stats check (#7748) #7812

Merged

ti-chi-bot added the needs-cherry-pick-release-7.1 Should cherry pick this PR to release-7.1 branch. label Feb 22, 2024

ti-chi-bot pushed a commit to ti-chi-bot/pd that referenced this pull request Feb 22, 2024

This is an automated cherry-pick of tikv#7748

ebc342c

close tikv#7728 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Feb 22, 2024

*: fix region stats check (#7748) #7840

Merged

CabinfeverB deleted the fix_stats_region branch February 26, 2024 06:29

rleungx pushed a commit to rleungx/pd that referenced this pull request Feb 27, 2024

*: fix region stats check (tikv#7748) (tikv#264)

f09344b

ti-chi-bot added the needs-cherry-pick-release-6.5 Should cherry pick this PR to release-6.5 branch. label Mar 25, 2024

ti-chi-bot pushed a commit to ti-chi-bot/pd that referenced this pull request Mar 25, 2024

This is an automated cherry-pick of tikv#7748

f52a6e1

close tikv#7728 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Mar 25, 2024

*: fix region stats check (#7748) #7976

Merged

CabinfeverB added a commit to ti-chi-bot/pd that referenced this pull request Mar 27, 2024

*: fix region stats check (tikv#7748)

a9d5716

close tikv#7728 Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx mentioned this pull request Oct 15, 2024

Memory leak in region statistics #8710

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: fix region stats check #7748

*: fix region stats check #7748

CabinfeverB commented Jan 23, 2024 •

edited

Loading

ti-chi-bot bot commented Jan 23, 2024 •

edited

Loading

CabinfeverB commented Jan 23, 2024

rleungx Jan 23, 2024

codecov bot commented Jan 24, 2024 •

edited

Loading

HuSharp Feb 1, 2024

CabinfeverB Feb 1, 2024

HuSharp Feb 1, 2024

CabinfeverB Feb 1, 2024

HuSharp Feb 1, 2024

CabinfeverB Feb 1, 2024

CabinfeverB commented Feb 6, 2024

ti-chi-bot bot commented Feb 6, 2024

ti-chi-bot bot commented Feb 6, 2024

ti-chi-bot commented Feb 6, 2024

ti-chi-bot commented Feb 22, 2024

ti-chi-bot commented Mar 25, 2024

*: fix region stats check #7748

*: fix region stats check #7748

Conversation

CabinfeverB commented Jan 23, 2024 • 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 Jan 23, 2024 • edited Loading

CabinfeverB commented Jan 23, 2024

rleungx Jan 23, 2024

Choose a reason for hiding this comment

codecov bot commented Jan 24, 2024 • edited Loading

Codecov Report

HuSharp Feb 1, 2024

Choose a reason for hiding this comment

CabinfeverB Feb 1, 2024

Choose a reason for hiding this comment

HuSharp Feb 1, 2024

Choose a reason for hiding this comment

CabinfeverB Feb 1, 2024

Choose a reason for hiding this comment

HuSharp Feb 1, 2024

Choose a reason for hiding this comment

CabinfeverB Feb 1, 2024

Choose a reason for hiding this comment

CabinfeverB commented Feb 6, 2024

ti-chi-bot bot commented Feb 6, 2024

ti-chi-bot bot commented Feb 6, 2024

ti-chi-bot commented Feb 6, 2024

ti-chi-bot commented Feb 22, 2024

ti-chi-bot commented Mar 25, 2024

CabinfeverB commented Jan 23, 2024 •

edited

Loading

ti-chi-bot bot commented Jan 23, 2024 •

edited

Loading

codecov bot commented Jan 24, 2024 •

edited

Loading