Implement TSO Client and provide general client side service discovery framework and general gPRC stream handling framework. #6037

binshi-bing · 2023-02-22T09:24:50Z

What problem does this PR solve?

Refactor pd client with template design pattern to provide general client side service discovery framework and general gPRC stream batching, forwarding, async and pooling framework.

Issue Number: Ref #5836

What is changed and how does it work?

Changes:
1. Define the interface BaseClient which is generally for service discovery on a quorum-based cluster or a primary/secondy configured cluster so that the grpc client logic layer can decouple from the server discovery layer.
2. Rename baseClient to pdBaseClient then refactor it to implements BaseClient interface. It provides a basic implementation of service discovery on a quorum-based cluster.
3. Refactor pd client with template design patter to provide general gPRC stream batching, forwarding, async and pooling framework.
4. Add skeleton of tsoBaseClient which is a basic implementation of server discover on a primary/secondary configured cluster.

Check List

Tests

Unit test
Manual test

pd-tso-bench testing result:
~/code/pingcap/my-pd/bin   tso-integration ●  ./pd-tso-bench -v -duration 5s -pd "127.0.0.1:3379"  1 ↵  4577  17:46:06
Start benchmark #0, duration: 5s
Create 1 client(s) for benchmark
[2023/02/23 17:46:10.110 -08:00] [INFO] [client.go:431] ["[pd] create pd(tso) client with endpoints"] [pd-address="[127.0.0.1:3379]"]
[2023/02/23 17:46:10.110 -08:00] [INFO] [tso_client.go:336] ["[tso] switch primary"] [new-primary=http://127.0.0.1:3379/] [old-primary=]
[2023/02/23 17:46:10.110 -08:00] [INFO] [client.go:762] ["[pd] tso dispatcher created"] [dc-location=global]
count: 495783, max: 4.3819ms, min: 0.1444ms, avg: 2.0098ms
<1ms: 10410, >1ms: 228832, >2ms: 256541, >5ms: 0, >10ms: 0, >30ms: 0, >50ms: 0, >100ms: 0, >200ms: 0, >400ms: 0, >800ms: 0, >1s: 0
count: 481110, max: 4.6088ms, min: 0.0558ms, avg: 2.0063ms
<1ms: 24825, >1ms: 165334, >2ms: 290951, >5ms: 0, >10ms: 0, >30ms: 0, >50ms: 0, >100ms: 0, >200ms: 0, >400ms: 0, >800ms: 0, >1s: 0
count: 479289, max: 4.3606ms, min: 0.3493ms, avg: 2.0835ms
<1ms: 515, >1ms: 190880, >2ms: 287894, >5ms: 0, >10ms: 0, >30ms: 0, >50ms: 0, >100ms: 0, >200ms: 0, >400ms: 0, >800ms: 0, >1s: 0
count: 478138, max: 4.4927ms, min: 0.3457ms, avg: 2.0902ms
<1ms: 342, >1ms: 185102, >2ms: 292694, >5ms: 0, >10ms: 0, >30ms: 0, >50ms: 0, >100ms: 0, >200ms: 0, >400ms: 0, >800ms: 0, >1s: 0
Total:
count: 1934320, max: 4.6088ms, min: 0.0558ms, avg: 2.0471ms
<1ms: 36092, >1ms: 770148, >2ms: 1128080, >5ms: 0, >10ms: 0, >30ms: 0, >50ms: 0, >100ms: 0, >200ms: 0, >400ms: 0, >800ms: 0, >1s: 0
count: 1934320, <1ms: 1.87%, >1ms: 39.81%, >2ms: 58.32%, >5ms: 0.00%, >10ms: 0.00%, >30ms: 0.00%, >50ms: 0.00%, >100ms: 0.00%, >200ms: 0.00%, >400ms: 0.00%, >800ms: 0.00%, >1s: 0.00%
P0.5: 2.0442ms, P0.8: 2.2536ms, P0.9: 2.4216ms, P0.99: 3.1541ms

Remaining issues:
Killing etcd (remote)backend and the primary in sequence, the secondary doesn't switch to primary. This is because leadership.Watch() in the secondary is stuck. This seems to be a bug in monolithic architecture but uncovered by microservice (detached etcd) scenario.
pd(tso)-client exit isn't clean.

Release note

None.

ti-chi-bot · 2023-02-22T09:24:52Z

[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-22T09:25:00Z

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.

JmPotato · 2023-02-22T09:31:11Z

client/base_client.go

+	// GetTSOAllocators returns {dc-location -> TSO allocator leader URL} connection map
+	GetTSOAllocators() *sync.Map
+	// GetTSOAllocatorLeaderAddrByDCLocation returns the tso allocator of the given dcLocation
+	GetTSOAllocatorLeaderAddrByDCLocation(dcLocation string) (string, bool)
+	// GetTSOAllocatorClientConnByDCLocation returns the tso allocator grpc client connection
+	// of the given dcLocation
+	GetTSOAllocatorClientConnByDCLocation(dcLocation string) (*grpc.ClientConn, string)


As a base client interface, could we only provide some general methods to build up the specified client upon it? Introducing TSO-related methods here does not sound good to me.

We should move TSO-related methods out, but we can do it incrementally. The reason why we have TSO-related methods here is that previous baseClient(pdBaseClient) mixed up service discovery and tso related service discovery logic. There is historical reason, now we have to refactor it step by step. @JmPotato

Ok, if the final goal is like that, then this makes sense to me.

BTW, I'll move CreateTsoStream, ProcessTSORequests and the related private functions to another interface. @JmPotato

Moved to tso_stream. Template, Abstract Factor, Builder design patterns are used to make it possible.

client/base_client.go

client/tso_client.go

client/base_client.go

codecov · 2023-02-25T01:46:57Z

Codecov Report

Base: 74.20% // Head: 74.00% // Decreases project coverage by -0.21% ⚠️

Coverage data is based on head (b7005e2) compared to base (8b82aa9).
Patch coverage: 67.50% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6037      +/-   ##
==========================================
- Coverage   74.20%   74.00%   -0.21%     
==========================================
  Files         373      376       +3     
  Lines       37354    37501     +147     
==========================================
+ Hits        27718    27751      +33     
- Misses       7197     7314     +117     
+ Partials     2439     2436       -3

Flag	Coverage Δ
unittests	`74.00% <67.50%> (-0.21%)`	⬇️

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

Impacted Files	Coverage Δ
client/tso_client.go	`0.00% <0.00%> (ø)`
client/resource_manager_client.go	`66.84% <25.00%> (ø)`
client/tso_stream.go	`47.88% <47.88%> (ø)`
client/keyspace_client.go	`63.63% <50.00%> (ø)`
client/tso_batch_controller.go	`59.25% <59.25%> (ø)`
client/grpcutil/grpcutil.go	`73.33% <64.70%> (-11.29%)`	⬇️
client/base_client.go	`85.10% <82.05%> (+1.92%)`	⬆️
client/tso_request_dispatcher.go	`82.18% <82.18%> (ø)`
client/client.go	`63.67% <82.25%> (-7.09%)`	⬇️
pkg/utils/metricutil/metricutil.go	`62.06% <0.00%> (-20.69%)`	⬇️
... and 21 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.

rleungx

Mostly, LGTM.

client/grpcutil/grpcutil.go

client/base_client.go

lhy1024

diff so much, we need to review it carefully

lhy1024 · 2023-02-27T05:25:27Z

client/grpcutil/grpcutil.go

+	if ok {
+		return conn.(*grpc.ClientConn), nil
+	}
+	tc, err := tlsCfg.ToTLSConfig()


tc is usually used to be test cluster in this repo, I think tls is a better name for it

tls is the package name and mightn't be appropriate for the config name. tlsCfg is already used for tlsCfg *tlsutil.TLSConfig passed to this function as func param. I changed the name to tlsConfig.

client/base_client.go

client/tso_client.go

lhy1024 · 2023-02-27T06:44:24Z

client/tso_request_dispatcher.go

+	for {
+		// the pd/allocator leader change, we need to re-establish the stream
+		if u != url {
+			log.Info("[pd] the leader of the allocator leader is changed", zap.String("dc", dc), zap.String("origin", url), zap.String("new", u))


These functions are used by both tso APIs in pd client and tso APIs in the independent tso client. I changed name to [pd/tso]

Also changed every [pd] to [pd/tso] in this file.

client/tso_request_dispatcher.go

lhy1024 · 2023-02-27T06:51:56Z

client/tso_stream.go

+	stream tsopb.TSO_TsoClient
+}
+
+func (s *tsoTSOStream) processRequests(clusterID uint64, dcLocation string, requests []*tsoRequest,


this two function is so similiar

It's not easy to remove duplicate code now, because the request in stream.Send(), the stream, the response from stream.Recv() are in different types from pdpb and tsopb. We have used several design patterns, including template, Abstract Factory, Builder as you can see in base_client.go and tso_stream.go, so that we can share the service discovery code and tso batching/forwarding/async framework. These two functions are closest to the protobuf code and only different in types so it is even not possible to dedup further.

client/tso_request_dispatcher.go

lhy1024 · 2023-02-27T06:54:32Z

client/client.go

+		go c.tsLoop()
+		go c.tsCancelLoop()
+	}
+	if enableAdmissionCtl {


what does it mean?

tso mcs mode doesn't need to enable admisstion control module, so this flag is needed.

Do you mean we can have a better name, such as enableAdmissionControl?

I removed the flags for now. The original purpose of adding these flags is to disable some clients when the server side doesn't provide the correspoinding services. For example, disable tso client when the server side is resource management microservice, or disable resource management client when the server side the tso microservice.

client/client.go

Changes: 1. Define the interface BaseClient which is generally for service discovery on a quorum-based cluster or a primary/secondy configured cluster so that the grpc client logic layer can decouple from the server discovery layer. 2. Rename baseClient to pdBaseClient then refactor it to implements BaseClient interface. It provides a basic implementation of service discovery on a quorum-based cluster. 3. Refactor pd client with template design patter to provides general client side service discovery framework and general TSO batching, forwarding, async and pooling framework. 4. Add skeleton of tsoBaseClient which is a basic implementation of server discover on a primary/secondary configured cluster. Signed-off-by: Bin Shi <[email protected]>

Signed-off-by: Bin Shi <[email protected]>

If -m is specified, then talk to tso microservice, pd otherwise. Set -tso to specify tso serving addresses. e.g., ./pd-tso-bench -v -m -duration 5s -tso "127.0.0.1:3379" Signed-off-by: Bin Shi <[email protected]>

AddTSOAllocatorServiceEndpointSwitchedCallback adds callbacks which will be called when any global/local tso allocator service endpoint is switched. Signed-off-by: Bin Shi <[email protected]>

…adability of client.go Signed-off-by: Bin Shi <[email protected]>

…t_dispatcher.go to improve code readability. Signed-off-by: Bin Shi <[email protected]>

…nd move GetOrCreateGRPCConn to grpcutil. Signed-off-by: Bin Shi <[email protected]>

…r refactor the code. Signed-off-by: Bin Shi <[email protected]>

Signed-off-by: Bin Shi <[email protected]>

… client. Signed-off-by: Bin Shi <[email protected]>

lhy1024

mostly LGTM

lhy1024 · 2023-02-27T09:08:46Z

/merge

ti-chi-bot · 2023-02-27T09:08:47Z

@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-27T09:08:49Z

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

Commit hash: 3ca0398

nolouch

looks awesome!

nolouch · 2023-02-27T10:00:02Z

/ok-to-test

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

ti-chi-bot requested review from nolouch and rleungx February 22, 2023 09:25

JmPotato reviewed Feb 22, 2023

View reviewed changes

binshi-bing changed the title ~~tso client impl (part 1)~~ Refactor pd client with template design pattern to provide general client side service discovery framework and general gPRC stream batching, forwarding, async and pooling framework. Feb 22, 2023

binshi-bing mentioned this pull request Feb 22, 2023

Separate TSO into an independent service #5836

Closed

binshi-bing force-pushed the tso-client-impl branch from 5183f8b to dfaf5b4 Compare February 22, 2023 19:30

rleungx reviewed Feb 23, 2023

View reviewed changes

client/base_client.go Show resolved Hide resolved

client/tso_client.go Outdated Show resolved Hide resolved

client/tso_client.go Outdated Show resolved Hide resolved

client/base_client.go Show resolved Hide resolved

binshi-bing force-pushed the tso-client-impl branch 5 times, most recently from 0f9c7a6 to 764c81c Compare February 24, 2023 03:13

binshi-bing changed the title ~~Provide general client side service discovery framework and general gPRC stream handling framework.~~ TSO Client implement and provide general client side service discovery framework and general gPRC stream handling framework. Feb 24, 2023

binshi-bing force-pushed the tso-client-impl branch 2 times, most recently from 353b48e to 06f60fc Compare February 26, 2023 23:21

rleungx approved these changes Feb 27, 2023

View reviewed changes

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

binshi-bing force-pushed the tso-client-impl branch 2 times, most recently from 3f2323c to f0580b8 Compare February 27, 2023 03:53

lhy1024 reviewed Feb 27, 2023

View reviewed changes

client/grpcutil/grpcutil.go Outdated Show resolved Hide resolved

client/base_client.go Outdated Show resolved Hide resolved

client/base_client.go Outdated Show resolved Hide resolved

client/base_client.go Outdated Show resolved Hide resolved

binshi-bing force-pushed the tso-client-impl branch 2 times, most recently from 9eff40c to 8a70da1 Compare February 27, 2023 05:59

lhy1024 reviewed Feb 27, 2023

View reviewed changes

rleungx reviewed Feb 27, 2023

View reviewed changes

client/client.go Outdated Show resolved Hide resolved

binshi-bing added 14 commits February 27, 2023 00:58

Implement tsoBaseClient and add NewTSOClientWithContext()

c212cd4

Signed-off-by: Bin Shi <[email protected]>

Handle feedback

5018c2e

Signed-off-by: Bin Shi <[email protected]>

Fix bugs

15adaff

Signed-off-by: Bin Shi <[email protected]>

Refine PD client start and exit logic

0697700

Signed-off-by: Bin Shi <[email protected]>

Upgrade pd-test-bench to talk to tso microservice

aa7f391

If -m is specified, then talk to tso microservice, pd otherwise. Set -tso to specify tso serving addresses. e.g., ./pd-tso-bench -v -m -duration 5s -tso "127.0.0.1:3379" Signed-off-by: Bin Shi <[email protected]>

Fix unittest failure by adding tsoAllocatorLeaderSwitchedCallback.

f5f2501

AddTSOAllocatorServiceEndpointSwitchedCallback adds callbacks which will be called when any global/local tso allocator service endpoint is switched. Signed-off-by: Bin Shi <[email protected]>

move tsoBatchController to tso_batch_controller.go to improve code re…

fd0de0d

…adability of client.go Signed-off-by: Bin Shi <[email protected]>

Move tso dispatching and stream processing related code to tso_reques…

a23b018

…t_dispatcher.go to improve code readability. Signed-off-by: Bin Shi <[email protected]>

Remove GetTSOAllocatorServingEndpointURLs from BaseClient Interface a…

52295b2

…nd move GetOrCreateGRPCConn to grpcutil. Signed-off-by: Bin Shi <[email protected]>

Add tsoStreamBuilderFactory, tsoStreamBuilder and tsoStream to furthe…

b7dc49a

…r refactor the code. Signed-off-by: Bin Shi <[email protected]>

Handle Hanyang's feedback

85ce900

Signed-off-by: Bin Shi <[email protected]>

handle more feedback

fbca15b

Signed-off-by: Bin Shi <[email protected]>

Remove the flags to enable/disable TSO Client and resource management…

3ca0398

… client. Signed-off-by: Bin Shi <[email protected]>

binshi-bing force-pushed the tso-client-impl branch from c74d478 to 3ca0398 Compare February 27, 2023 09:00

lhy1024 approved these changes Feb 27, 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 27, 2023

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

Merge branch 'master' into tso-client-impl

b7005e2

nolouch reviewed Feb 27, 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 Feb 27, 2023

ti-chi-bot merged commit 6be15a5 into tikv:master Feb 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement TSO Client and provide general client side service discovery framework and general gPRC stream handling framework. #6037

Implement TSO Client and provide general client side service discovery framework and general gPRC stream handling framework. #6037

binshi-bing commented Feb 22, 2023 •

edited

Loading

ti-chi-bot commented Feb 22, 2023 •

edited

Loading

ti-chi-bot commented Feb 22, 2023

JmPotato Feb 22, 2023

binshi-bing Feb 22, 2023 •

edited

Loading

JmPotato Feb 22, 2023

binshi-bing Feb 22, 2023

binshi-bing Feb 27, 2023

codecov bot commented Feb 25, 2023 •

edited

Loading

rleungx left a comment

lhy1024 left a comment

lhy1024 Feb 27, 2023

binshi-bing Feb 27, 2023

lhy1024 Feb 27, 2023

binshi-bing Feb 27, 2023

binshi-bing Feb 27, 2023

lhy1024 Feb 27, 2023

binshi-bing Feb 27, 2023

lhy1024 Feb 27, 2023

binshi-bing Feb 27, 2023

binshi-bing Feb 27, 2023

binshi-bing Feb 27, 2023 •

edited

Loading

lhy1024 left a comment

lhy1024 commented Feb 27, 2023

ti-chi-bot commented Feb 27, 2023

ti-chi-bot commented Feb 27, 2023

nolouch left a comment

nolouch commented Feb 27, 2023

Implement TSO Client and provide general client side service discovery framework and general gPRC stream handling framework. #6037

Implement TSO Client and provide general client side service discovery framework and general gPRC stream handling framework. #6037

Conversation

binshi-bing commented Feb 22, 2023 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot commented Feb 22, 2023 • edited Loading

ti-chi-bot commented Feb 22, 2023

Choose a reason for hiding this comment

binshi-bing Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 25, 2023 • edited Loading

Codecov Report

rleungx left a comment

Choose a reason for hiding this comment

lhy1024 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

binshi-bing Feb 27, 2023 • edited Loading

Choose a reason for hiding this comment

lhy1024 left a comment

Choose a reason for hiding this comment

lhy1024 commented Feb 27, 2023

ti-chi-bot commented Feb 27, 2023

ti-chi-bot commented Feb 27, 2023

nolouch left a comment

Choose a reason for hiding this comment

nolouch commented Feb 27, 2023

binshi-bing commented Feb 22, 2023 •

edited

Loading

ti-chi-bot commented Feb 22, 2023 •

edited

Loading

binshi-bing Feb 22, 2023 •

edited

Loading

codecov bot commented Feb 25, 2023 •

edited

Loading

binshi-bing Feb 27, 2023 •

edited

Loading