br: skip incomplete metakv by Leavrth · Pull Request #61734 · pingcap/tidb

Leavrth · 2025-06-13T13:32:27Z

What problem does this PR solve?

Issue Number: close #61728 close #61731

Problem Summary:
the incomplete metakv cannot be restored

What changed and how does it work?

skip the incomplete metakv

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 <jianjun.liao@outlook.com>

tiprow · 2025-06-13T13:32:47Z

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.

Details

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.

ti-chi-bot · 2025-06-13T13:33:41Z

@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
pull-integration-e2e-test-next-gen	`a3600c3`	link	false	`/test pull-integration-e2e-test-next-gen`

Full PR test history. Your PR dashboard.

Details

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.

codecov · 2025-06-13T13:50:27Z

Codecov Report

Attention: Patch coverage is 50.00000% with 39 lines in your changes missing coverage. Please review.

Project coverage is 74.9112%. Comparing base (3c7a2a2) to head (a684ec2).
Report is 7 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #61734        +/-   ##
================================================
+ Coverage   73.1141%   74.9112%   +1.7971%     
================================================
  Files          1730       1746        +16     
  Lines        481167     482774      +1607     
================================================
+ Hits         351801     361652      +9851     
+ Misses       107845      98872      -8973     
- Partials      21521      22250       +729

Flag	Coverage Δ
integration	`46.2443% <30.7692%> (?)`
unit	`72.5160% <38.4615%> (+0.1596%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.7804% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`62.8758% <50.0000%> (+15.8903%)`	⬆️

🚀 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.

Tristan1900 · 2025-06-13T15:58:47Z

br/pkg/restore/misc.go

 			// the meta kv may not be backed by log restore
-			if checkTableIDLost(tableId) {
+			if checkTableIdLost(tableId) {
+				log.Warn("the table is lost in the log backup storage, so that it can not be restored.", zap.Int64("table id", tableId))


maybe use Info level? as this is expected, and maybe rephrase the log message, I feel like it might be a bit concerning to see this log if people don't have context

Maybe the tableId is included in the --filter, and the user want to restore it. However, finally it is not restored.

Tristan1900 · 2025-06-13T16:01:37Z

br/pkg/restore/misc.go

+					dbId, dbNameByDbId(dbId), restoredTs, restoreCommitTs, startTs)
+			}
+			// the meta kv may not be backed by log restore
+			if checkDBIdlost(dbId) {


actually do we need this check since it can lost for a reason? how about just keep the tracker check

Tristan1900 · 2025-06-13T16:21:39Z

br/pkg/restore/misc.go

 	SnapshotBackupTs uint64 `protobuf:"varint,2,opt,name=snapshot_backup_ts,proto3"`
-	// TableIDs records the table IDs blocklist of the cluster running the log backup task.
+	// RewriteTs records the rewritten timestamp of the meta kvs in this PITR restore.
+	RewriteTs uint64 `protobuf:"varint,6,opt,name=rewrite_ts,proto3"`


hmm, what's the difference between RewriteTs and RestoreCommitTs

RestoreCommitTs > RewriteTs
Therefore, RestoreCommitTs is newer than the timestamp of any kv that is restored by log restore.

Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>

br/pkg/stream/table_mapping.go

Tristan1900 · 2025-06-16T12:43:07Z

/lgtm
/hold
for one unaddressed comment

ti-chi-bot · 2025-06-16T12:43:16Z

[LGTM Timeline notifier]

Timeline:

2025-06-16 11:15:53.924141777 +0000 UTC m=+97606.647320756: ☑️ agreed by 3pointer.
2025-06-16 12:43:15.685094265 +0000 UTC m=+102848.408273246: ☑️ agreed by Tristan1900.

ti-chi-bot · 2025-06-16T12:43:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 3pointer, Tristan1900

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

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

Tristan1900 · 2025-06-16T16:08:57Z

/unhold

Tristan1900 · 2025-06-16T17:27:55Z

/cherrypick release-8.5-20250114-v8.5.0

ti-chi-bot · 2025-06-16T17:28:38Z

@Tristan1900: new pull request created to branch release-8.5-20250114-v8.5.0: #61762.

Details

In response to this:

/cherrypick release-8.5-20250114-v8.5.0

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 ti-community-infra/tichi repository.

close #61728, close #61731

close pingcap#61728, close pingcap#61731

skip incomplete metakv

a3600c3

Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>

ti-chi-bot bot added do-not-merge/needs-tests-checked release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-triage-completed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed do-not-merge/needs-tests-checked labels Jun 13, 2025

ti-chi-bot bot removed the do-not-merge/needs-triage-completed label Jun 13, 2025

Tristan1900 reviewed Jun 13, 2025

View reviewed changes

BornChanger mentioned this pull request Jun 15, 2025

br: add table filter for Pitr restore #57613

Closed

29 tasks

add unit test

a684ec2

Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>

3pointer reviewed Jun 16, 2025

View reviewed changes

br/pkg/stream/table_mapping.go Show resolved Hide resolved

3pointer approved these changes Jun 16, 2025

View reviewed changes

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

ti-chi-bot bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jun 16, 2025

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 16, 2025

ti-chi-bot bot merged commit f46112d into pingcap:master Jun 16, 2025
31 of 33 checks passed

ti-chi-bot mentioned this pull request Jun 16, 2025

br: skip incomplete metakv (#61734) | tidb-test=release-8.5.1 #61762

Merged

13 tasks

ti-chi-bot bot pushed a commit that referenced this pull request Jun 17, 2025

br: skip incomplete metakv (#61734) (#61762)

4d2856c

close #61728, close #61731

Leavrth added a commit to Leavrth/tidb that referenced this pull request Dec 12, 2025

br: skip incomplete metakv (pingcap#61734)

57ef282

close pingcap#61728, close pingcap#61731

Leavrth mentioned this pull request Dec 12, 2025

br: deliver "pitr table filter" to release-8.5 #64908

Closed

Conversation

Leavrth commented Jun 13, 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 Jun 13, 2025

Uh oh!

ti-chi-bot bot commented Jun 13, 2025

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Tristan1900 Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

Leavrth Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

Tristan1900 Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

Tristan1900 Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

Leavrth Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Tristan1900 commented Jun 16, 2025

Uh oh!

ti-chi-bot bot commented Jun 16, 2025

[LGTM Timeline notifier]

Uh oh!

ti-chi-bot bot commented Jun 16, 2025

Uh oh!

Tristan1900 commented Jun 16, 2025

Uh oh!

Uh oh!

Tristan1900 commented Jun 16, 2025

Uh oh!

ti-chi-bot commented Jun 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Leavrth commented Jun 13, 2025 •

edited

Loading

codecov bot commented Jun 13, 2025 •

edited

Loading