-
Notifications
You must be signed in to change notification settings - Fork 288
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
capture(ticdc): fix the problem that openapi is blocked when pd is abnormal #4788
capture(ticdc): fix the problem that openapi is blocked when pd is abnormal #4788
Conversation
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
9ec5aba
to
095421b
Compare
/run-all-tests |
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. @@ Coverage Diff @@
## master #4788 +/- ##
================================================
+ Coverage 55.6402% 56.0149% +0.3746%
================================================
Files 494 524 +30
Lines 61283 65886 +4603
================================================
+ Hits 34098 36906 +2808
- Misses 23750 25379 +1629
- Partials 3435 3601 +166 |
@@ -118,9 +118,17 @@ func NewCapture4Test(o owner.Owner) *Capture { | |||
} | |||
|
|||
func (c *Capture) reset(ctx context.Context) error { | |||
conf := config.GetGlobalServerConfig() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need ut cover the change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, PTAL
d2a4eda
to
206852d
Compare
206852d
to
ac9775c
Compare
/run-all-tests |
1 similar comment
/run-all-tests |
a776844
to
128e768
Compare
/run-all-tests |
933f9a0
to
cec8e43
Compare
pkg/etcd/client.go
Outdated
@@ -149,8 +158,10 @@ func (c *Client) Txn(ctx context.Context, cmps []clientv3.Cmp, opsThen, opsElse | |||
// Grant delegates request to clientv3.Lease.Grant | |||
func (c *Client) Grant(ctx context.Context, ttl int64) (resp *clientv3.LeaseGrantResponse, err error) { | |||
err = retryRPC(EtcdGrant, c.metrics[EtcdGrant], func() error { | |||
grantCtx, cancel := context.WithTimeout(ctx, etcdClientTimeoutWithoutRetry) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use etcdClientTimeoutWithRetry
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to ensure that it returns in a reasonable amount of time, otherwise the operation may never retry due to blocking.
pkg/etcd/client.go
Outdated
@@ -181,8 +192,10 @@ func isRetryableError(rpcName string) retry.IsRetryable { | |||
// Revoke delegates request to clientv3.Lease.Revoke | |||
func (c *Client) Revoke(ctx context.Context, id clientv3.LeaseID) (resp *clientv3.LeaseRevokeResponse, err error) { | |||
err = retryRPC(EtcdRevoke, c.metrics[EtcdRevoke], func() error { | |||
revokeCtx, cancel := context.WithTimeout(ctx, etcdClientTimeoutWithoutRetry) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
pkg/etcd/client.go
Outdated
@@ -191,8 +204,10 @@ func (c *Client) Revoke(ctx context.Context, id clientv3.LeaseID) (resp *clientv | |||
// TimeToLive delegates request to clientv3.Lease.TimeToLive | |||
func (c *Client) TimeToLive(ctx context.Context, lease clientv3.LeaseID, opts ...clientv3.LeaseOption) (resp *clientv3.LeaseTimeToLiveResponse, err error) { | |||
err = retryRPC(EtcdRevoke, c.metrics[EtcdRevoke], func() error { | |||
timeToLiveCtx, cancel := context.WithTimeout(ctx, etcdClientTimeoutWithoutRetry) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
cdc/capture/main_test.go
Outdated
goleak.IgnoreTopFunction("google.golang.org/grpc.(*ccBalancerWrapper).watcher"), | ||
goleak.IgnoreTopFunction("google.golang.org/grpc.(*addrConn).resetTransport"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why didn't the gRPC goroutines exit in test case?
cdc/capture/capture.go
Outdated
@@ -605,6 +606,8 @@ func (c *Capture) WriteDebugInfo(ctx context.Context, w io.Writer) { | |||
if c.processorManager != nil { | |||
fmt.Fprintf(w, "\n\n*** processors info ***:\n\n") | |||
c.processorManager.WriteDebugInfo(ctx, w, doneM) | |||
} else { | |||
close(doneM) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an existing bug, how can it be reproduced?
43b8403
to
cb3035e
Compare
Signed-off-by: ti-chi-bot <[email protected]>
In response to a cherrypick label: new pull request created: #5109. |
Signed-off-by: ti-chi-bot <[email protected]>
In response to a cherrypick label: new pull request created: #5110. |
Signed-off-by: ti-chi-bot <[email protected]>
In response to a cherrypick label: new pull request created: #5111. |
Signed-off-by: ti-chi-bot <[email protected]>
In response to a cherrypick label: new pull request created: #5112. |
Signed-off-by: ti-chi-bot <[email protected]>
In response to a cherrypick label: new pull request created: #5113. |
Signed-off-by: ti-chi-bot <[email protected]>
In response to a cherrypick label: new pull request created: #5114. |
Signed-off-by: ti-chi-bot <[email protected]>
What problem does this PR solve?
Issue Number: close #4778
What is changed and how it works?
(*openApi).forwardToOwner
Check List
Tests
Related changes
Release note