Skip to content
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

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Tema
Copy link
Contributor

@Tema Tema commented Jan 7, 2025

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

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

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 ti-chi-bot bot added do-not-merge/needs-linked-issue do-not-merge/needs-tests-checked release-note-none Denotes a PR that doesn't merit a release note. needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 7, 2025
Copy link

ti-chi-bot bot commented Jan 7, 2025

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.

Copy link

ti-chi-bot bot commented Jan 7, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Needs approval from an approver in each of these files:

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

Copy link

tiprow bot commented Jan 7, 2025

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 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
Copy link
Member

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

@Tema
Copy link
Contributor Author

Tema commented Jan 7, 2025

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

Copy link

codecov bot commented Jan 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.3619%. Comparing base (6180fb7) to head (f53ea85).
Report is 8 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #58737        +/-   ##
================================================
+ Coverage   73.4990%   74.3619%   +0.8628%     
================================================
  Files          1680       1730        +50     
  Lines        465221     477021     +11800     
================================================
+ Hits         341933     354722     +12789     
+ Misses       102424      99889      -2535     
- Partials      20864      22410      +1546     
Flag Coverage Δ
integration 48.6723% <100.0000%> (?)
unit 72.1917% <100.0000%> (-0.1207%) ⬇️

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

Components Coverage Δ
dumpling 52.6910% <ø> (ø)
parser ∅ <ø> (∅)
br 57.0600% <ø> (+11.6554%) ⬆️

@Tema
Copy link
Contributor Author

Tema commented Jan 8, 2025

@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

@ti-chi-bot 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
@rleungx
Copy link
Member

rleungx commented Jan 8, 2025

@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
Copy link
Member

rleungx commented Jan 8, 2025

@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
Copy link
Contributor Author

Tema commented Jan 8, 2025

@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
Copy link
Member

rleungx commented Jan 9, 2025

@hawkingrei PTAL

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

Tema commented Jan 15, 2025

@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
Copy link
Member

rleungx commented Jan 16, 2025

/test build

Copy link

tiprow bot commented Jan 16, 2025

@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
Copy link
Member

rleungx commented Jan 16, 2025

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
Copy link
Member

rleungx commented Jan 16, 2025

/test fast_test_tiprow

Copy link

ti-chi-bot bot commented Jan 16, 2025

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

Copy link

tiprow bot commented Jan 16, 2025

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

Copy link

tiprow bot commented Jan 16, 2025

@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
Copy link
Member

rleungx commented Jan 16, 2025

/retest

@rleungx
Copy link
Member

rleungx commented Jan 16, 2025

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

@@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Tema added 3 commits January 16, 2025 09:21
Signed-off-by: artem_danilov <[email protected]>
Signed-off-by: artem_danilov <[email protected]>
Signed-off-by: artem_danilov <[email protected]>
@Tema
Copy link
Contributor Author

Tema commented Jan 16, 2025

@rleungx all tests passed finally!

@rleungx
Copy link
Member

rleungx commented Jan 17, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Indicates a PR is ready to be tested. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support setting circuit breaker to avoid PD overloaded
4 participants