Implement tso grpc service include both server side and client side in a basic manner #5986

binshi-bing · 2023-02-14T05:29:57Z

What problem does this PR solve?

This change implements tso grpc service include both server side and client side.
The corresponding kvproto pr is here pingcap/kvproto#1053.

Issue Number: Ref #5836

What is changed and how does it work?

This pr is split from "basic implement tso gPRC service #5949".

This pr only implements Tso() rpc. SyncMaxTS, GetDCLocationInfo, SetExternalTimestamp and GetExternalTimestamp rpcs depend on the global tso allocator and some other related data structures which are tightly coupled with pdpb messages, so they can't be used by tso service before further refactor on these data structures. And this pr #5949 is the version which refactors and reuses server/grpc_server.go to avoid duplicate code as much as possible.

Implement the server side of the tso grpc service.

Check List

Tests

Unit test
Integration test

Code changes

Side effects

Introduced duplicate code. The TSO grpc service implementation doesn't reuse the code in PD grpc service because of two reasons:
1. Some code are code to reuse because they are mixed with pdpb message types.
1. For now, it isn't clear for how to refactor the tso related data structures and logic in the pkg/tso.
1. Minimize the change and refactor in server/grpc_server to avoid introducing bugs into PD path.

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
PR to update pingcap/tiup:
Need to cherry-pick to the release branch

Release note

None

ti-chi-bot · 2023-02-14T05:29:59Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

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

ti-chi-bot · 2023-02-14T05:30:06Z

Hi @binshi-bing. Thanks for your PR.

I'm waiting for a tikv member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 kubernetes/test-infra repository.

pkg/utils/etcdutil/etcdutil.go

pkg/utils/etcdutil/etcdutil_test.go

codecov · 2023-02-14T05:40:24Z

Codecov Report

Base: 75.15% // Head: 75.15% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (e6d3d0a) compared to base (645ceeb).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5986   +/-   ##
=======================================
  Coverage   75.15%   75.15%           
=======================================
  Files         362      363    +1     
  Lines       36316    36322    +6     
=======================================
+ Hits        27292    27297    +5     
  Misses       6655     6655           
- Partials     2369     2370    +1

Flag	Coverage Δ
unittests	`75.15% <0.00%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
client/client.go	`70.75% <ø> (ø)`
client/tso_client.go	`0.00% <0.00%> (ø)`
pkg/mcs/discovery/register.go	`53.48% <0.00%> (-18.61%)`	⬇️
pkg/utils/tempurl/tempurl.go	`45.00% <0.00%> (-15.00%)`	⬇️
pkg/election/leadership.go	`75.25% <0.00%> (-5.16%)`	⬇️
pkg/utils/etcdutil/etcdutil.go	`82.17% <0.00%> (-2.33%)`	⬇️
pkg/dashboard/adapter/manager.go	`79.31% <0.00%> (-2.30%)`	⬇️
server/schedule/labeler/rules.go	`87.50% <0.00%> (-2.28%)`	⬇️
server/region_syncer/client.go	`85.82% <0.00%> (-2.24%)`	⬇️
pkg/encryption/key_manager.go	`70.11% <0.00%> (-1.60%)`	⬇️
... and 16 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

pkg/mcs/tso/server/grpc_service.go

pkg/mcs/tso/server/metrics.go

pkg/mcs/tso/server/server.go

client/tso_client.go

pkg/mcs/tso/server/server.go

Signed-off-by: Bin Shi <binshi.bing@gmail.com>

Remove the rpcs SyncMaxTS, GetDCLocationInfo, SetExternalTimestamp and GetExternalTimestamp for now. These rpcs depend on the global tso allocator and some other related data structures which depend on pdpb messages, so they can't be used by tso service before futher refactor on these data structures. Signed-off-by: Bin Shi <binshi.bing@gmail.com>

Signed-off-by: Bin Shi <binshi.bing@gmail.com>

TODO: Refactor and share the TSO streaming framework in the PD client. The implementation in this change is in a very basic manner and only for testing and integration purpose -- no batching, no async, no pooling, no forwarding, no retry and no deliberate error handling. Signed-off-by: Bin Shi <binshi.bing@gmail.com>

Signed-off-by: Bin Shi <binshi.bing@gmail.com>

.golangci.yml

client/tso_client.go

Signed-off-by: Bin Shi <binshi.bing@gmail.com>

rleungx

Mostly, LGTM

…tLocalTSWithinKeyspaceAsync() in TSOClient. Remove the basic implementation of GetTSWithinKeyspace for now and will integrate TSOClient with PDClient's TSO batching/forwarding/async/pooling framework in the next few prs. Signed-off-by: Bin Shi <binshi.bing@gmail.com>

client/go.mod

go.mod

tests/client/go.mod

tests/mcs/go.mod

tools/pd-tso-bench/go.mod

Signed-off-by: Bin Shi <binshi.bing@gmail.com>

lhy1024

we need more tests and remove duplicate code later

lhy1024 · 2023-02-16T07:06:47Z

/merge

ti-chi-bot · 2023-02-16T07:06:48Z

@lhy1024: 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.

ti-chi-bot · 2023-02-16T07:06:50Z

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

Commit hash: 4228db8

lhy1024 · 2023-02-16T08:43:09Z

/ok-to-test

ti-chi-bot added release-note-none needs-ok-to-test labels Feb 14, 2023

ti-chi-bot requested review from disksing and rleungx February 14, 2023 05:30

binshi-bing commented Feb 14, 2023

View reviewed changes

pkg/utils/etcdutil/etcdutil.go Outdated Show resolved Hide resolved

binshi-bing commented Feb 14, 2023

View reviewed changes

pkg/utils/etcdutil/etcdutil_test.go Outdated Show resolved Hide resolved

binshi-bing force-pushed the implement-tso-service branch from 749e777 to 9686407 Compare February 14, 2023 05:54

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

binshi-bing force-pushed the implement-tso-service branch from 9686407 to f337514 Compare February 14, 2023 06:45

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

JmPotato reviewed Feb 14, 2023

View reviewed changes

pkg/mcs/tso/server/grpc_service.go Outdated Show resolved Hide resolved

pkg/mcs/tso/server/metrics.go Outdated Show resolved Hide resolved

pkg/mcs/tso/server/server.go Outdated Show resolved Hide resolved

binshi-bing mentioned this pull request Feb 14, 2023

Separate TSO into an independent service #5836

Closed

binshi-bing force-pushed the implement-tso-service branch from f337514 to 6444e71 Compare February 14, 2023 17:13

binshi-bing changed the title ~~Implement tso grpc service include both server side and client side~~ Implement tso grpc service include both server side and client side in a basic manner Feb 15, 2023

binshi-bing force-pushed the implement-tso-service branch 3 times, most recently from 9006faf to 546b7c9 Compare February 15, 2023 07:25

rleungx reviewed Feb 15, 2023

View reviewed changes

client/tso_client.go Outdated Show resolved Hide resolved

pkg/mcs/tso/server/server.go Show resolved Hide resolved

pkg/mcs/tso/server/server.go Show resolved Hide resolved

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

binshi-bing force-pushed the implement-tso-service branch from 546b7c9 to f3bef7e Compare February 15, 2023 17:37

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

binshi-bing force-pushed the implement-tso-service branch 2 times, most recently from d198441 to 1ac5635 Compare February 15, 2023 22:46

binshi-bing added 4 commits February 15, 2023 21:03

implement the server side of the tso grpc service

9a38f00

Signed-off-by: Bin Shi <binshi.bing@gmail.com>

Address feedbacks

954b3cc

Signed-off-by: Bin Shi <binshi.bing@gmail.com>

Handle feedbacks

6e446c9

Signed-off-by: Bin Shi <binshi.bing@gmail.com>

binshi-bing force-pushed the implement-tso-service branch from 1ac5635 to 6e446c9 Compare February 16, 2023 05:04

lhy1024 reviewed Feb 16, 2023

View reviewed changes

.golangci.yml Outdated Show resolved Hide resolved

client/tso_client.go Show resolved Hide resolved

handle feedback. revert the lint check made before.

7a3b61f

Signed-off-by: Bin Shi <binshi.bing@gmail.com>

rleungx approved these changes Feb 16, 2023

View reviewed changes

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

lhy1024 reviewed Feb 16, 2023

View reviewed changes

client/go.mod Outdated Show resolved Hide resolved

go.mod Outdated Show resolved Hide resolved

tests/client/go.mod Outdated Show resolved Hide resolved

tests/mcs/go.mod Outdated Show resolved Hide resolved

tools/pd-tso-bench/go.mod Outdated Show resolved Hide resolved

go get github.com/pingcap/kvproto@master

4228db8

Signed-off-by: Bin Shi <binshi.bing@gmail.com>

binshi-bing force-pushed the implement-tso-service branch from b905264 to 4228db8 Compare February 16, 2023 06:56

lhy1024 reviewed Feb 16, 2023

View reviewed changes

lhy1024 approved these changes Feb 16, 2023

View reviewed changes

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 Feb 16, 2023

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

ti-chi-bot added ok-to-test and removed needs-ok-to-test labels Feb 16, 2023

Merge branch 'master' into implement-tso-service

e6d3d0a

ti-chi-bot merged commit 5302e81 into tikv:master Feb 16, 2023

binshi-bing deleted the implement-tso-service branch May 25, 2023 19:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement tso grpc service include both server side and client side in a basic manner #5986

Implement tso grpc service include both server side and client side in a basic manner #5986

binshi-bing commented Feb 14, 2023 •

edited

ti-chi-bot commented Feb 14, 2023 •

edited

ti-chi-bot commented Feb 14, 2023

codecov bot commented Feb 14, 2023 •

edited

rleungx left a comment

lhy1024 left a comment

lhy1024 commented Feb 16, 2023

ti-chi-bot commented Feb 16, 2023

ti-chi-bot commented Feb 16, 2023

lhy1024 commented Feb 16, 2023

Implement tso grpc service include both server side and client side in a basic manner #5986

Implement tso grpc service include both server side and client side in a basic manner #5986

Conversation

binshi-bing commented Feb 14, 2023 • edited

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot commented Feb 14, 2023 • edited

ti-chi-bot commented Feb 14, 2023

codecov bot commented Feb 14, 2023 • edited

Codecov Report

rleungx left a comment

Choose a reason for hiding this comment

lhy1024 left a comment

Choose a reason for hiding this comment

lhy1024 commented Feb 16, 2023

ti-chi-bot commented Feb 16, 2023

ti-chi-bot commented Feb 16, 2023

lhy1024 commented Feb 16, 2023

binshi-bing commented Feb 14, 2023 •

edited

ti-chi-bot commented Feb 14, 2023 •

edited

codecov bot commented Feb 14, 2023 •

edited