mcs: fix the scatter error #7241

rleungx · 2023-10-20T09:06:06Z

What problem does this PR solve?

Issue Number: Close #7234.

What is changed and how does it work?

Check List

Tests

Integration test

https://tcms.pingcap.net/dashboard/executions/plan/3450271

Release note

None.

ti-chi-bot · 2023-10-20T09:06:09Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

JmPotato
lhy1024

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.

codecov · 2023-10-23T02:55:43Z

Codecov Report

Merging #7241 (ee05b8c) into master (1899f41) will increase coverage by 0.01%.
Report is 2 commits behind head on master.
The diff coverage is 29.16%.

@@            Coverage Diff             @@
##           master    #7241      +/-   ##
==========================================
+ Coverage   74.35%   74.37%   +0.01%     
==========================================
  Files         446      446              
  Lines       48159    48198      +39     
==========================================
+ Hits        35810    35846      +36     
+ Misses       9191     9189       -2     
- Partials     3158     3163       +5

Flag	Coverage Δ
unittests	`74.37% <29.16%> (+0.01%)`	⬆️

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

rleungx · 2023-10-23T02:58:20Z

/check-issue-triage-complete

Signed-off-by: Ryan Leung <rleungx@gmail.com>

lhy1024

the rest LGTM

lhy1024 · 2023-10-23T12:45:22Z

pkg/schedule/scatter/region_scatterer.go

+			if len(regionsID) == 1 {
+				return 0, nil, errors.New("region not found")
+			}


how about moving it to L188

if len(regionsID) == 1 { if r.cluster.GetRegion(regionsID[0])==nil{ scatterSkipNoRegionCounter.Inc() return 0, nil, errors.New("region not found") } }

lhy1024 · 2023-10-23T12:46:24Z

server/grpc_service.go

+			var regionsID []uint64
+			// nolint
+			if request.GetRegionId() != 0 {
+				// nolint
+				regionsID = []uint64{request.GetRegionId()}
+			} else {
+				regionsID = request.GetRegionsId()
+			}


why need nolint?

GetRegionId is deprecated

lhy1024 · 2023-10-23T12:47:02Z

server/grpc_service.go

@@ -2134,6 +2152,15 @@ func (s *GrpcServer) invalidValue(msg string) *pdpb.ResponseHeader {
 func (s *GrpcServer) convertHeader(header *schedulingpb.ResponseHeader) *pdpb.ResponseHeader {
 	switch header.GetError().GetType() {
 	case schedulingpb.ErrorType_UNKNOWN:
+		if strings.Contains(header.GetError().GetMessage(), "region not found") {


how about using const value for "region not found"

Signed-off-by: Ryan Leung <rleungx@gmail.com>

lhy1024 · 2023-10-24T05:06:58Z

pkg/schedule/scatter/region_scatterer.go

-		return 0, nil, errors.New("empty region")
+		return 0, nil, errEmptyRegion
+	}
+	if len(regionsID) == 1 {


It's different from previous, do we need to check whether it is nil?

Signed-off-by: Ryan Leung <rleungx@gmail.com>

lhy1024 · 2023-10-25T02:38:49Z

/merge

ti-chi-bot · 2023-10-25T02:38:50Z

@lhy1024: 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-10-25T02:38:52Z

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

Commit hash: 7e28af7

close tikv#7234 Signed-off-by: Ryan Leung <rleungx@gmail.com> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>

ti-chi-bot bot added do-not-merge/needs-triage-completed release-note-none Denotes a PR that doesn't merit a release note. labels Oct 20, 2023

ti-chi-bot bot requested review from disksing and Yisaer October 20, 2023 09:06

ti-chi-bot bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 20, 2023

rleungx requested review from JmPotato and lhy1024 and removed request for disksing and Yisaer October 23, 2023 02:28

rleungx force-pushed the fix-scatter-issue branch from 894a41b to 87c4368 Compare October 23, 2023 02:30

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

fix the scatter error

e19265f

Signed-off-by: Ryan Leung <rleungx@gmail.com>

rleungx force-pushed the fix-scatter-issue branch from 87c4368 to e19265f Compare October 23, 2023 07:35

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 23, 2023

JmPotato approved these changes Oct 23, 2023

View reviewed changes

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

fix stream panic

55dd28e

Signed-off-by: Ryan Leung <rleungx@gmail.com>

lhy1024 reviewed Oct 23, 2023

View reviewed changes

address comments

c209d34

Signed-off-by: Ryan Leung <rleungx@gmail.com>

lhy1024 reviewed Oct 24, 2023

View reviewed changes

address comments

7e28af7

Signed-off-by: Ryan Leung <rleungx@gmail.com>

lhy1024 approved these changes Oct 25, 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 Oct 25, 2023

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

Merge branch 'master' into fix-scatter-issue

ee05b8c

ti-chi-bot bot merged commit 35b2719 into tikv:master Oct 25, 2023
19 checks passed

rleungx deleted the fix-scatter-issue branch October 25, 2023 04:16

rleungx mentioned this pull request Nov 30, 2023

Separate scheduling into an independent service #5839

Closed

rleungx added a commit to rleungx/pd that referenced this pull request Dec 1, 2023

mcs: fix the scatter error (tikv#7241)

0c4819f

close tikv#7234 Signed-off-by: Ryan Leung <rleungx@gmail.com> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mcs: fix the scatter error #7241

mcs: fix the scatter error #7241

rleungx commented Oct 20, 2023 •

edited

Loading

ti-chi-bot bot commented Oct 20, 2023 •

edited

Loading

codecov bot commented Oct 23, 2023 •

edited

Loading

rleungx commented Oct 23, 2023

lhy1024 left a comment

lhy1024 Oct 23, 2023

lhy1024 Oct 23, 2023

rleungx Oct 24, 2023

lhy1024 Oct 23, 2023

lhy1024 Oct 24, 2023

lhy1024 commented Oct 25, 2023

ti-chi-bot bot commented Oct 25, 2023

ti-chi-bot bot commented Oct 25, 2023

mcs: fix the scatter error #7241

mcs: fix the scatter error #7241

Conversation

rleungx commented Oct 20, 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 Oct 20, 2023 • edited Loading

codecov bot commented Oct 23, 2023 • edited Loading

Codecov Report

rleungx commented Oct 23, 2023

lhy1024 left a comment

Choose a reason for hiding this comment

lhy1024 Oct 23, 2023

Choose a reason for hiding this comment

lhy1024 Oct 23, 2023

Choose a reason for hiding this comment

rleungx Oct 24, 2023

Choose a reason for hiding this comment

lhy1024 Oct 23, 2023

Choose a reason for hiding this comment

lhy1024 Oct 24, 2023

Choose a reason for hiding this comment

lhy1024 commented Oct 25, 2023

ti-chi-bot bot commented Oct 25, 2023

ti-chi-bot bot commented Oct 25, 2023

rleungx commented Oct 20, 2023 •

edited

Loading

ti-chi-bot bot commented Oct 20, 2023 •

edited

Loading

codecov bot commented Oct 23, 2023 •

edited

Loading