domain: add retry on pd failure for InfoSyncer (fix #61132) #61236

NaturezzZ · 2025-05-21T08:10:57Z

What problem does this PR solve?

Issue Number: close #61132

Problem Summary:

Drop database fail on pd transient unavaible response.

create database test_1;
use test_1;
create table t (a int);
insert into t values (1);
drop database test_1;

If pd fail on RPC runtime.goexit:1223;net/http.(*conn).serve:2092;net/http.serverHandler.ServeHTTP:3210;go.etcd.io/etcd/server/v3/embed.(*accessController).ServeHTTP:458;net/http.(*ServeMux).ServeHTTP:2747;github.com/urfave/negroni.(*Negroni).ServeHTTP:96;github.com/urfave/negroni.middleware.ServeHTTP:38;github.com/urfave/negroni.(*Recovery).ServeHTTP:193;github.com/urfave/negroni.middleware.ServeHTTP:38;github.com/urfave/negroni.HandlerFunc.ServeHTTP:29;github.com/urfave/negroni.(*Negroni).UseHandler.Wrap.func1:46;github.com/gorilla/mux.(*Router).ServeHTTP:210;github.com/gorilla/mux.(*Router).ServeHTTP:210;github.com/urfave/negroni.(*Negroni).ServeHTTP:96;github.com/urfave/negroni.middleware.ServeHTTP:38;github.com/tikv/pd/pkg/utils/apiutil/serverapi.(*runtimeServiceValidator).ServeHTTP:48;github.com/urfave/negroni.middleware.ServeHTTP:38;github.com/tikv/pd/pkg/utils/apiutil/serverapi.(*redirector).ServeHTTP:187;github.com/urfave/negroni.middleware.ServeHTTP:38;github.com/urfave/negroni.HandlerFunc.ServeHTTP:29;github.com/tikv/pd/server/api.NewHandler.Wrap.func21:46;github.com/gorilla/mux.(*Router).ServeHTTP:210;net/http.HandlerFunc.ServeHTTP:2220;github.com/tikv/pd/server/api.clusterMiddleware.middleware-fm.clusterMiddleware.middleware.func1:101;github.com/urfave/negroni.(*Negroni).ServeHTTP:96;github.com/urfave/negroni.middleware.ServeHTTP:38;github.com/tikv/pd/server/api.(*requestInfoMiddleware).ServeHTTP:78;github.com/urfave/negroni.middleware.ServeHTTP:38;github.com/tikv/pd/server/api.(*auditMiddleware).ServeHTTP:152;github.com/urfave/negroni.middleware.ServeHTTP:38;github.com/tikv/pd/server/api.(*rateLimitMiddleware).ServeHTTP:184;github.com/urfave/negroni.middleware.ServeHTTP:38;github.com/urfave/negroni.HandlerFunc.ServeHTTP:29;github.com/tikv/pd/server/api.(*serviceMiddlewareBuilder).createHandler.WrapFunc.func1:56;github.com/tikv/pd/server/api.(*regionLabelHandler).PatchRegionLabelRules:71;github.com/tikv/pd/pkg/schedule/labeler.(*RegionLabeler).Patch:302;github.com/tikv/pd/pkg/storage/endpoint.RunBatchOpInTxn:94;github.com/tikv/pd/pkg/storage/kv.(*etcdKVBase).RunInTxn:215;github.com/tikv/pd/pkg/storage/kv.(*etcdTxn).commit:287;github.com/tikv/pd/pkg/storage/kv.(*SlowLogTxn).Commit:171;go.etcd.io/etcd/client/v3.(*txn).Commit:145;go.etcd.io/etcd/client/v3.(*retryKVClient).Txn:117;go.etcd.io/etcd/api/v3/etcdserverpb.(*kVClient).Txn:6487 ,
drop database does not work.

What changed and how does it work?

Add retry policy on pdHTTPCli.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

This PR has been tested manually by injecting a RPC failure between tidb and pd, and the result shows that this PR has fixed #61132. We did this by instrumenting the RPC framework and differential testing. The code base is a little bit large and maybe not very easy to integrate with existing tidb testing codes. We plan to open-source our testing tool months later.

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

Fix the issue that pd transient error breaks tidb dropping database.

ti-chi-bot · 2025-05-21T08:11:07Z

Hi @NaturezzZ. Thanks for your PR.

I'm waiting for a pingcap 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-sigs/prow repository.

tiprow · 2025-05-21T08:11:16Z

Hi @NaturezzZ. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

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-sigs/prow repository.

lance6716 · 2025-05-22T14:04:46Z

/ok-to-test

lance6716

/cc @okJiang @JmPotato

pkg/domain/infosync/info.go

NaturezzZ · 2025-05-22T14:18:42Z

/retest

codecov · 2025-05-22T14:34:39Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.0323%. Comparing base (00efb5f) to head (cc41c62).
Report is 33 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #61236        +/-   ##
================================================
+ Coverage   73.1411%   75.0323%   +1.8911%     
================================================
  Files          1726       1748        +22     
  Lines        478040     489977     +11937     
================================================
+ Hits         349644     367641     +17997     
+ Misses       106943      99412      -7531     
- Partials      21453      22924      +1471

Flag	Coverage Δ
integration	`48.6955% <0.0000%> (?)`
unit	`72.4401% <100.0000%> (+0.0601%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.7804% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`61.8304% <ø> (+14.2825%)`	⬆️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

NaturezzZ · 2025-05-22T14:53:55Z

/retest

NaturezzZ · 2025-05-22T15:15:07Z

/retest

NaturezzZ · 2025-05-24T06:09:42Z

Hi @lance6716, I removed dependency on the pdutil package. Is it okay now?

ti-chi-bot · 2025-05-24T08:02:16Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lance6716
Once this PR has been reviewed and has the lgtm label, please assign d3hunter for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

pkg/domain/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2025-05-24T08:02:18Z

[LGTM Timeline notifier]

Timeline:

2025-05-24 08:02:18.545892495 +0000 UTC m=+83268.917679956: ☑️ agreed by lance6716.

NaturezzZ · 2025-05-24T08:04:47Z

Hi @D3Hunter, would you mind take a look? Thank you very much!

D3Hunter

have you test it manually, does it work for the case in the issue? i don't see UT in your PR, so I un-checked it in the description

D3Hunter · 2025-05-26T02:28:45Z

pkg/domain/infosync/info.go

@@ -241,6 +242,11 @@ type infoschemaMinTS interface {
 	GetAndResetRecentInfoSchemaTS(now uint64) uint64
 }

+const (
+	// InfoSyncerRetryTime is retry time limit for InfoSyncer.
+	InfoSyncerRetryTime = 120


Suggested change

InfoSyncerRetryTime = 120

InfoSyncerRetryTime = 30*time.Second

2 minutes is too long, i prefer to make it fail faster

D3Hunter · 2025-05-26T02:32:44Z

pkg/domain/infosync/info.go

@@ -255,7 +261,8 @@ func GlobalInfoSyncerInit(
 	if pdHTTPCli != nil {
 		pdHTTPCli = pdHTTPCli.
 			WithCallerID("tidb-info-syncer").
-			WithRespHandler(pdResponseHandler)
+			WithRespHandler(pdResponseHandler).
+			WithBackoffer(retry.InitialBackoffer(time.Second, time.Second, InfoSyncerRetryTime*time.Second))


Suggested change

WithBackoffer(retry.InitialBackoffer(time.Second, time.Second, InfoSyncerRetryTime*time.Second))

WithBackoffer(retry.InitialBackoffer(100*time.Millisecond, time.Second, InfoSyncerRetryTime*time.Second))

NaturezzZ · 2025-05-26T03:26:04Z

Hi @D3Hunter, I have tested manually by injecting a RPC failure between tidb and pd, and confirmed that this issue is fixed by this PR. We did this by instrumenting the RPC framework and differential testing. The code base is a little bit large and maybe not very easy to integrate with tidb testing codes. We plan to open-source our testing tool months later.

D3Hunter · 2025-05-26T06:20:55Z

Hi @D3Hunter, I have tested manually by injecting a RPC failure between tidb and pd, and confirmed that this issue is fixed by this PR. We did this by instrumenting the RPC framework and differential testing. The code base is a little bit large and maybe not very easy to integrate with tidb testing codes. We plan to open-source our testing tool months later.

can you update it in the description under Manual test (add detailed scripts or steps below), and also make it checked

fix 61132: add retry on pd failure for InfoSyncer

8993fc5

This was referenced May 21, 2025

Transient failure causes end-to-end failure in SQL execution tikv/pd#9326

Open

TiDB does not handle transient error raised by PD #61132

Open

ti-chi-bot 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 May 22, 2025

lance6716 reviewed May 22, 2025

View reviewed changes

pkg/domain/infosync/info.go Outdated Show resolved Hide resolved

ti-chi-bot bot requested review from JmPotato and okJiang May 22, 2025 14:08

replace br retrytime with local variable

d26c27f

update bazel

cdfbc6c

ti-chi-bot bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 22, 2025

nits

cc41c62

lance6716 approved these changes May 24, 2025

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label May 24, 2025

ti-chi-bot bot added do-not-merge/needs-tests-checked and removed do-not-merge/needs-triage-completed labels May 26, 2025

D3Hunter reviewed May 26, 2025

View reviewed changes

ti-chi-bot bot removed the do-not-merge/needs-tests-checked label May 27, 2025

	InfoSyncerRetryTime = 120
	InfoSyncerRetryTime = 30*time.Second

	WithBackoffer(retry.InitialBackoffer(time.Second, time.Second, InfoSyncerRetryTime*time.Second))
	WithBackoffer(retry.InitialBackoffer(100time.Millisecond, time.Second, InfoSyncerRetryTimetime.Second))

domain: add retry on pd failure for InfoSyncer (fix #61132) #61236

Are you sure you want to change the base?

domain: add retry on pd failure for InfoSyncer (fix #61132) #61236

Conversation

NaturezzZ commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

Uh oh!

ti-chi-bot bot commented May 21, 2025

Uh oh!

tiprow bot commented May 21, 2025

Uh oh!

lance6716 commented May 22, 2025

Uh oh!

lance6716 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NaturezzZ commented May 22, 2025

Uh oh!

codecov bot commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

NaturezzZ commented May 22, 2025

Uh oh!

NaturezzZ commented May 22, 2025

Uh oh!

NaturezzZ commented May 24, 2025

Uh oh!

ti-chi-bot bot commented May 24, 2025

Uh oh!

ti-chi-bot bot commented May 24, 2025

[LGTM Timeline notifier]

Uh oh!

NaturezzZ commented May 24, 2025

Uh oh!

D3Hunter left a comment

Choose a reason for hiding this comment

Uh oh!

D3Hunter May 26, 2025

Choose a reason for hiding this comment

Uh oh!

D3Hunter May 26, 2025

Choose a reason for hiding this comment

Uh oh!

NaturezzZ commented May 26, 2025

Uh oh!

D3Hunter commented May 26, 2025

Uh oh!

Uh oh!

NaturezzZ commented May 21, 2025 •

edited

Loading

codecov bot commented May 22, 2025 •

edited

Loading