br: cli refactor backup error handling logic #54697

Tristan1900 · 2024-07-17T17:46:43Z

What problem does this PR solve?

Issue Number: close #54696

Problem Summary:

What changed and how does it work?

Moved backup specific error handling logic out of the retry file, since the retry is shared by other usages as well.
Renamed the method and a few other variables.
Move as much as possible of hardcoded strings to be const so unit test can use
Delete unused methods, handleErrorPb and HandleError
Delete and move the permission file, as in the file only one method is related to permission.

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-17T17:46:46Z

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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/test-infra repository.

ti-chi-bot · 2024-07-17T17:46:50Z

[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 3pointer for approval. 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:

br/OWNERS

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

sre-bot · 2024-07-17T17:46:50Z

All committers have signed the CLA.

tiprow · 2024-07-17T17:47:01Z

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-17T17:53:10Z

br/pkg/utils/error_handling.go

+}
+
+// messageIsNotFoundStorageError checks whether the message returning from TiKV is "NotFound" storage I/O error
+func messageIsNotFoundStorageError(msg string) bool {


@3pointer does the storage here mean S3 or any backup destination? At first I thought it's TiKV internal but there is a proto type for it already so got a bit confused. Thanks

at this context storage means TiKV.

codecov · 2024-07-17T18:13:31Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.4630%. Comparing base (eab8700) to head (6b994c9).

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #54697         +/-   ##
=================================================
- Coverage   72.8958%   56.4630%   -16.4329%     
=================================================
  Files          1552       1674        +122     
  Lines        437298     609406     +172108     
=================================================
+ Hits         318772     344089      +25317     
- Misses        99002     241622     +142620     
- Partials      19524      23695       +4171

Flag	Coverage Δ
integration	`38.2258% <65.8227%> (?)`
unit	`71.7056% <100.0000%> (-0.2295%)`	⬇️

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

Components	Coverage Δ
dumpling	`52.9656% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`62.7950% <100.0000%> (+16.9003%)`	⬆️

3pointer · 2024-07-18T01:30:21Z

br/pkg/utils/error_handling.go

@@ -0,0 +1,181 @@
+package utils


new file should add license first, you can copy from other files.

ah! good catch

3pointer · 2024-07-18T01:34:12Z

br/pkg/utils/error_handling.go

+}
+
+// messageIsNotFoundStorageError checks whether the message returning from TiKV is "NotFound" storage I/O error
+func messageIsNotFoundStorageError(msg string) bool {


at this context storage means TiKV.

ti-chi-bot · 2024-07-18T07:36:53Z

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/test-infra repository.

Signed-off-by: Wenqi Mou <[email protected]>

YuJuncen

rest lgtm

YuJuncen · 2024-07-19T02:46:51Z

br/pkg/utils/error_handling.go

+
+const (
+ // Retry error can be retried but will consume the backoff attempt quota.
+ Retry ErrorHandlingStrategy = iota


Perhaps use StrategyRetry so we can know its type when using outside this package.

ti-chi-bot bot added do-not-merge/invalid-title do-not-merge/needs-linked-issue do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jul 17, 2024

ti-chi-bot bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 17, 2024

Tristan1900 changed the title ~~br cli refactor backup error handling logic~~ br: cli refactor backup error handling logic Jul 17, 2024

ti-chi-bot bot removed do-not-merge/invalid-title do-not-merge/needs-linked-issue labels Jul 17, 2024

Tristan1900 commented Jul 17, 2024

View reviewed changes

3pointer reviewed Jul 18, 2024

View reviewed changes

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

Tristan1900 added 6 commits July 18, 2024 12:06

br cli refactor backup error handling logic

e8e870f

Signed-off-by: Wenqi Mou <[email protected]>

refactor test and minor

976ad25

Signed-off-by: Wenqi Mou <[email protected]>

fix bazel

f917357

Signed-off-by: Wenqi Mou <[email protected]>

fix bazel

597dd71

Signed-off-by: Wenqi Mou <[email protected]>

annotate TODO

0642669

Signed-off-by: Wenqi Mou <[email protected]>

use goimports, add license

62b39d0

Signed-off-by: Wenqi Mou <[email protected]>

Tristan1900 force-pushed the br-refactor-backup-error-handling branch from 0387ae6 to 62b39d0 Compare July 18, 2024 17:23

fix import in test....

6b994c9

Signed-off-by: Wenqi Mou <[email protected]>

YuJuncen reviewed Jul 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

br: cli refactor backup error handling logic #54697

br: cli refactor backup error handling logic #54697

Tristan1900 commented Jul 17, 2024 •

edited

Loading

ti-chi-bot bot commented Jul 17, 2024

ti-chi-bot bot commented Jul 17, 2024

sre-bot commented Jul 17, 2024 •

edited

Loading

tiprow bot commented Jul 17, 2024

Tristan1900 Jul 17, 2024 •

edited

Loading

3pointer Jul 18, 2024

codecov bot commented Jul 17, 2024 •

edited

Loading

3pointer Jul 18, 2024

Tristan1900 Jul 18, 2024

3pointer Jul 18, 2024

ti-chi-bot bot commented Jul 18, 2024

YuJuncen left a comment

YuJuncen Jul 19, 2024

br: cli refactor backup error handling logic #54697

Are you sure you want to change the base?

br: cli refactor backup error handling logic #54697

Conversation

Tristan1900 commented Jul 17, 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 17, 2024

ti-chi-bot bot commented Jul 17, 2024

sre-bot commented Jul 17, 2024 • edited Loading

tiprow bot commented Jul 17, 2024

Tristan1900 Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

3pointer Jul 18, 2024

Choose a reason for hiding this comment

codecov bot commented Jul 17, 2024 • edited Loading

Codecov Report

3pointer Jul 18, 2024

Choose a reason for hiding this comment

Tristan1900 Jul 18, 2024

Choose a reason for hiding this comment

3pointer Jul 18, 2024

Choose a reason for hiding this comment

ti-chi-bot bot commented Jul 18, 2024

YuJuncen left a comment

Choose a reason for hiding this comment

YuJuncen Jul 19, 2024

Choose a reason for hiding this comment

Tristan1900 commented Jul 17, 2024 •

edited

Loading

sre-bot commented Jul 17, 2024 •

edited

Loading

Tristan1900 Jul 17, 2024 •

edited

Loading

codecov bot commented Jul 17, 2024 •

edited

Loading