*: integrate circuitbreaker for get region calls to PD #58737

Tema · 2025-01-07T05:53:23Z

What problem does this PR solve?

Issue Number: close #58780

Problem Summary:

What changed and how does it work?

Introduce a global session variable to control PD Get region call circuitbreaker: https://github.com/tikv/rfcs/blob/master/text/0115-circuit-breaker.md#system-variables

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.

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.

Introduces `tidb_cb_pd_metadata_error_rate_threshold_pct` global system variable which could be used to enable a circuit breaker for PD calls to retrieve region metadata. When a non-zero value is specified, the GetRegion calls to PD will be throttled automatically if error rate exceeds the specified threshold, so that cluster can recover on its own.

ti-chi-bot · 2025-01-07T05:53:33Z

Hi @Tema. 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-01-07T05:53:41Z

Hi @Tema. 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.

ti-chi-bot · 2025-01-07T05:53:54Z

Now you can start all CI jobs with /test all in comment or query the triggers with /test ?

Tema · 2025-01-07T05:54:22Z

PTAL @rleungx
I still need to land tikv/client-go#1543 first and add tests

codecov · 2025-01-07T22:18:25Z

Codecov Report

Attention: Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 74.6179%. Comparing base (a69f863) to head (566bda8).
Report is 2 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #58737        +/-   ##
================================================
+ Coverage   73.1091%   74.6179%   +1.5087%     
================================================
  Files          1689       1735        +46     
  Lines        466728     479488     +12760     
================================================
+ Hits         341221     357784     +16563     
+ Misses       104552      99111      -5441     
- Partials      20955      22593      +1638

Flag	Coverage Δ
integration	`48.4768% <100.0000%> (?)`
unit	`72.2452% <100.0000%> (-0.0540%)`	⬇️

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

Components	Coverage Δ
dumpling	`52.6910% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`61.1039% <ø> (+15.8008%)`	⬆️

Tema · 2025-01-08T01:51:39Z

@rleungx could you please help to see why it fails with the following? I'm brining the same version as you brought into client-go. Locally it builds w/o any issue.

compilepkg: missing strict dependencies:
	/DATA/disk1/bazel/remote/shard/executions/487f99c3-30da-4b7e-91f2-5b5dcf6d2ecf/pkg/domain/domain_sysvars.go: import of "github.com/tikv/pd/client/pkg/circuitbreaker"
No dependencies were provided.

https://tiprow.hawkingrei.com/view/gs/pingcapprow/pr-logs/pull/pingcap_tidb/58737/fast_test_tiprow/1876806084379807744

rleungx · 2025-01-08T03:21:01Z

@Tema, I have created an issue: #58780. You can refer to this one in the PR description. We will check if each PR has at least one associated issue in the corresponding repo.

rleungx · 2025-01-08T03:26:31Z

@rleungx could you please help to see why it fails with the following? I'm brining the same version as you brought into client-go. Locally it builds w/o any issue.
compilepkg: missing strict dependencies:
	/DATA/disk1/bazel/remote/shard/executions/487f99c3-30da-4b7e-91f2-5b5dcf6d2ecf/pkg/domain/domain_sysvars.go: import of "github.com/tikv/pd/client/pkg/circuitbreaker"
No dependencies were provided.
https://tiprow.hawkingrei.com/view/gs/pingcapprow/pr-logs/pull/pingcap_tidb/58737/fast_test_tiprow/1876806084379807744

Have you run make bazel_prepare?

Tema · 2025-01-08T23:48:36Z

@rleungx could you please help to see why it fails with the following? I'm brining the same version as you brought into client-go. Locally it builds w/o any issue.

Have you run make bazel_prepare?

Thanks. I've done it now but it still complains with other error:
Repository rule go_repository defined at:

/root/.cache/bazel/_bazel_root/37ff7b0eddea115f60ab9f853788d7f5/external/bazel_gazelle/internal/go_repository.bzl:363:32: in <toplevel>
ERROR: /root/.cache/bazel/_bazel_root/37ff7b0eddea115f60ab9f853788d7f5/external/bazel_gazelle/internal/go_repository.bzl:150:42: An error occurred during the fetch of repository 'com_github_tikv_client_go_v2':
 Traceback (most recent call last):
  File "/root/.cache/bazel/_bazel_root/37ff7b0eddea115f60ab9f853788d7f5/external/bazel_gazelle/internal/go_repository.bzl", line 150, column 42, in _go_repository_impl
  	result = ctx.download_and_extract(
Error in download_and_extract: java.io.IOException: Error downloading [http://bazel-cache.pingcap.net:8080/gomod/github.com/Tema/client-go/v2/com_github_tema_client_go_v2-v2.0.0-20250108222013-bf5b9450dbba.zip, http://ats.apps.svc/gomod/github.com/Tema/client-go/v2/com_github_tema_client_go_v2-v2.0.0-20250108222013-bf5b9450dbba.zip, https://cache.hawkingrei.com/gomod/github.com/Tema/client-go/v2/com_github_tema_client_go_v2-v2.0.0-20250108222013-bf5b9450dbba.zip, https://storage.googleapis.com/pingcapmirror/gomod/github.com/Tema/client-go/v2/com_github_tema_client_go_v2-v2.0.0-20250108222013-bf5b9450dbba.zip] to /root/.cache/bazel/_bazel_root/37ff7b0eddea115f60ab9f853788d7f5/external/com_github_tikv_client_go_v2/temp61861513358319[65](https://tiprow.hawkingrei.com/view/gs/pingcapprow/pr-logs/pull/pingcap_tidb/58737/fast_test_tiprow/1877138363971014656#1:build-log.txt%3A65)327/com_github_tema_client_go_v2-v2.0.0-20250108222013-bf5b9450dbba.zip: GET returned 404 Not Found

Looks like bazel stuff does not work well with referenced to personal repos like github.com/Tema/client-go/v2.

rleungx · 2025-01-09T02:44:54Z

@hawkingrei PTAL

Signed-off-by: artem_danilov <[email protected]>

Tema · 2025-01-15T20:34:39Z

@rleungx I've rebased this PR and refer the new tip of the master https://github.com/tikv/client-go, but bazel still fails even though I've included results of make bazel_prepare. Looks like https://github.com/tikv/client-go?tab=readme-ov-file#test-with-tidb instruction does not work since bazel was introduced and there are some steps missing now.

from fast_test_tiprow below:

ERROR: /root/.cache/bazel/_bazel_root/37ff7b0eddea115f60ab9f853788d7f5/external/bazel_gazelle/internal/go_repository.bzl:150:42: An error occurred during the fetch of repository 'com_github_tikv_client_go_v2':
   Traceback (most recent call last):
	File "/root/.cache/bazel/_bazel_root/37ff7b0eddea115f60ab9f853788d7f5/external/bazel_gazelle/internal/go_repository.bzl", line 150, column 42, in _go_repository_impl
		result = ctx.download_and_extract(
Error in download_and_extract: java.io.IOException: Error downloading [http://bazel-cache.pingcap.net:8080/gomod/github.com/tikv/client-go/v2/com_github_tikv_client_go_v2-v2.0.8-0.20250115040613-be4b478c11de.zip, http://ats.apps.svc/gomod/github.com/tikv/client-go/v2/com_github_tikv_client_go_v2-v2.0.8-0.20250115040613-be4b478c11de.zip, https://cache.hawkingrei.com/gomod/github.com/tikv/client-go/v2/com_github_tikv_client_go_v2-v2.0.8-0.20250115040613-be4b478c11de.zip, https://storage.googleapis.com/pingcapmirror/gomod/github.com/tikv/client-go/v2/com_github_tikv_client_go_v2-v2.0.8-0.20250115040613-be4b478c11de.zip] to /root/.cache/bazel/_bazel_root/37ff7b0eddea115f60ab9f853788d7f5/external/com_github_tikv_client_go_v2/temp302576732149030251/com_github_tikv_client_go_v2-v2.0.8-0.20250115040613-be4b478c11de.zip: GET returned 404 Not Found

rleungx · 2025-01-16T03:07:23Z

/test build

tiprow · 2025-01-16T03:07:29Z

@rleungx: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test fast_test_tiprow
/test tidb_parser_test

Use /test all to run all jobs.

In response to this:

/test build

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.

rleungx · 2025-01-16T03:30:31Z

ERROR: /home/jenkins/agent/workspace/pingcap/tidb/ghpr_build/tidb/pkg/domain/BUILD.bazel:3:11: Validating nogo output for //pkg/domain:domain failed: (Exit 1): builder failed: error executing command (from target //pkg/domain:domain) bazel-out/k8-opt-exec-2B5CBBC6-ST-00bca8d33aad/bin/external/go_sdk/builder_reset/builder nogovalidation bazel-out/k8-fastbuild-ST-00bca8d33aad/bin/pkg/domain/domain.nogo ... (remaining 1 argument skipped)



Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging



nogo: errors found by nogo during build-time code analysis:

pkg/domain/domain_sysvars.go:158:7: unused-receiver: method receiver 'do' is not referenced in method's body, consider removing or renaming it as _ (revive)

I think it works now.

rleungx · 2025-01-16T03:31:04Z

/test fast_test_tiprow

ti-chi-bot · 2025-01-16T03:31:07Z

@rleungx: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test build

/test check-dev

/test check-dev2

/test mysql-test

/test pull-br-integration-test

/test pull-integration-ddl-test

/test pull-integration-e2e-test

/test pull-lightning-integration-test

/test pull-mysql-client-test

/test pull-unit-test-ddlv1

/test unit-test

The following commands are available to trigger optional jobs:

/test pingcap/tidb/canary_ghpr_unit_test

/test pull-common-test

/test pull-e2e-test

/test pull-integration-common-test

/test pull-integration-copr-test

/test pull-integration-jdbc-test

/test pull-integration-mysql-test

/test pull-integration-nodejs-test

/test pull-integration-python-orm-test

/test pull-scan-deps

/test pull-sqllogic-test

/test pull-tiflash-test

Use /test all to run the following jobs that were automatically triggered:

pingcap/tidb/ghpr_build

pingcap/tidb/ghpr_check

pingcap/tidb/ghpr_check2

pingcap/tidb/ghpr_mysql_test

pingcap/tidb/ghpr_unit_test

pingcap/tidb/pull_br_integration_test

pingcap/tidb/pull_integration_ddl_test

pingcap/tidb/pull_integration_e2e_test

pingcap/tidb/pull_lightning_integration_test

pingcap/tidb/pull_mysql_client_test

pull-scan-deps

In response to this:

/test fast_test_tiprow

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-01-16T03:31:09Z

@rleungx: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test fast_test_tiprow
/test tidb_parser_test

Use /test all to run all jobs.

In response to this:

/test fast_test_tiprow

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-01-16T03:31:29Z

@ti-chi-bot[bot]: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test fast_test_tiprow
/test tidb_parser_test

Use /test all to run all jobs.

In response to this:

@rleungx: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:
/test build
/test check-dev
/test check-dev2
/test mysql-test
/test pull-br-integration-test
/test pull-integration-ddl-test
/test pull-integration-e2e-test
/test pull-lightning-integration-test
/test pull-mysql-client-test
/test pull-unit-test-ddlv1
/test unit-test
The following commands are available to trigger optional jobs:
/test pingcap/tidb/canary_ghpr_unit_test
/test pull-common-test
/test pull-e2e-test
/test pull-integration-common-test
/test pull-integration-copr-test
/test pull-integration-jdbc-test
/test pull-integration-mysql-test
/test pull-integration-nodejs-test
/test pull-integration-python-orm-test
/test pull-scan-deps
/test pull-sqllogic-test
/test pull-tiflash-test
Use /test all to run the following jobs that were automatically triggered:
pingcap/tidb/ghpr_build
pingcap/tidb/ghpr_check
pingcap/tidb/ghpr_check2
pingcap/tidb/ghpr_mysql_test
pingcap/tidb/ghpr_unit_test
pingcap/tidb/pull_br_integration_test
pingcap/tidb/pull_integration_ddl_test
pingcap/tidb/pull_integration_e2e_test
pingcap/tidb/pull_lightning_integration_test
pingcap/tidb/pull_mysql_client_test
pull-scan-deps
In response to this:

/test fast_test_tiprow

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.

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.

rleungx · 2025-01-16T03:36:15Z

/retest

rleungx · 2025-01-16T05:12:33Z

@Tema FYI, the test fast_test_tiprow seems have problem. If wanna rerun failed tests, you can just comment retest.

lcwangchao · 2025-01-16T05:16:42Z

pkg/domain/domain_sysvars.go

@@ -150,3 +154,9 @@ func (do *Domain) changeSchemaCacheSize(ctx context.Context, size uint64) error
 	do.infoCache.Data.SetCacheCapacity(size)
 	return nil
 }
+
+func (do *Domain) changePDMetadataCircuitBreakerErrorRateThresholdPct(errorRatePct uint32) {
+	tikv.ChangePDRegionMetaCircuitBreakerSettings(func(config *circuitbreaker.Settings) {


Seems this method does not need the Domain state, why not just call tikv. ChangePDRegionMetaCircuitBreakerSettings in SetGlobal ?

Signed-off-by: artem_danilov <[email protected]>

Tema · 2025-01-16T19:30:44Z

@rleungx all tests passed finally!

rleungx · 2025-01-17T08:29:21Z

This PR introduces a new variable tidb_cb_pd_metadata_error_rate_threshold_pct. The value can be 0-100, 0 stands for disabling the circuit breaker. After enabling it, whether the circuit breaker work or not is decided by if the corresponding request error rate reaches the tidb_cb_pd_metadata_error_rate_threshold_pct.
We can use this mechanism to prevent PD from being overwhelmed by requests when PD couldn't work. More detailed information can be found in the PR description.

@benmeadowcroft @yudongusa PTAL

okJiang

LGTM except the comment

ti-chi-bot · 2025-01-21T09:14:20Z

[LGTM Timeline notifier]

Timeline:

2025-01-21 06:21:04.437865536 +0000 UTC m=+161791.768784937: ☑️ agreed by rleungx.
2025-01-21 09:14:19.340782935 +0000 UTC m=+172186.671702341: ☑️ agreed by tangenta.

tangenta · 2025-01-21T09:14:22Z

lgtm for domain part

Tema · 2025-01-21T19:59:18Z

LGTM except the comment

@okJiang what comment? I'v addressed all I could see.

Signed-off-by: artem_danilov <[email protected]>

rleungx · 2025-01-23T07:01:23Z

/retest

ti-chi-bot · 2025-01-24T01:13:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lcwangchao, rleungx, tangenta, yudongusa

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [lcwangchao,tangenta]
~~pkg/domain/OWNERS~~ [tangenta]
~~pkg/sessionctx/vardef/OWNERS~~ [yudongusa]
~~pkg/sessionctx/variable/OWNERS~~ [yudongusa]

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

Signed-off-by: artem_danilov <[email protected]>

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 Jan 7, 2025

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

ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jan 8, 2025

ti-chi-bot bot removed the do-not-merge/needs-linked-issue label Jan 8, 2025

Tema force-pushed the pd-circuit-breaker branch from fab27a5 to 3586031 Compare January 8, 2025 19:52

Tema mentioned this pull request Jan 8, 2025

integrate circuitbreaker for region calls tikv/client-go#1543

Merged

*: integrate circuitbreaker for get region calls to PD

1bc1b64

Signed-off-by: artem_danilov <[email protected]>

Tema force-pushed the pd-circuit-breaker branch from 8f454b4 to 1bc1b64 Compare January 15, 2025 20:27

lcwangchao reviewed Jan 16, 2025

View reviewed changes

Tema added 3 commits January 16, 2025 09:21

fix func definition

4b88223

Signed-off-by: artem_danilov <[email protected]>

fix comment format

813644e

Signed-off-by: artem_danilov <[email protected]>

fix comment format

f53ea85

Signed-off-by: artem_danilov <[email protected]>

rleungx approved these changes Jan 21, 2025

View reviewed changes

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

okJiang reviewed Jan 21, 2025

View reviewed changes

tangenta approved these changes Jan 21, 2025

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jan 21, 2025

Merge remote-tracking branch 'tema/master' into pd-circuit-breaker

03ed2d4

Signed-off-by: artem_danilov <[email protected]>

lcwangchao approved these changes Jan 22, 2025

View reviewed changes

yudongusa approved these changes Jan 23, 2025

View reviewed changes

ti-chi-bot bot added the approved label Jan 23, 2025

Merge remote-tracking branch 'tema/master' into pd-circuit-breaker

ac51a33

Tema added 2 commits January 23, 2025 17:59

fix merge typos

54a5aa5

Signed-off-by: artem_danilov <[email protected]>

fix merge issues

566bda8

Signed-off-by: artem_danilov <[email protected]>

ti-chi-bot bot merged commit a573e49 into pingcap:master Jan 24, 2025
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: integrate circuitbreaker for get region calls to PD #58737

*: integrate circuitbreaker for get region calls to PD #58737

Tema commented Jan 7, 2025 •

edited

Loading

ti-chi-bot bot commented Jan 7, 2025

tiprow bot commented Jan 7, 2025

ti-chi-bot commented Jan 7, 2025

Tema commented Jan 7, 2025

codecov bot commented Jan 7, 2025 •

edited

Loading

Tema commented Jan 8, 2025

rleungx commented Jan 8, 2025 •

edited

Loading

rleungx commented Jan 8, 2025

Tema commented Jan 8, 2025

rleungx commented Jan 9, 2025

Tema commented Jan 15, 2025 •

edited

Loading

rleungx commented Jan 16, 2025

tiprow bot commented Jan 16, 2025

rleungx commented Jan 16, 2025

rleungx commented Jan 16, 2025

ti-chi-bot bot commented Jan 16, 2025

tiprow bot commented Jan 16, 2025

tiprow bot commented Jan 16, 2025

rleungx commented Jan 16, 2025

rleungx commented Jan 16, 2025

lcwangchao Jan 16, 2025

Tema commented Jan 16, 2025

rleungx commented Jan 17, 2025 •

edited

Loading

okJiang left a comment

ti-chi-bot bot commented Jan 21, 2025

tangenta commented Jan 21, 2025

Tema commented Jan 21, 2025

rleungx commented Jan 23, 2025

ti-chi-bot bot commented Jan 24, 2025

*: integrate circuitbreaker for get region calls to PD #58737

*: integrate circuitbreaker for get region calls to PD #58737

Conversation

Tema commented Jan 7, 2025 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Jan 7, 2025

tiprow bot commented Jan 7, 2025

ti-chi-bot commented Jan 7, 2025

Tema commented Jan 7, 2025

codecov bot commented Jan 7, 2025 • edited Loading

Codecov Report

Tema commented Jan 8, 2025

rleungx commented Jan 8, 2025 • edited Loading

rleungx commented Jan 8, 2025

Tema commented Jan 8, 2025

rleungx commented Jan 9, 2025

Tema commented Jan 15, 2025 • edited Loading

rleungx commented Jan 16, 2025

tiprow bot commented Jan 16, 2025

rleungx commented Jan 16, 2025

rleungx commented Jan 16, 2025

ti-chi-bot bot commented Jan 16, 2025

tiprow bot commented Jan 16, 2025

tiprow bot commented Jan 16, 2025

rleungx commented Jan 16, 2025

rleungx commented Jan 16, 2025

lcwangchao Jan 16, 2025

Choose a reason for hiding this comment

Tema commented Jan 16, 2025

rleungx commented Jan 17, 2025 • edited Loading

okJiang left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Jan 21, 2025

[LGTM Timeline notifier]

tangenta commented Jan 21, 2025

Tema commented Jan 21, 2025

rleungx commented Jan 23, 2025

ti-chi-bot bot commented Jan 24, 2025

Tema commented Jan 7, 2025 •

edited

Loading

codecov bot commented Jan 7, 2025 •

edited

Loading

rleungx commented Jan 8, 2025 •

edited

Loading

Tema commented Jan 15, 2025 •

edited

Loading

rleungx commented Jan 17, 2025 •

edited

Loading