br: refactor error handle mechanism to tolerant unexpect kv errors. #48646
Conversation
ti-chi-bot
bot
added
do-not-merge/needs-tests-checked
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
bot
added
needs-cherry-pick-release-6.5
|
Hi @3pointer. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with 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/test-infra repository. |
ti-chi-bot
bot
added
size/XL
ti-chi-bot
bot
removed
the
do-not-merge/work-in-progress
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #48646 +/- ##
================================================
+ Coverage 71.0454% 72.0836% +1.0382%
================================================
Files 1368 1407 +39
Lines 402969 417762 +14793
================================================
+ Hits 286291 301138 +14847
- Misses 96737 97753 +1016
+ Partials 19941 18871 -1070
Flags with carried forward coverage won't be shown. Click here to find out more.
|
br/pkg/utils/retry.go
Outdated
|
|
||
| const ( | ||
| // This type can be retry but consume the backoffer attempts. | ||
| Retry ErrorStrategy = iota |
There was a problem hiding this comment.
I will prefer add a prefix Strategy to this or extract this retry related utilities to another package. utils.Retry looks like a function.
| if messageIsNotFoundStorageError(msg) { | ||
| reason := fmt.Sprintf("File or directory not found on TiKV Node (store id: %v). "+ | ||
| "work around:please ensure br and tikv nodes share a same storage and the user of br and tikv has same uid.", | ||
| uuid) |
There was a problem hiding this comment.
because during restore. we cannot get store id
br/pkg/utils/retry.go
Outdated
| return ErrorResult{GiveUp, "unknown error and retry too many times, give up"} | ||
| } | ||
|
|
||
| func (ec *ErrorContext) HandleErrorPb(e *backuppb.Error, uuid uint64, canIgnore bool) ErrorResult { |
There was a problem hiding this comment.
canIgnore looks so magical here, I guess it should be the requirement of the coarse-grained backup. I will prefer make another function to handle the scenario.
There was a problem hiding this comment.
pushBackup need it, but fine-grained doesn't. Do you think make it a field of errContext is better?
There was a problem hiding this comment.
I think we can make two functions: HandleErrAndIgnoreStoreErrors and HandleErr or something. 🤔
3pointer
force-pushed
the
error_refactor
branch
from
e2c5f46 to
26819c9
Compare
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the For example:
📖 For more info, you can check the "Contribute Code" section in the development guide. |
ti-chi-bot
bot
added
approved
needs-1-more-lgtm
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
|
/test mysql-test |
|
@3pointer: Cannot trigger testing until a trusted user reviews the PR and leaves an 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/test-infra repository. |
|
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <[email protected]>
|
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <[email protected]>
3pointer
added
the
needs-cherry-pick-release-7.5
|
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <[email protected]>
What problem does this PR solve?
Issue Number: close #47656
Problem Summary:
record the state of unknown kv error and give some tolerance of these unknown error.
What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.