Support tso microservice auto service discovery in the pd client. #6065

binshi-bing · 2023-03-01T00:30:58Z

What problem does this PR solve?

Issue Number: Ref #5836

What is changed and how does it work?

Support tso microservice auto service discovery in the pd client.

Check List

Tests

Unit test
Manual test (add detailed scripts or steps below)
~/code/pingcap/my-pd/bin   tso-client-svc-discovery  ./pd-tso-bench -v -m -duration 10s -pd "127.0.0.1:2379"  ✔  5354  16:29:52

Start benchmark #0, duration: 10s
Create 1 client(s) for benchmark
[2023/02/28 16:29:56.663 -08:00] [INFO] [client.go:306] ["[pd(tso)] create tso client with endpoints"] [pd(api)-address="[127.0.0.1:2379]"]
[2023/02/28 16:29:56.665 -08:00] [INFO] [base_client.go:477] ["[pd] switch leader"] [new-leader=http://127.0.0.1:2379] [old-leader=]
[2023/02/28 16:29:56.665 -08:00] [INFO] [base_client.go:153] ["[pd] init cluster id"] [cluster-id=7203525516143219003]
[2023/02/28 16:29:56.666 -08:00] [INFO] [tso_client.go:353] ["[pd(tso)] switch primary"] [new-primary=http://127.0.0.1:3380] [old-primary=]
[2023/02/28 16:29:56.666 -08:00] [INFO] [tso_request_dispatcher.go:287] ["[pd/tso] tso dispatcher created"] [dc-location=global]

count: 498504, max: 4.2795ms, min: 0.3284ms, avg: 2.0016ms
<1ms: 684, >1ms: 271848, >2ms: 225972, >5ms: 0, >10ms: 0, >30ms: 0, >50ms: 0, >100ms: 0, >200ms: 0, >400ms: 0, >800ms: 0, >1s: 0
count: 494935, max: 3.9645ms, min: 0.3201ms, avg: 2.0182ms
<1ms: 562, >1ms: 257241, >2ms: 237132, >5ms: 0, >10ms: 0, >30ms: 0, >50ms: 0, >100ms: 0, >200ms: 0, >400ms: 0, >800ms: 0, >1s: 0

Release note

None.

ti-chi-bot · 2023-03-01T00:31:00Z

[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-03-01T00:31:08Z

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.

rleungx · 2023-03-01T08:40:50Z

client/tso_client.go

+	// tsoKeyspaceGroupPrimaryElectionPrefix defines the key prefix for keyspace group primary election.
+	// The entire key is in the format of "/pd/<cluster-id>/microservice/tso/keyspace-group-XXXXX/primary" in which
+	// XXXXX is 5 digits integer with leading zeros. For now we use 0 as the default cluster id.
+	tsoKeyspaceGroupPrimaryElectionPrefix = "/pd/0/microservice/tso/keyspace-group-"


I noticed that there is a comment left previously. See #6008 (comment)

Yes, we can certainly move "keyspace-group-" from the path to make it more compact. I'll make the change.

rleungx · 2023-03-01T08:43:08Z

client/tso_client.go

+	if err := proto.Unmarshal(value, member); err != nil {
+		return errs.ErrClientProtoUnmarshal.Wrap(err).GenWithStackByCause()
+	}
+	return c.switchPrimary(addrsToUrls([]string{member.Name}))


member.Name?

That's a hack by reusing pdpb.Member and the Name field as service endpoint. My next pr will introduce a new struct as the primary value type as we discussed before.

lhy1024 · 2023-03-01T12:19:41Z

/ok-to-test

lhy1024 · 2023-03-01T12:28:27Z

client/client.go

-func NewTSOClientWithContext(ctx context.Context, svrAddrs []string, security SecurityOption, opts ...ClientOption) (Client, error) {
-	log.Info("[pd(tso)] create tso client with endpoints", zap.Strings("tso-address", svrAddrs))
+// TODO:
+// relace NewClientWithContext with this API after we enable auto service discovery.


Suggested change

// relace NewClientWithContext with this API after we enable auto service discovery.

// replace NewClientWithContext with this API after we enable auto service discovery.

changed locally.

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

lhy1024

mostly LGTM

lhy1024 · 2023-03-03T08:11:36Z

client/client.go

@@ -290,21 +291,30 @@ func NewClientWithContext(ctx context.Context, svrAddrs []string, security Secur
 	c, clientCtx, clientCancel, tlsCfg := createClient(ctx, &security)
 	c.tsoStreamBuilderFactory = &pdTSOStreamBuilderFactory{}
 	c.bc = newPDBaseClient(clientCtx, clientCancel, &c.wg, addrsToUrls(svrAddrs), tlsCfg, c.option)
+	c.tsobc = c.bc


the same two clients?

tsobc is an interface of tso baseclient which can be either tso in pd client or tso microservice client. For the former, tso in pd client is the pd client itself, that's why c.tsobc = c.bc. Actually, I planned one more refactor to make it much clearer -- Client contains pdServiceDiscovery by renaming pdBaseClient after we move all the tso related logic out of pdBaseClient. c.tsobc will be backed by either pdServiceDiscovery or tsoServiceDiscovery. After that, we won't see 'c.tsobc = c.bc' such thing which looks weird at the first sight.

codecov · 2023-03-03T08:19:13Z

Codecov Report

Patch coverage: 34.06% and project coverage change: -0.21 ⚠️

Comparison is base (05228ba) 74.03% compared to head (84494fc) 73.82%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6065      +/-   ##
==========================================
- Coverage   74.03%   73.82%   -0.21%     
==========================================
  Files         383      383              
  Lines       37816    37874      +58     
==========================================
- Hits        27996    27960      -36     
- Misses       7359     7433      +74     
- Partials     2461     2481      +20

Flag	Coverage Δ
unittests	`73.82% <34.06%> (-0.21%)`	⬇️

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

Impacted Files	Coverage Δ
client/errs/errno.go	`100.00% <ø> (ø)`
client/tso_client.go	`0.00% <0.00%> (ø)`
client/client.go	`63.14% <45.45%> (-0.53%)`	⬇️
client/tso_request_dispatcher.go	`82.18% <78.57%> (+0.25%)`	⬆️
client/base_client.go	`85.18% <100.00%> (+0.07%)`	⬆️
pkg/mcs/tso/server/server.go	`44.86% <100.00%> (+0.53%)`	⬆️
tests/mcs/tso/testutil.go	`92.00% <100.00%> (ø)`
pkg/utils/tempurl/tempurl.go	`45.00% <0.00%> (-15.00%)`	⬇️
pkg/storage/endpoint/rule.go	`83.33% <0.00%> (-11.12%)`	⬇️
...erver/config/service_middleware_persist_options.go	`91.66% <0.00%> (-8.34%)`	⬇️
... and 27 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.

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

lhy1024 · 2023-03-03T08:45:49Z

/merge

ti-chi-bot · 2023-03-03T08:45:51Z

@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-03-03T08:45:53Z

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

Commit hash: 84494fc

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Mar 1, 2023

ti-chi-bot requested review from JmPotato and rleungx March 1, 2023 00:31

rleungx reviewed Mar 1, 2023

View reviewed changes

ti-chi-bot added ok-to-test Indicates a PR is ready to be tested. and removed needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Mar 1, 2023

lhy1024 reviewed Mar 1, 2023

View reviewed changes

rleungx mentioned this pull request Mar 2, 2023

Separate TSO into an independent service #5836

Closed

binshi-bing force-pushed the tso-client-svc-discovery branch from f439ff7 to 9c08e78 Compare March 3, 2023 00:51

rleungx approved these changes Mar 3, 2023

View reviewed changes

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

binshi-bing added 2 commits March 3, 2023 00:08

Support tso microservice auto service discovery in the pd client.

0ce2f55

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

Handle feedback.

43a1d93

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

binshi-bing force-pushed the tso-client-svc-discovery branch from 5d2a332 to bce8409 Compare March 3, 2023 08:09

lhy1024 approved these changes Mar 3, 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 Mar 3, 2023

Refactor tso mcs test

84494fc

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

binshi-bing force-pushed the tso-client-svc-discovery branch from bce8409 to 84494fc Compare March 3, 2023 08:27

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

ti-chi-bot merged commit d63eada into tikv:master Mar 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support tso microservice auto service discovery in the pd client. #6065

Support tso microservice auto service discovery in the pd client. #6065

binshi-bing commented Mar 1, 2023

ti-chi-bot commented Mar 1, 2023 •

edited

Loading

ti-chi-bot commented Mar 1, 2023

rleungx Mar 1, 2023

binshi-bing Mar 3, 2023

rleungx Mar 1, 2023

binshi-bing Mar 3, 2023 •

edited

Loading

lhy1024 commented Mar 1, 2023

lhy1024 Mar 1, 2023

binshi-bing Mar 3, 2023

lhy1024 left a comment

lhy1024 Mar 3, 2023

binshi-bing Mar 3, 2023

codecov bot commented Mar 3, 2023 •

edited

Loading

lhy1024 commented Mar 3, 2023

ti-chi-bot commented Mar 3, 2023

ti-chi-bot commented Mar 3, 2023

	// relace NewClientWithContext with this API after we enable auto service discovery.
	// replace NewClientWithContext with this API after we enable auto service discovery.

Support tso microservice auto service discovery in the pd client. #6065

Support tso microservice auto service discovery in the pd client. #6065

Conversation

binshi-bing commented Mar 1, 2023

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot commented Mar 1, 2023 • edited Loading

ti-chi-bot commented Mar 1, 2023

rleungx Mar 1, 2023

Choose a reason for hiding this comment

binshi-bing Mar 3, 2023

Choose a reason for hiding this comment

rleungx Mar 1, 2023

Choose a reason for hiding this comment

binshi-bing Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

lhy1024 commented Mar 1, 2023

lhy1024 Mar 1, 2023

Choose a reason for hiding this comment

binshi-bing Mar 3, 2023

Choose a reason for hiding this comment

lhy1024 left a comment

Choose a reason for hiding this comment

lhy1024 Mar 3, 2023

Choose a reason for hiding this comment

binshi-bing Mar 3, 2023

Choose a reason for hiding this comment

codecov bot commented Mar 3, 2023 • edited Loading

Codecov Report

lhy1024 commented Mar 3, 2023

ti-chi-bot commented Mar 3, 2023

ti-chi-bot commented Mar 3, 2023

ti-chi-bot commented Mar 1, 2023 •

edited

Loading

binshi-bing Mar 3, 2023 •

edited

Loading

codecov bot commented Mar 3, 2023 •

edited

Loading