client: add backoff mechanism for gRPC client #7863

CabinfeverB · 2024-03-01T02:14:47Z

What problem does this PR solve?

Issue Number: Close #8047

What is changed and how does it work?

introduce the RPCClinet interface
introduce the BakcofferClinet
the default strategy of BakcoffClinet is that it will unlimitedly retry on leader change error.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Code changes

Has the configuration change
Has HTTP API interfaces changed (Don't forget to add the declarative for the new API)
Has persistent data change

Side effects

Possible performance regression
Increased code complexity
Breaking backward compatibility

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.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot · 2024-03-01T02:14:50Z

[REVIEW NOTIFICATION]

This pull request has not been approved.

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 · 2024-03-01T02:14:50Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

codecov · 2024-04-01T10:24:21Z

Codecov Report

Merging #7863 (9b95c1c) into master (2b8116e) will decrease coverage by 0.40%.
Report is 12 commits behind head on master.
The diff coverage is 7.60%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7863      +/-   ##
==========================================
- Coverage   77.31%   76.91%   -0.40%     
==========================================
  Files         468      469       +1     
  Lines       60867    61221     +354     
==========================================
+ Hits        47058    47088      +30     
- Misses      10273    10595     +322     
- Partials     3536     3538       +2

Flag	Coverage Δ
unittests	`76.91% <7.60%> (-0.40%)`	⬇️

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

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot · 2024-04-09T08:40:17Z

PR needs rebase.

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.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

JmPotato · 2024-04-11T06:00:57Z

client/backoffer_client.go

+	if err == errs.ErrClientTSOStreamClosed {
+		return true
+	}
+	errMsg := err.Error()
+	return strings.Contains(errMsg, errs.NotLeaderErr) ||
+		strings.Contains(errMsg, errs.MismatchLeaderErr) ||
+		strings.Contains(errMsg, errs.NotServedErr)


Better unify the checking method in the same way.

JmPotato · 2024-04-11T06:06:31Z

pkg/mcs/tso/server/grpc_service.go

@@ -100,7 +100,7 @@ func (s *Service) Tso(stream tsopb.TSO_TsoServer) error {
 		start := time.Now()
 		// TSO uses leader lease to determine validity. No need to check leader here.
 		if s.IsClosed() {
-			return status.Errorf(codes.Unknown, "server not started")
+			return ErrNotStarted


This will change codes.Unknown to codes.Unavailable, perhaps it is necessary to evaluate whether there are side effects.

JmPotato · 2024-04-11T06:06:36Z

server/grpc_service.go

@@ -568,7 +568,7 @@ func (s *GrpcServer) Tso(stream pdpb.PD_TsoServer) error {
 		start := time.Now()
 		// TSO uses leader lease to determine validity. No need to check leader here.
 		if s.IsClosed() {
-			return status.Errorf(codes.Unknown, "server not started")
+			return ErrNotStarted


JmPotato · 2024-04-11T06:13:48Z

client/client.go

+		if err := req.bo.ExecWithoutReset(ctx, func() error {
+			return c.dispatchTSORequestWithFastRetry(req)
+		}); err != nil {
+			req.tryDone(err)


This retry might be meaningless if the req.tryDone(err) has been called elsewhere before retrying. We might need to drain it first.

HuSharp · 2024-04-11T08:31:58Z

client/client.go

@@ -154,6 +144,25 @@ type Client interface {
 	GCClient
 	// ResourceManagerClient manages resource group metadata and token assignment.
 	ResourceManagerClient
+}
+
+// Client is a PD (Placement Driver) RPC client.


Suggested change

// Client is a PD (Placement Driver) RPC client.

// Client is a PD (Placement Driver) client.

HuSharp · 2024-04-11T08:41:13Z

client/backoffer_client.go

+
+// baseBackoffClient is a base RPCClient that retries requests using the given backoffer.
+// It does not support backoff for GetTSAsync and GetLocalTSAsync.
+// baseBackoffClient is mostly used for mock PD client.


what does for mock PD client. mean? can you show a scenario? :)

ti-chi-bot · 2024-04-12T11:44:14Z

PR needs rebase.

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.

CabinfeverB · 2024-04-22T08:26:03Z

It's work that needs to be done, but I don't have time to finish it

first commit for backoff client

b3741b5

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot bot added do-not-merge/needs-linked-issue release-note-none do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Mar 1, 2024

ti-chi-bot bot requested review from rleungx and Yisaer March 1, 2024 02:14

ti-chi-bot bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 1, 2024

CabinfeverB added 6 commits March 7, 2024 14:40

merge master

c3bed08

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

update

9fbad74

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

merge master

e028dc0

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

impl baseBackoffClient for mock client

8557ee9

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

fix

3b421ee

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

modify default backoff strategy

f54d096

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

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

merge master

6216f3f

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

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

fix

659a0c6

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

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

CabinfeverB added 2 commits April 10, 2024 11:15

merge master

c14b221

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

merge master

721520d

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

CabinfeverB marked this pull request as ready for review April 10, 2024 03:19

ti-chi-bot bot removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/needs-linked-issue labels Apr 10, 2024

CabinfeverB removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 10, 2024

fix test

9b95c1c

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

JmPotato removed the request for review from Yisaer April 11, 2024 05:55

JmPotato requested review from JmPotato and HuSharp April 11, 2024 05:55

JmPotato reviewed Apr 11, 2024

View reviewed changes

HuSharp reviewed Apr 11, 2024

View reviewed changes

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

CabinfeverB closed this Apr 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: add backoff mechanism for gRPC client #7863

client: add backoff mechanism for gRPC client #7863

CabinfeverB commented Mar 1, 2024 •

edited

ti-chi-bot bot commented Mar 1, 2024

ti-chi-bot bot commented Mar 1, 2024

codecov bot commented Apr 1, 2024 •

edited

ti-chi-bot bot commented Apr 9, 2024

JmPotato Apr 11, 2024

JmPotato Apr 11, 2024

JmPotato Apr 11, 2024

JmPotato Apr 11, 2024

HuSharp Apr 11, 2024

HuSharp Apr 11, 2024

ti-chi-bot bot commented Apr 12, 2024

CabinfeverB commented Apr 22, 2024

	// Client is a PD (Placement Driver) RPC client.
	// Client is a PD (Placement Driver) client.

client: add backoff mechanism for gRPC client #7863

client: add backoff mechanism for gRPC client #7863

Conversation

CabinfeverB commented Mar 1, 2024 • edited

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Mar 1, 2024

ti-chi-bot bot commented Mar 1, 2024

codecov bot commented Apr 1, 2024 • edited

Codecov Report

ti-chi-bot bot commented Apr 9, 2024

JmPotato Apr 11, 2024

Choose a reason for hiding this comment

JmPotato Apr 11, 2024

Choose a reason for hiding this comment

JmPotato Apr 11, 2024

Choose a reason for hiding this comment

JmPotato Apr 11, 2024

Choose a reason for hiding this comment

HuSharp Apr 11, 2024

Choose a reason for hiding this comment

HuSharp Apr 11, 2024

Choose a reason for hiding this comment

ti-chi-bot bot commented Apr 12, 2024

CabinfeverB commented Apr 22, 2024

CabinfeverB commented Mar 1, 2024 •

edited

codecov bot commented Apr 1, 2024 •

edited