client: support getting region with buckets #4699
Conversation
Signed-off-by: youjiali1995 <[email protected]>
…uckets Signed-off-by: youjiali1995 <[email protected]>
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
ti-chi-bot
added
the
release-note-none
|
@bufferflies PTAL |
youjiali1995
force-pushed
the
update-kvproto-for-buckets
branch
2 times, most recently
from
870337d to
0f5b1f5
Compare
Signed-off-by: youjiali1995 <[email protected]>
youjiali1995
force-pushed
the
update-kvproto-for-buckets
branch
from
0f5b1f5 to
e6dbfeb
Compare
client/client.go
Outdated
| @@ -76,13 +77,13 @@ type Client interface { | |||
| // taking care of region change. | |||
| // Also it may return nil if PD finds no Region for the key temporarily, | |||
| // client should retry later. | |||
| GetRegion(ctx context.Context, key []byte) (*Region, error) | |||
| GetRegion(ctx context.Context, key []byte, needBuckets bool) (*Region, error) | |||
There was a problem hiding this comment.
Yes. Another way is to add GetRegionAndBucket()/GetPrevRegionAndBucket()/GetRegionAndBucketByID(), but there are many places the Client interface is implemented for a mock client, it's also a breaking change and doesn't reduce the workload. Since the main(maybe the only one) user of PD client is client-go and TiDB, I think we can choose either way to utilize the new interface.
There was a problem hiding this comment.
can we use the option pattern. Or can we put the option into context?
There was a problem hiding this comment.
OK. I will use the option pattern.
|
how about add some unit test for needBuckets is true? |
I don't know how to do it when the server doesn't support buckets now. I depend on you to test it... |
Signed-off-by: youjiali1995 <[email protected]>
youjiali1995
force-pushed
the
update-kvproto-for-buckets
branch
from
722fea3 to
4bf8d46
Compare
…uckets Signed-off-by: youjiali1995 <[email protected]>
Signed-off-by: youjiali1995 <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #4699 +/- ##
==========================================
+ Coverage 74.96% 74.97% +0.01%
==========================================
Files 284 284
Lines 27877 27888 +11
==========================================
+ Hits 20898 20910 +12
- Misses 5106 5113 +7
+ Partials 1873 1865 -8
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
@disksing @bufferflies PTAL again, thanks! |
| go.uber.org/goleak v1.1.11/go.mod h1:cwTWslyiVhfpKIDGSZEM2HlOvcqm+tG4zioyIeLoqMQ= | ||
| go.uber.org/goleak v1.1.12 h1:gZAh5/EyT/HQwlpkCy6wTpqfH9H8Lz8zbm3dZh+OyzA= |
There was a problem hiding this comment.
go mod tidy did it.
|
@AndreMouche: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments. In response to this:
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. |
|
/merge |
|
@nolouch: It seems you want to merge this PR, I will help you trigger all the tests: /run-all-tests 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. |
|
This pull request has been accepted and is ready to merge. Commit hash: 8135954
|
ti-chi-bot
added
the
status/can-merge
What problem does this PR solve?
Issue Number: Close #4698
What is changed and how it works?
Check List
Tests
Code changes
Side effects
Related changes
Release note