txn: restore failed due to file end key is empty #52572
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
do-not-merge/needs-tests-checked
do-not-merge/work-in-progress
|
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
removed
the
do-not-merge/work-in-progress
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #52572 +/- ##
=================================================
- Coverage 72.3811% 56.2314% -16.1497%
=================================================
Files 1482 1595 +113
Lines 428743 598501 +169758
=================================================
+ Hits 310329 336546 +26217
- Misses 99115 238482 +139367
- Partials 19299 23473 +4174
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
/test unit-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. |
| @@ -1588,7 +1588,10 @@ func (rc *Client) WaitForFilesRestored(ctx context.Context, files []*backuppb.Fi | |||
| fileReplica := file | |||
| rc.workerPool.ApplyOnErrorGroup(eg, | |||
| func() error { | |||
| defer updateCh.Inc() | |||
| defer func() { | |||
| log.Info("import sst files done", logutil.Files(files)) | |||
There was a problem hiding this comment.
Will this be too verbose?
There was a problem hiding this comment.
tidb backup also have the same log. I think it's okay when restore a large amount data. it's better to know br is importing rather than nothing output.
br/pkg/restore/import.go
Outdated
| if err != nil { | ||
| return nil, nil, errors.Trace(err) | ||
| } | ||
| return start, end, nil |
There was a problem hiding this comment.
Maybe just return GetRewriteRawKeys(f, rules)?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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:
Approvers can indicate their approval by writing |
|
@3pointer: The following test failed, say
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/test-infra repository. I understand the commands that are listed here. |
|
/test unit-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. |
|
Does br txn support incremental backup? How to use br txn for incremental backup? |
|
For now |
Do you have any plans to implement this feature? |
For now, no. 😢 But you may try to implement a tiny version manually, that is, parse the argument Then it might be able to backup the diff between last backup and now(but still not a real-time continuous backup.). Also notice this may not be pretty useful as it requires the gc safe point doesn't exceed the |
Can the continuity of backups be ensured by disabling the GC or increasing the time of |
Perhaps, backing up diffs then restore them one-by-one probably works. In fact backing up txn data is simpler because we have get rid of the DDLs and table schemas. The potential problem is that nobody have tested this as far as I know - theoretically it works, but when you use it, you are in risk. BTW discussing this topic here is somehow beyond this PR. Could you open an issue for further discussion? |
|
When executing the
What is the cause? Is there any impact on backup and recovery? |
|
In response to a cherrypick label: new pull request created to branch |
What problem does this PR solve?
Issue Number: close #52574
Problem Summary:
Restore txn kv might failed about handling empty end key. This PR fixed it.
What changed and how does it work?
Don't encode empty value of end key.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.