Skip to content

client/base_client: do tiny refator for the function getMembers#3777

Merged
ti-chi-bot merged 6 commits into
tikv:masterfrom
AndreMouche:client/getmembers
Jun 15, 2021
Merged

client/base_client: do tiny refator for the function getMembers#3777
ti-chi-bot merged 6 commits into
tikv:masterfrom
AndreMouche:client/getmembers

Conversation

@AndreMouche
Copy link
Copy Markdown
Member

@AndreMouche AndreMouche commented Jun 15, 2021 •
edited
Loading

Signed-off-by: shirly AndreMouche@126.com

What problem does this PR solve?

This PR do some tiny refactor for the function getMembers in BaseClient

What is changed and how it works?

with this PR, we can use pd/clint by go get github.com/tikv/pd/client

Check List

Release note 

NONE

@AndreMouche
client/base_client: do tiny refator for funtion getMembers
3195d63 
Signed-off-by: shirly <AndreMouche@126.com>
@ti-chi-bot ti-chi-bot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Jun 15, 2021
@ti-chi-bot ti-chi-bot requested review from HunDunDM and rleungx June 15, 2021 08:46
@ti-chi-bot ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jun 15, 2021
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 15, 2021
@ti-chi-bot
Copy link
Copy Markdown
Member

ti-chi-bot commented Jun 15, 2021

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • disksing
  • 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.

Details

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

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 15, 2021
@disksing
Copy link
Copy Markdown
Contributor

disksing commented Jun 15, 2021

/merge

@ti-chi-bot
Copy link
Copy Markdown
Member

ti-chi-bot commented Jun 15, 2021

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

/run-all-tests

Details

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
Copy link
Copy Markdown
Member

ti-chi-bot commented Jun 15, 2021

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

DetailsCommit hash: 3195d63

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 15, 2021
rleungx
rleungx reviewed Jun 15, 2021
View reviewed changes
Comment thread client/base_client.go Outdated
}

func (c *baseClient) getMembers(ctx context.Context, url string) (*pdpb.GetMembersResponse, error) {
func (c *baseClient) getMembers(basicCtx context.Context, url string, timeout time.Duration) (*pdpb.GetMembersResponse, error) {
Copy link
Copy Markdown
Member

@rleungx rleungx Jun 15, 2021

Choose a reason for hiding this comment

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

What is basicCtx used for?

Copy link
Copy Markdown
Member Author

@AndreMouche AndreMouche Jun 15, 2021

Choose a reason for hiding this comment

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

good catch

@rleungx
Copy link
Copy Markdown
Member

rleungx commented Jun 15, 2021

/hold

@ti-chi-bot ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 15, 2021
@AndreMouche
client/base_client: address comments
5581cc0 
Signed-off-by: shirly <AndreMouche@126.com>
@ti-chi-bot ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Jun 15, 2021
@rleungx
Copy link
Copy Markdown
Member

rleungx commented Jun 15, 2021

/hold cancel

@ti-chi-bot ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 15, 2021
@rleungx
Copy link
Copy Markdown
Member

rleungx commented Jun 15, 2021

/merge

@ti-chi-bot
Copy link
Copy Markdown
Member

ti-chi-bot commented Jun 15, 2021

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

/run-all-tests

Details

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
Copy link
Copy Markdown
Member

ti-chi-bot commented Jun 15, 2021

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

DetailsCommit hash: 5581cc0

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 15, 2021
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 15, 2021 •
edited
Loading

Codecov Report

Merging #3777 (eacda0c) into master (938948a) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3777      +/-   ##
==========================================
- Coverage   75.01%   74.94%   -0.07%     
==========================================
  Files         245      245              
  Lines       24531    24529       -2     
==========================================
- Hits        18401    18384      -17     
- Misses       4501     4514      +13     
- Partials     1629     1631       +2
Flag Coverage Δ
unittests 74.94% <100.00%> (-0.07%) ⬇️

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

Impacted Files Coverage Δ
client/base_client.go 82.29% <100.00%> (-2.25%) ⬇️
server/id/id.go 76.19% <0.00%> (-4.77%) ⬇️
server/region_syncer/client.go 78.19% <0.00%> (-4.52%) ⬇️
server/statistics/hot_cache_task.go 75.00% <0.00%> (-4.17%) ⬇️
pkg/etcdutil/etcdutil.go 84.70% <0.00%> (-3.53%) ⬇️
server/schedulers/random_merge.go 60.00% <0.00%> (-3.34%) ⬇️
server/election/lease.go 94.91% <0.00%> (-1.70%) ⬇️
server/server.go 70.56% <0.00%> (-1.67%) ⬇️
server/schedulers/utils.go 93.75% <0.00%> (-1.05%) ⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 938948a...eacda0c. Read the comment docs.

@ti-chi-bot ti-chi-bot merged commit 7c8f3a7 into tikv:master Jun 15, 2021
@rleungx rleungx mentioned this pull request Jun 17, 2021
Closed
bufferflies pushed a commit to bufferflies/pd that referenced this pull request Jul 9, 2021
@AndreMouche @ti-chi-bot @bufferflies
client/base_client: do tiny refator for the function getMembers (tikv…
aaf8d3d 
…#3777)

* client/base_client: do tiny refator for funtion getMembers

Signed-off-by: shirly <AndreMouche@126.com>

* client/base_client: address comments

Signed-off-by: shirly <AndreMouche@126.com>

Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io>
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

@disksing disksing disksing approved these changes

@lhy1024 lhy1024 lhy1024 approved these changes

@HunDunDM HunDunDM Awaiting requested review from HunDunDM

Assignees

No one assigned

Labels

release-note-none Denotes a PR that doesn't merit a release note. 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.

5 participants

@AndreMouche @ti-chi-bot @disksing @rleungx @lhy1024