br: compatibility of log backup and log restore #61238

Leavrth · 2025-05-21T08:57:15Z

What problem does this PR solve?

Issue Number: close #61237

Problem Summary:
compatibility of log backup and log restore

What changed and how does it work?

PITR can restore on the cluster running log backup, and these restored tables can not be restored until there is a full backup later.

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

Please refer to Release Notes Language Style Guide to write a quality release note.

None

Signed-off-by: Jianjun Liao <[email protected]>

tiprow · 2025-05-21T08:57:35Z

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

codecov · 2025-05-21T09:17:46Z

Codecov Report

Attention: Patch coverage is 63.78378% with 67 lines in your changes missing coverage. Please review.

Project coverage is 74.8684%. Comparing base (e3478a5) to head (f25e044).
Report is 18 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #61238        +/-   ##
================================================
+ Coverage   73.1657%   74.8684%   +1.7026%     
================================================
  Files          1725       1726         +1     
  Lines        477926     482337      +4411     
================================================
+ Hits         349678     361118     +11440     
+ Misses       106834      98996      -7838     
- Partials      21414      22223       +809

Flag	Coverage Δ
integration	`45.9862% <49.7297%> (?)`

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

Components	Coverage Δ
dumpling	`52.7804% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`61.9994% <63.7837%> (+14.4378%)`	⬆️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

BornChanger · 2025-05-21T13:34:24Z

/retest

tiprow · 2025-05-21T13:34:52Z

@BornChanger: 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.

br/pkg/restore/misc.go

br/pkg/restore/export_test.go

br/pkg/restore/misc.go

Tristan1900 · 2025-05-22T00:17:53Z

br/pkg/task/restore.go

+			restoreErr = err
+			return
+		}
+		tableIds := make([]int64, 0, len(cfg.PiTRTableTracker.TableIdToDBIds))


I think PiTRtableTracker might be nil if this is not a pitr restore, does the check logTaskBackend == nil guarantee this is a PiTR?

br/pkg/task/restore.go

Signed-off-by: Jianjun Liao <[email protected]>

3pointer

rest LGTM

3pointer · 2025-05-22T06:32:16Z

br/pkg/restore/misc.go

+		if parsed {
+			if filterOutFn(restoreCommitTs, snapshotBackupTs) {
+				return nil
+			}
+		}
+		filenames = append(filenames, path)


if not parsed, why still need append this file?

I'm worried that some external storage make the filename dirty.

3pointer · 2025-05-22T06:34:26Z

br/pkg/restore/misc.go

+			if err != nil {
+				return errors.Trace(err)
+			}
+			if filterOutFn(restoreCommitTs, snapshotBackupTs) {


it seems a little bit duplicated?

3pointer · 2025-05-22T06:38:19Z

br/pkg/restore/misc.go

+				return errors.Errorf(
+					"cannot restore the table(Id=%d) because it is restored(at %d) before snapshot backup(at %d). "+
+						"Please respecify the filter that does not contain the table or replace with a newer snapshot backup.",
+					tableId, restoreCommitTs, startTs)


It's better to record more table info(like table name), do we have chance to make this log more easy to debug?

3pointer · 2025-05-22T06:44:05Z

br/pkg/task/restore.go

+			return
+		}
+		tableIds := make([]int64, 0, len(cfg.PiTRTableTracker.TableIdToDBIds))
+		for tableId := range cfg.PiTRTableTracker.TableIdToDBIds {


should we handle PartitionIds here?

to handle the exchange partition ddl. Maybe it is now a partition in this PITR restore.

ti-chi-bot · 2025-05-22T06:44:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 3pointer

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~~ [3pointer]

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 · 2025-05-22T06:44:30Z

[LGTM Timeline notifier]

Timeline:

2025-05-22 06:44:30.051607501 +0000 UTC m=+74774.468667059: ☑️ agreed by 3pointer.

Signed-off-by: Jianjun Liao <[email protected]>

ti-chi-bot · 2025-05-22T11:06:55Z

@Leavrth: 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
idc-jenkins-ci-tidb/unit-test	`f25e044`	link	true	`/test unit-test`

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.

Leavrth added 2 commits May 21, 2025 16:50

compatibility of log backup and log restore

e3760d2

Signed-off-by: Jianjun Liao <[email protected]>

bazel prepare

1688190

Signed-off-by: Jianjun Liao <[email protected]>

ti-chi-bot bot added do-not-merge/needs-tests-checked release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed do-not-merge/needs-tests-checked labels May 21, 2025

BornChanger reviewed May 21, 2025

View reviewed changes

br/pkg/restore/misc.go Outdated Show resolved Hide resolved

Tristan1900 reviewed May 22, 2025

View reviewed changes

add integration test

690592d

Signed-off-by: Jianjun Liao <[email protected]>

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 22, 2025

3pointer approved these changes May 22, 2025

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels May 22, 2025

Leavrth added 4 commits May 22, 2025 15:30

fix integration test

5efdadd

Signed-off-by: Jianjun Liao <[email protected]>

fix integration test

fc6fe64

Signed-off-by: Jianjun Liao <[email protected]>

fix integration test

bd92204

Signed-off-by: Jianjun Liao <[email protected]>

fix integration test

f25e044

Signed-off-by: Jianjun Liao <[email protected]>

BornChanger mentioned this pull request May 22, 2025

br: add table filter for Pitr restore #57613

Open

15 tasks

br: compatibility of log backup and log restore #61238

Are you sure you want to change the base?

br: compatibility of log backup and log restore #61238

Uh oh!

Conversation

Leavrth commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

Uh oh!

tiprow bot commented May 21, 2025

Uh oh!

codecov bot commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

BornChanger commented May 21, 2025

Uh oh!

tiprow bot commented May 21, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Tristan1900 May 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

3pointer left a comment

Choose a reason for hiding this comment

Uh oh!

3pointer May 22, 2025

Choose a reason for hiding this comment

Uh oh!

Leavrth May 22, 2025

Choose a reason for hiding this comment

Uh oh!

3pointer May 22, 2025

Choose a reason for hiding this comment

Uh oh!

3pointer May 22, 2025

Choose a reason for hiding this comment

Uh oh!

3pointer May 22, 2025

Choose a reason for hiding this comment

Uh oh!

Leavrth May 22, 2025

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented May 22, 2025

Uh oh!

ti-chi-bot bot commented May 22, 2025

[LGTM Timeline notifier]

Uh oh!

ti-chi-bot bot commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Leavrth commented May 21, 2025 •

edited

Loading

codecov bot commented May 21, 2025 •

edited

Loading

ti-chi-bot bot commented May 22, 2025 •

edited

Loading