Skip to content

Add circuit breaker variable #20129

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

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

Add circuit breaker variable #20129

wants to merge 2 commits into from

Conversation

rleungx
Copy link
Member

@rleungx rleungx commented Jan 23, 2025

First-time contributors' checklist

What is changed, added or deleted? (Required)

Which TiDB version(s) do your changes apply to? (Required)

Tips for choosing the affected version(s):

By default, CHOOSE MASTER ONLY so your changes will be applied to the next TiDB major or minor releases. If your PR involves a product feature behavior change or a compatibility change, CHOOSE THE AFFECTED RELEASE BRANCH(ES) AND MASTER.

For details, see tips for choosing the affected versions.

  • master (the latest development version)
  • v9.0 (TiDB 9.0 versions)
  • v8.5 (TiDB 8.5 versions)
  • v8.4 (TiDB 8.4 versions)
  • v8.3 (TiDB 8.3 versions)
  • v8.1 (TiDB 8.1 versions)
  • v7.5 (TiDB 7.5 versions)
  • v7.1 (TiDB 7.1 versions)
  • v6.5 (TiDB 6.5 versions)
  • v6.1 (TiDB 6.1 versions)
  • v5.4 (TiDB 5.4 versions)

What is the related PR or file link(s)?

Do your changes match any of the following descriptions?

  • Delete files
  • Change aliases
  • Need modification after applied to another branch
  • Might cause conflicts after applied to another branch

@ti-chi-bot ti-chi-bot bot added missing-translation-status This PR does not have translation status info. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 23, 2025
@hfxsd hfxsd added the translation/done This PR has been translated from English into Chinese and updated to pingcap/docs-cn in a PR. label Jan 23, 2025
@ti-chi-bot ti-chi-bot bot removed the missing-translation-status This PR does not have translation status info. label Jan 23, 2025
@hfxsd hfxsd self-assigned this Jan 23, 2025
@hfxsd hfxsd requested a review from benmeadowcroft January 23, 2025 05:55
@ti-chi-bot ti-chi-bot bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 23, 2025
@hfxsd hfxsd added the type/compatibility-or-feature-change This PR involves compatibility changes or feature behavior changes. label Jan 23, 2025
@hfxsd hfxsd requested a review from lilin90 January 23, 2025 07:55
@rleungx
Copy link
Member Author

rleungx commented Mar 11, 2025

/hold

@ti-chi-bot ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 11, 2025
@hfxsd hfxsd added the v9.0-beta.2 This PR/issue applies to TiDB v9.0-beta.2. label Mar 31, 2025
Copy link

ti-chi-bot bot commented Jun 5, 2025

@benmeadowcroft: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

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
Collaborator

@benmeadowcroft benmeadowcroft left a comment

Choose a reason for hiding this comment

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

Let's make it a bit clearer how this should be set and the meaning.

E.g. perhaps updating to:

This variable is used to control when TiDB triggers the circuit breaker. If set to 0 (the default) then the circuit breaker is disabled. If the variable is set to 1 to 100 then the circuit breaker is triggered if the error rate percentage, of the specific requests sent to PD, meets or exceeds the threshold.

Copy link

ti-chi-bot bot commented Jun 5, 2025

@benmeadowcroft: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

Let's make it a bit clearer how this should be set and the meaning.

E.g. perhaps updating to:

This variable is used to control when TiDB triggers the circuit breaker. If set to 0 (the default) then the circuit breaker is disabled. If the variable is set to 1 to 100 then the circuit breaker is triggered if the error rate percentage, of the specific requests sent to PD, meets or exceeds the threshold.

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.

@benmeadowcroft
Copy link
Collaborator

benmeadowcroft commented Jun 5, 2025

I am also confused why we are using Range: [0, 100] for this variable instead of Range: [0.0, 1.0] as we have done for other ratio/percentage based thresholds in this configuration file.

@rleungx
Copy link
Member Author

rleungx commented Jun 10, 2025

Maybe we can change it to [0, 1]?
/cc @Tema @niubell @okJiang

@ti-chi-bot ti-chi-bot bot requested review from niubell and okJiang June 10, 2025 03:17
Copy link

ti-chi-bot bot commented Jun 10, 2025

@rleungx: GitHub didn't allow me to request PR reviews from the following users: tema.

Note that only pingcap members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Maybe we can change it to [0, 1]?
/cc @Tema @niubell @okJiang

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.

@okJiang
Copy link
Member

okJiang commented Jun 10, 2025

Maybe we can change it to [0, 1]? /cc @Tema @niubell @okJiang

LGTM

rleungx added 2 commits June 10, 2025 13:37
Signed-off-by: Ryan Leung <[email protected]>
@rleungx rleungx force-pushed the add-circuit-breaker branch from 224b5d5 to 1682aea Compare June 10, 2025 05:37
Copy link

ti-chi-bot bot commented Jun 10, 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 ask for approval from hfxsd. 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:

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 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 Jun 10, 2025
Copy link

ti-chi-bot bot commented Jun 10, 2025

@rleungx: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-verify 1682aea link true /test pull-verify

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

Copy link

ti-chi-bot bot commented Jun 10, 2025

@benmeadowcroft: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

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.

@Tema
Copy link

Tema commented Jun 10, 2025

Maybe we can change it to [0, 1]? /cc @Tema @niubell @okJiang

@rleungx I don't mind the pct/ratio, but I wish you changed the name as well. The _pct suffix in the name meant to mean percentage. Now it is a bit confusing.
cc: @benmeadowcroft

@rleungx
Copy link
Member Author

rleungx commented Jun 11, 2025

AFAIK, we have another variable tidb_instance_plan_cache_reserved_percentage which uses the percentage as the suffix. And its range is [0, 1]. See https://docs.pingcap.com/tidb/stable/system-variables/#tidb_instance_plan_cache_reserved_percentage-new-in-v840. /cc @benmeadowcroft

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. translation/done This PR has been translated from English into Chinese and updated to pingcap/docs-cn in a PR. type/compatibility-or-feature-change This PR involves compatibility changes or feature behavior changes. v9.0-beta.2 This PR/issue applies to TiDB v9.0-beta.2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants