statistics: get region info via core cluster inside RegionStatistics #6804

JmPotato · 2023-07-13T09:02:37Z

What problem does this PR solve?

Issue Number: Close #6560.

What is changed and how does it work?

Instead of maintaining its own region info cache, use the core cluster to
fetch the region info inside `RegionStatistics` to make sure consistency.

Check List

Tests

Unit test
Integration test
Manual test

Run pd-ctl region check miss-peer:

[
  {
      "id": 22,
      "start_key": "7480000000000000FF5C00000000000000F8",
      "end_key": "748000FFFFFFFFFFFFF900000000000000F8",
      "epoch": {
        "conf_ver": 1,
        "version": 56
      },
      "peers": [
        {
          "id": 23,
          "store_id": 1,
          "role_name": "Voter"
        }
      ],
      "leader": {
        "id": 23,
        "store_id": 1,
        "role_name": "Voter"
      },
      "cpu_usage": 0,
      "written_bytes": 204,
      "read_bytes": 0,
      "written_keys": 4,
      "read_keys": 0,
      "approximate_size": 1,
      "approximate_keys": 0
  }
]

Run pd-ctl region 22:

{
  "id": 22,
  "start_key": "7480000000000000FF5C00000000000000F8",
  "end_key": "748000FFFFFFFFFFFFF900000000000000F8",
  "epoch": {
    "conf_ver": 1,
    "version": 56
  },
  "peers": [
    {
      "id": 23,
      "store_id": 1,
      "role_name": "Voter"
    }
  ],
  "leader": {
    "id": 23,
    "store_id": 1,
    "role_name": "Voter"
  },
  "cpu_usage": 0,
  "written_bytes": 204,
  "read_bytes": 0,
  "written_keys": 4,
  "read_keys": 0,
  "approximate_size": 1,
  "approximate_keys": 0
}

Release note

None.

ti-chi-bot · 2023-07-13T09:02:39Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

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

bufferflies · 2023-07-13T09:21:48Z

pkg/statistics/region_collection.go

-	for _, r := range r.stats[typ] {
-		res = append(res, r.RegionInfo.Clone())
+	for regionID := range r.stats[typ] {
+		res = append(res, r.core.GetRegion(regionID).Clone())


Do you need to check the region status that satisfy the required type?
for example , some region is down peer in the stats , but is healthy peer in th core.

I think it's unnecessary since (*RegionStatistics).Observe will be called right after the region info is updated in *core.RegionsInfo, therefore according to the current code, there will be no inconsistency.

codecov · 2023-07-13T09:27:39Z

Codecov Report

Merging #6804 (510a9c8) into master (58eb48b) will increase coverage by 0.13%.
The diff coverage is 100.00%.

❗ Current head 510a9c8 differs from pull request most recent head b729f30. Consider uploading reports for the commit b729f30 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6804      +/-   ##
==========================================
+ Coverage   74.13%   74.26%   +0.13%     
==========================================
  Files         413      413              
  Lines       43413    43304     -109     
==========================================
- Hits        32183    32160      -23     
+ Misses       8354     8285      -69     
+ Partials     2876     2859      -17

Flag	Coverage Δ
unittests	`74.26% <100.00%> (+0.13%)`	⬆️

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

rleungx · 2023-07-13T09:45:27Z

pkg/statistics/region_collection.go

-	for _, r := range r.offlineStats[typ] {
-		res = append(res, r.Clone())
+	for regionID := range r.offlineStats[typ] {
+		res = append(res, r.core.GetRegion(regionID).Clone())


Will it affect the heartbeat?

Considering that it will only be manually called by HTTP API and pd-ctl, I don't think it will affect the heartbeat too much.

What if we have some stores that are offline and each of them has many regions?

What about keeping the offline peer region recorded in the RegionStatistics as before to gain better performance in this case?

I changed this part of the logic to a mixture of old and new. PTAL.

pkg/statistics/region_collection.go

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

pkg/statistics/region_collection.go

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

JmPotato · 2023-07-17T06:27:54Z

/merge

ti-chi-bot · 2023-07-17T06:27:56Z

@JmPotato: 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-07-17T06:27:58Z

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

Commit hash: b729f30

ti-chi-bot · 2023-07-17T06:37:23Z

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

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

ti-chi-bot · 2023-07-17T06:38:02Z

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

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

ti-chi-bot · 2023-07-17T06:38:40Z

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

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

ti-chi-bot · 2023-07-17T06:39:19Z

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

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

ti-chi-bot · 2023-07-17T06:39:57Z

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

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

…ikv#6804) close tikv#6560 Instead of maintaining its own region info cache, use the core cluster to fetch the region info inside `RegionStatistics` to make sure consistency. Signed-off-by: JmPotato <[email protected]>

JmPotato requested review from nolouch, bufferflies and rleungx July 13, 2023 09:02

JmPotato force-pushed the fix_region_statis branch from 82aa324 to d472ccb Compare July 13, 2023 09:16

bufferflies reviewed Jul 13, 2023

View reviewed changes

rleungx reviewed Jul 13, 2023

View reviewed changes

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 13, 2023

JmPotato force-pushed the fix_region_statis branch from 1af975a to 896d6d5 Compare July 14, 2023 02:04

JmPotato requested review from bufferflies and rleungx July 14, 2023 02:05

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2023

JmPotato force-pushed the fix_region_statis branch from 896d6d5 to 8a1360a Compare July 17, 2023 03:24

ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 17, 2023

JmPotato added 5 commits July 17, 2023 11:25

Get region info via core cluster inside RegionStatistics

508437c

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

Fix the typo

e722bbb

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

Keep the offline region recorded inside RegionStatistics

464ac9e

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

Refine the naming

102b07f

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

Add offline check test

c86e28e

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

Refine the code

510a9c8

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

rleungx reviewed Jul 17, 2023

View reviewed changes

pkg/statistics/region_collection.go Outdated Show resolved Hide resolved

Remove offlineRegionStatusGauge

b729f30

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

rleungx approved these changes Jul 17, 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 Jul 17, 2023

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

CabinfeverB approved these changes Jul 17, 2023

View reviewed changes

ti-chi-bot bot merged commit 40eaa35 into tikv:master Jul 17, 2023

JmPotato deleted the fix_region_statis branch July 17, 2023 06:36

ti-chi-bot mentioned this pull request Jul 17, 2023

statistics: get region info via core cluster inside RegionStatistics (#6804) #6814

Closed

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

This is an automated cherry-pick of tikv#6804

7ccc118

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

ti-chi-bot mentioned this pull request Jul 17, 2023

statistics: get region info via core cluster inside RegionStatistics (#6804) #6816

Closed

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

This is an automated cherry-pick of tikv#6804

8993fcb

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

ti-chi-bot mentioned this pull request Jul 17, 2023

statistics: get region info via core cluster inside RegionStatistics (#6804) #6817

Closed

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

This is an automated cherry-pick of tikv#6804

8f03456

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

ti-chi-bot mentioned this pull request Jul 17, 2023

statistics: get region info via core cluster inside RegionStatistics (#6804) #6818

Closed

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

This is an automated cherry-pick of tikv#6804

e0fd110

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

ti-chi-bot mentioned this pull request Jul 17, 2023

statistics: get region info via core cluster inside RegionStatistics (#6804) #6819

Closed

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

This is an automated cherry-pick of tikv#6804

cf93563

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

lhy1024 mentioned this pull request Oct 26, 2023

pd panic which lead to lightning import failed #7261

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

statistics: get region info via core cluster inside RegionStatistics #6804

statistics: get region info via core cluster inside RegionStatistics #6804

JmPotato commented Jul 13, 2023 •

edited

Loading

ti-chi-bot bot commented Jul 13, 2023 •

edited

Loading

bufferflies Jul 13, 2023

JmPotato Jul 13, 2023 •

edited

Loading

codecov bot commented Jul 13, 2023 •

edited

Loading

rleungx Jul 13, 2023

JmPotato Jul 13, 2023

rleungx Jul 13, 2023

JmPotato Jul 13, 2023

JmPotato Jul 13, 2023

JmPotato commented Jul 17, 2023

ti-chi-bot bot commented Jul 17, 2023

ti-chi-bot bot commented Jul 17, 2023

ti-chi-bot commented Jul 17, 2023

ti-chi-bot commented Jul 17, 2023

ti-chi-bot commented Jul 17, 2023

ti-chi-bot commented Jul 17, 2023

ti-chi-bot commented Jul 17, 2023

statistics: get region info via core cluster inside RegionStatistics #6804

statistics: get region info via core cluster inside RegionStatistics #6804

Conversation

JmPotato commented Jul 13, 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 Jul 13, 2023 • edited Loading

bufferflies Jul 13, 2023

Choose a reason for hiding this comment

JmPotato Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jul 13, 2023 • edited Loading

Codecov Report

rleungx Jul 13, 2023

Choose a reason for hiding this comment

JmPotato Jul 13, 2023

Choose a reason for hiding this comment

rleungx Jul 13, 2023

Choose a reason for hiding this comment

JmPotato Jul 13, 2023

Choose a reason for hiding this comment

JmPotato Jul 13, 2023

Choose a reason for hiding this comment

JmPotato commented Jul 17, 2023

ti-chi-bot bot commented Jul 17, 2023

ti-chi-bot bot commented Jul 17, 2023

ti-chi-bot commented Jul 17, 2023

ti-chi-bot commented Jul 17, 2023

ti-chi-bot commented Jul 17, 2023

ti-chi-bot commented Jul 17, 2023

ti-chi-bot commented Jul 17, 2023

JmPotato commented Jul 13, 2023 •

edited

Loading

ti-chi-bot bot commented Jul 13, 2023 •

edited

Loading

JmPotato Jul 13, 2023 •

edited

Loading

codecov bot commented Jul 13, 2023 •

edited

Loading