Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

*: Bug Fix/CLUSTER_INFO system table may not work after PD is scaled-in #2830

Merged
merged 9 commits into from
Aug 26, 2020
Merged

*: Bug Fix/CLUSTER_INFO system table may not work after PD is scaled-in #2830

merged 9 commits into from
Aug 26, 2020

Conversation

jyz0309
Copy link
Contributor

@jyz0309 jyz0309 commented Aug 24, 2020 •
edited
Loading

What problem does this PR solve?

Issue Number: close pingcap/tidb#18990

Problem Summary:

CLUSTER_INFO system table may not work after PD is scaled-in

What is changed and how it works?

What's Changed:

Add the GetMemberInfo function to client interface, use grpc GetMembers() to get the pd members info.

How it Works:

use grpc GetMembers() to get the pd members info.

Related changes

  • PR to update pingcap/pd/client/client.go;
  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below)

steps:

  • use tiup playground scale-out --pd 1 to scale out pd
  • use mysql to connect TiDB
  • select * from INFORMATION_SCHEMA.CLUSTER_INFO; to test scale out and scale in

Side effects

  • Performance regression
    • Consumes more CPU

Release note

  • add GetMemberInfo() function to get pd member info

@jyz0309
init
918745d 
Signed-off-by: jyz0309 <45495947@qq.com>
@ti-srebot ti-srebot added the contribution This PR is from a community contributor. label Aug 24, 2020
@jyz0309
add the cmdDurationGetMemberInfo
de33264 
Signed-off-by: jyz0309 <45495947@qq.com>
@tikv tikv deleted a comment from jyz0309 Aug 25, 2020
@JmPotato
Copy link
Member

/rebuild

rleungx
rleungx reviewed Aug 25, 2020
View reviewed changes
client/client.go Outdated Show resolved Hide resolved
@JmPotato JmPotato added the component/client Client logic. label Aug 25, 2020
@jyz0309
add the request header
c0c6590 
Signed-off-by: jyz0309 <45495947@qq.com>
Yisaer
Yisaer reviewed Aug 25, 2020
View reviewed changes
Copy link
Contributor

@Yisaer Yisaer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM

client/base_client.go Show resolved Hide resolved
server/api/status.go Outdated Show resolved Hide resolved
go.sum Outdated Show resolved Hide resolved
client/client.go Outdated Show resolved Hide resolved
client/client.go Outdated Show resolved Hide resolved
@jyz0309
client/client.go
db34622 
Signed-off-by: jyz0309 <45495947@qq.com>
@jyz0309
Merge branch 'bug-fix/pd_scale_bug' of github.com:jyz0309/pd into bug…
e0e75ae 
…-fix/pd_scale_bug

to update
@jyz0309
Copy link
Contributor Author

jyz0309 commented Aug 25, 2020

/rebuild

@jyz0309
update
cc27d56 
Signed-off-by: jyz0309 <45495947@qq.com>
@jyz0309
Copy link
Contributor Author

jyz0309 commented Aug 26, 2020

@JmPotato @Yisaer @rleungx PTAL

Yisaer
Yisaer approved these changes Aug 26, 2020
View reviewed changes
Copy link
Contributor

@Yisaer Yisaer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

client/base_client.go Show resolved Hide resolved
@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 26, 2020
nolouch
nolouch approved these changes Aug 26, 2020
View reviewed changes
Copy link
Contributor

@nolouch nolouch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-srebot ti-srebot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 26, 2020
@nolouch
Copy link
Contributor

nolouch commented Aug 26, 2020

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 26, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit e12e5a2 into tikv:master Aug 26, 2020
@jyz0309 jyz0309 deleted the bug-fix/pd_scale_bug branch August 26, 2020 14:09
@breezewish breezewish mentioned this pull request Sep 3, 2020
Merged
@HunDunDM
Copy link
Member

HunDunDM commented Sep 4, 2020 •
edited
Loading

I think GetAllMembers may be a more reasonable name. GetMemberInfo looks like it gets a single member.
For example, the function of the same name in server.go.

@nolouch nolouch added the needs-cherry-pick-release-4.0 The PR needs to cherry pick to release-4.0 branch. label Sep 17, 2020
ti-srebot pushed a commit to ti-srebot/pd that referenced this pull request Sep 17, 2020
@jyz0309 @ti-srebot
cherry pick tikv#2830 to release-4.0
c324041 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Sep 17, 2020
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #2980

ti-srebot added a commit that referenced this pull request Sep 21, 2020
@ti-srebot
*: Bug Fix/CLUSTER_INFO system table may not work after PD is scaled-…
9ee41c4 
…in (#2830) (#2980)

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
Signed-off-by: jyz0309 <45495947@qq.com>
This was referenced Sep 25, 2020
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rleungx rleungx rleungx left review comments

@JmPotato JmPotato JmPotato left review comments

@ti-srebot ti-srebot ti-srebot approved these changes

@Yisaer Yisaer Yisaer approved these changes

@nolouch nolouch nolouch approved these changes

Assignees
No one assigned
Labels
component/client Client logic. contribution This PR is from a community contributor. needs-cherry-pick-release-4.0 The PR needs to cherry pick to release-4.0 branch. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CLUSTER_INFO system table may not work after PD is scaled-in
7 participants
@jyz0309 @JmPotato @nolouch @ti-srebot @HunDunDM @Yisaer @rleungx