br: refactor br cli backoff logic #54644

Tristan1900 · 2024-07-15T22:54:53Z

What problem does this PR solve?

Issue Number: close #54643

Problem Summary:

Refactor the BR CLI backoff retry logic to make is more simplified and extendable in future.

What changed and how does it work?

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

NA

ti-chi-bot · 2024-07-15T22:54:56Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

sre-bot · 2024-07-15T22:55:00Z

All committers have signed the CLA.

tiprow · 2024-07-15T22:55:13Z

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

Tristan1900 · 2024-07-15T22:59:49Z

tests to be fixed
also pending on #54596

codecov · 2024-10-01T16:21:31Z

Codecov Report

Attention: Patch coverage is 82.33438% with 56 lines in your changes missing coverage. Please review.

Project coverage is 74.8428%. Comparing base (ec8b81b) to head (77b4f63).
Report is 8 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #54644        +/-   ##
================================================
+ Coverage   72.8314%   74.8428%   +2.0114%     
================================================
  Files          1677       1722        +45     
  Lines        463934     472924      +8990     
================================================
+ Hits         337890     353950     +16060     
+ Misses       105166      96868      -8298     
- Partials      20878      22106      +1228

Flag	Coverage Δ
integration	`49.4041% <72.9299%> (?)`
unit	`72.3484% <76.3406%> (+0.1258%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.7673% <100.0000%> (ø)`
parser	`∅ <ø> (∅)`
br	`61.2357% <82.1656%> (+15.7033%)`	⬆️

YuJuncen

rest lgtm

YuJuncen · 2024-10-28T10:14:04Z

br/pkg/utils/backoff.go

+	isRetryErrFunc := buildIsRetryErrFunc(retryErrs, grpcRetryCodes)
+	isNonRetryErrFunc := buildIsNonRetryErrFunc(nonRetryErrs)
+
+	return newBackoffStrategy(maxRetry, delayTime, maxDelayTime, errContext, isRetryErrFunc, isNonRetryErrFunc)


Could you replace this constructor with functional options or builder pattern?

ti-chi-bot · 2024-11-08T08:50:32Z

PR needs rebase.

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.

Tristan1900 · 2024-11-25T02:58:01Z

br/pkg/restore/log_client/import_retry.go

@@ -251,10 +240,6 @@ func (r *RPCResult) StrategyForRetryStoreError() RetryStrategy {
 }

 func (r *RPCResult) StrategyForRetryGoError() RetryStrategy {
-	if r.Err == nil {


only get called when error is not nil

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

ti-chi-bot · 2024-11-26T04:16:40Z

[LGTM Timeline notifier]

Timeline:

2024-10-28 10:15:47.880664025 +0000 UTC m=+258460.719819570: ☑️ agreed by YuJuncen.
2024-11-26 04:16:39.546514181 +0000 UTC m=+523587.166168696: ☑️ agreed by Leavrth.

tiprow · 2024-11-26T08:23:31Z

@Tristan1900: 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
fast_test_tiprow	`77b4f63`	link	true	`/test fast_test_tiprow`

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.

Tristan1900 · 2024-11-26T16:11:37Z

/retest

tiprow · 2024-11-26T16:11:59Z

@Tristan1900: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

Tristan1900 · 2024-11-26T16:14:08Z

/assign @GMHDBJD

GMHDBJD

LGTM for dumpling

ti-chi-bot · 2024-11-26T16:17:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: GMHDBJD, Leavrth, YuJuncen

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:

~~br/OWNERS~~ [Leavrth,YuJuncen]
~~dumpling/OWNERS~~ [GMHDBJD]

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 bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 15, 2024

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 23, 2024

BornChanger added component/br This issue is related to BR of TiDB. and removed component/dumpling This is related to Dumpling of TiDB. labels Jul 29, 2024

Tristan1900 force-pushed the br-code-cleanup branch from bc730cc to eeb8f5a Compare October 1, 2024 00:43

Tristan1900 marked this pull request as ready for review October 1, 2024 00:44

ti-chi-bot bot removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/needs-linked-issue labels Oct 1, 2024

Tristan1900 force-pushed the br-code-cleanup branch from eeb8f5a to 81234e3 Compare October 1, 2024 16:01

ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 4, 2024

YuJuncen approved these changes Oct 28, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Oct 28, 2024

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2024

Tristan1900 force-pushed the br-code-cleanup branch from 33fa670 to 1cc3695 Compare November 25, 2024 02:53

Tristan1900 commented Nov 25, 2024

View reviewed changes

Tristan1900 added 5 commits November 25, 2024 13:32

refactor backoff

29c78b6

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

rebase master

c399523

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

fix tests

1d313d9

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

fix style

9dcb60e

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

address comments

77b4f63

Signed-off-by: Wenqi Mou <wenqimou@gmail.com>

Tristan1900 force-pushed the br-code-cleanup branch from ab6cb2d to 77b4f63 Compare November 25, 2024 18:34

Leavrth approved these changes Nov 26, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Nov 26, 2024

ti-chi-bot bot assigned GMHDBJD Nov 26, 2024

GMHDBJD approved these changes Nov 26, 2024

View reviewed changes

ti-chi-bot bot added the approved label Nov 26, 2024

ti-chi-bot bot merged commit 138386c into pingcap:master Nov 26, 2024
30 of 31 checks passed

Tristan1900 deleted the br-code-cleanup branch November 26, 2024 20:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

br: refactor br cli backoff logic #54644

br: refactor br cli backoff logic #54644

Tristan1900 commented Jul 15, 2024 •

edited

Loading

ti-chi-bot bot commented Jul 15, 2024

sre-bot commented Jul 15, 2024 •

edited

Loading

tiprow bot commented Jul 15, 2024

Tristan1900 commented Jul 15, 2024

codecov bot commented Oct 1, 2024 •

edited

Loading

YuJuncen left a comment

YuJuncen Oct 28, 2024

ti-chi-bot bot commented Nov 8, 2024

Tristan1900 Nov 25, 2024 •

edited

Loading

ti-chi-bot bot commented Nov 26, 2024

tiprow bot commented Nov 26, 2024

Tristan1900 commented Nov 26, 2024

tiprow bot commented Nov 26, 2024

Tristan1900 commented Nov 26, 2024

GMHDBJD left a comment

ti-chi-bot bot commented Nov 26, 2024

br: refactor br cli backoff logic #54644

br: refactor br cli backoff logic #54644

Conversation

Tristan1900 commented Jul 15, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Jul 15, 2024

sre-bot commented Jul 15, 2024 • edited Loading

tiprow bot commented Jul 15, 2024

Tristan1900 commented Jul 15, 2024

codecov bot commented Oct 1, 2024 • edited Loading

Codecov Report

YuJuncen left a comment

Choose a reason for hiding this comment

YuJuncen Oct 28, 2024

Choose a reason for hiding this comment

ti-chi-bot bot commented Nov 8, 2024

Tristan1900 Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

ti-chi-bot bot commented Nov 26, 2024

[LGTM Timeline notifier]

tiprow bot commented Nov 26, 2024

Tristan1900 commented Nov 26, 2024

tiprow bot commented Nov 26, 2024

Tristan1900 commented Nov 26, 2024

GMHDBJD left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Nov 26, 2024

Tristan1900 commented Jul 15, 2024 •

edited

Loading

sre-bot commented Jul 15, 2024 •

edited

Loading

codecov bot commented Oct 1, 2024 •

edited

Loading

Tristan1900 Nov 25, 2024 •

edited

Loading