etcd/client(ticdc): add retry operation for etcd transaction api (#4248) #4474

CharlesCheung96 · 2022-01-25T07:03:31Z

What problem does this PR solve?

Issue Number: close #4248

What is changed and how it works?

Add retry strategy for etcd transaction, which uniformly handles errors related to etcd transactions and only throws errors that are not retryable, such as context.Canceled, context.DeadlineExceeded, errors.ErrReachMaxTry.
Catch all of the EtcdError in the etcd_worker and retry the Txn operation again to prevent the cdc process from exiting abnormally.
When the connection between cdc and etcd is abnormal, session will be responsible for disconnecting and throwing ErrEtcdSessionDone error, which causes the capture to automatically recover after suicide.

Check List

Tests

Unit test

Code changes

Has exported function/method change
Has exported variable/fields change

Side effects

Possible performance regression
Increased code complexity

Related changes

Need to cherry-pick to the release branch

Release note

Fix a bug that owner exits abnormally when PD leader is killed

ti-chi-bot · 2022-01-25T07:03:32Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

amyangfei
asddongmen

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.

CharlesCheung96 · 2022-01-25T07:07:16Z

/run-all-tests

CharlesCheung96 · 2022-01-25T07:10:36Z

pkg/etcd/client.go

@@ -145,12 +159,31 @@ func isRetryableError(rpcName string) retry.IsRetryable {
 			return false
 		}
 		if rpcName == EtcdRevoke {
-			if etcdErr, ok := err.(rpctypes.EtcdError); ok && etcdErr.Code() == codes.NotFound {
+			if etcdErr, ok := err.(v3rpc.EtcdError); ok && etcdErr.Code() == codes.NotFound {
 				// it means the etcd lease is already expired or revoked
 				return false
 			}
 		}



ref https://github.com/pingcap/tiflow/blob/master/dm/pkg/etcdutil/etcdutil.go#L51-L66 👇

We can extract a function, such as EtcdRetryableError() bool in the future, similar to the work in #3242

pkg/orchestrator/etcd_worker.go

CharlesCheung96 · 2022-01-25T07:46:43Z

/run-all-tests

CharlesCheung96 · 2022-01-25T09:19:18Z

/run-dm-integration-tests
/run-integration-tests

liuzix · 2022-01-25T09:34:35Z

Please add unit tests

pkg/etcd/client.go

amyangfei · 2022-01-28T03:15:52Z

pkg/etcd/client.go

@@ -145,12 +159,31 @@ func isRetryableError(rpcName string) retry.IsRetryable {
 			return false
 		}
 		if rpcName == EtcdRevoke {
-			if etcdErr, ok := err.(rpctypes.EtcdError); ok && etcdErr.Code() == codes.NotFound {
+			if etcdErr, ok := err.(v3rpc.EtcdError); ok && etcdErr.Code() == codes.NotFound {
 				// it means the etcd lease is already expired or revoked
 				return false
 			}
 		}



We can extract a function, such as EtcdRetryableError() bool in the future, similar to the work in #3242

codecov-commenter · 2022-01-28T03:20:34Z

Codecov Report

Merging #4474 (29cac10) into master (9607554) will decrease coverage by 0.0929%.
The diff coverage is 58.2043%.

Flag	Coverage Δ
cdc	`60.3270% <79.7202%> (+0.4047%)`	⬆️
dm	`51.5643% <52.0874%> (-0.4646%)`	⬇️

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

@@               Coverage Diff                @@
##             master      #4474        +/-   ##
================================================
- Coverage   55.6402%   55.5472%   -0.0930%     
================================================
  Files           494        497         +3     
  Lines         61283      61760       +477     
================================================
+ Hits          34098      34306       +208     
- Misses        23750      24023       +273     
+ Partials       3435       3431         -4

amyangfei · 2022-01-28T05:54:57Z

Has the case in #4248 been tested manually

How long will PD recover after kill -9
What is the behavior of TiCDC after PD is killed

…gcap#4248)

ti-chi-bot · 2022-02-08T10:00:13Z

In response to a cherrypick label: new pull request created: #4528.