br: enable parallel restore #58724

Tristan1900 · 2025-01-06T18:15:23Z

What problem does this PR solve?

Issue Number: close #58725

Problem Summary:

What changed and how does it work?

To allow parallel restore tasks, we introduced a restore id for each task, the id is allocated by an internal TiDB registry table and it's monotonic increasing, the table is in mysql db called tide_restore_registry
We add this restore id to the suffix of checkpoint DB and storage, so each task can have their own checkpoint and not messing with others
For each new task, it registers itself to the registry, and make sure tables it's going to restore is not overlapping with restore task that has a smaller restore id. If there is conflict it will abort the restore and show the reason.
After finishing the restore, it will unregister itself from the registry
In case of failure, for a graceful one, it will change the task status in registry from running to pause, so next time user can retry and reusing the checkpoint. If disaster happens like OOM that BR didn't get a chance to pause, user needs to manually clean it up. We have a heartbeat mechanism that update the restore task timestamp, it can provide some hints whether this task is active or orphaned.

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

ti-chi-bot · 2025-01-06T18:15:26Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

tiprow · 2025-01-06T18:15:41Z

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.

codecov · 2025-01-06T22:51:08Z

Codecov Report

Attention: Patch coverage is 60.94771% with 239 lines in your changes missing coverage. Please review.

Project coverage is 74.9514%. Comparing base (35e7667) to head (d283e4b).
Report is 9 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #58724        +/-   ##
================================================
+ Coverage   73.1938%   74.9514%   +1.7576%     
================================================
  Files          1726       1751        +25     
  Lines        479561     490105     +10544     
================================================
+ Hits         351009     367341     +16332     
+ Misses       107030      99725      -7305     
- Partials      21522      23039      +1517

Flag	Coverage Δ
integration	`48.7958% <60.6209%> (?)`
unit	`72.3310% <6.5359%> (-0.1279%)`	⬇️

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

Components	Coverage Δ
dumpling	`52.7804% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`62.5304% <60.9917%> (+15.1004%)`	⬆️

🚀 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-03-27T22:48:26Z

/retest

tiprow · 2025-03-27T22:48:50Z

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

Tristan1900 · 2025-03-28T01:03:34Z

/retest

tiprow · 2025-03-28T01:03:59Z

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

Copilot

Copilot reviewed 14 out of 21 changed files in this pull request and generated no comments.

Files not reviewed (7)

br/pkg/registry/BUILD.bazel: Language not supported
br/pkg/restore/BUILD.bazel: Language not supported
br/pkg/task/BUILD.bazel: Language not supported
br/tests/br_parallel_restore/run.sh: Language not supported
br/tests/br_pitr/run.sh: Language not supported
br/tests/br_restore_checkpoint/run.sh: Language not supported
br/tests/run_group_br_tests.sh: Language not supported

Comments suppressed due to low confidence (1)

br/pkg/utils/schema.go:34

The comment contains a typo ('wheterh'); it should be corrected to 'whether'.

// IsTemplateSysDB checks wheterh the dbname is temporary system database(__TiDB_BR_Temporary_mysql or __TiDB_BR_Temporary_sys).

br/pkg/registry/registration.go

Leavrth · 2025-04-11T09:21:03Z

br/pkg/registry/registration.go

+			filter_strings(255),
+			start_ts,
+			restored_ts,
+			upstream_cluster_id,
+			with_sys_table,
+			cmd(255)


for cmd column, maybe it is better to use enum type?
for filter_strings, I think 255 is not long enough for type Text.

ERROR 1062 (23000): Duplicate entry '1234567890123456789012345678901234567890123456789012345678901234' for key 't1.unique_registration_params'

Do you actually want KEY instead of UNIQUE KEY?

Leavrth · 2025-04-11T09:35:12Z

br/pkg/registry/registration.go

+		AND upstream_cluster_id = %%?
+		AND with_sys_table = %%?
+		AND cmd = %%?
+		ORDER BY id DESC LIMIT 1`


no need limit 1 because there must be only one row in the final result.

Leavrth · 2025-04-11T09:36:23Z

br/pkg/registry/registration.go

+		}
+		// task exists but was either created by another process or has been running
+		// check if it's in running state
+		runningRows, _, err := execCtx.ExecRestrictedSQL(


Is it necessary to select again? runningRows[0] must be the same as rows[0].

ah yes you are right

br/pkg/registry/registration.go

Leavrth · 2025-04-11T10:01:17Z

br/pkg/task/restore.go

+		if hasCheckpointPersisted(c, cfg) {
+			log.Info("pausing restore task from registry",
+				zap.Uint64("restoreId", cfg.RestoreID), zap.Error(restoreError))
+			if err := restoreRegistry.PauseTask(c, cfg.RestoreID); err != nil {


What if BR is killed because OOM? BR might not have chance to pause the task.

good call, let me think about that

Leavrth · 2025-04-11T10:03:09Z

br/pkg/registry/registration.go

+		return 0, errors.Annotatef(err, "failed to create new registration")
+	}
+
+	// check if a row with our parameters exists


maybe after introduce a txn (begin ... commit), BR does not need to check again.

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

Tristan1900 · 2025-06-02T22:10:09Z

/retest

tiprow · 2025-06-02T22:10:32Z

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

benmeadowcroft · 2025-06-02T22:48:09Z

pkg/session/bootstrap.go

+		with_sys_table BOOLEAN NOT NULL DEFAULT TRUE,
+		status VARCHAR(20) NOT NULL DEFAULT 'running',
+		cmd TEXT,
+		start_timestamp BIGINT UNSIGNED NOT NULL,


From a user perspective I find the column naming confusing.

First, it is not clear how this is different from start_ts referenced above. Can you clarifying the naming here? If they are used for different things, is the a better name that can be used to make it more obvious? Or is this just intended to offer different levels of precision (if so why the same data type)?

Second, the suffix of the column name _timestamp implies this is aTIMESTAMP while the type is specified as BIGINT. This is confusing and inconsistent.

thanks for taking a look!
yes you are absolutely right the name is confusing, let me rename the latter one.

pkg/session/bootstrap.go

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

windtalker

expression/executor part lgtm

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

ti-chi-bot · 2025-06-03T02:00:53Z

[LGTM Timeline notifier]

Timeline:

2025-06-02 22:50:03.041854978 +0000 UTC m=+309289.244200244: ✖️🔁 reset by benmeadowcroft.
2025-06-03 01:17:40.517597771 +0000 UTC m=+318146.719943037: ☑️ agreed by windtalker.
2025-06-03 02:00:52.990407044 +0000 UTC m=+320739.192752308: ☑️ agreed by benmeadowcroft.

D3Hunter · 2025-06-03T02:29:09Z

/approve

ti-chi-bot · 2025-06-03T02:33:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benmeadowcroft, D3Hunter, Leavrth, windtalker, winoros

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:

~~OWNERS~~ [D3Hunter,Leavrth,windtalker,winoros]
~~br/OWNERS~~ [Leavrth]
~~pkg/executor/importer/OWNERS~~ [D3Hunter]
~~pkg/expression/OWNERS~~ [windtalker]
~~pkg/session/OWNERS~~ [benmeadowcroft]
~~pkg/statistics/OWNERS~~ [winoros]

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

winoros · 2025-06-03T02:34:08Z

for planner part

close pingcap#58725

ti-chi-bot bot removed do-not-merge/needs-linked-issue do-not-merge/needs-tests-checked labels Jan 6, 2025

Tristan1900 marked this pull request as ready for review January 6, 2025 22:25

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 6, 2025

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 Jan 6, 2025

Tristan1900 force-pushed the concurent-restore branch from c089404 to 13e1419 Compare March 19, 2025 21:42

Tristan1900 changed the title ~~br: use dedicated checkpoint table for each restore task~~ br: enable parallel restore Mar 19, 2025

Tristan1900 force-pushed the concurent-restore branch 4 times, most recently from d6875ff to d33d597 Compare March 27, 2025 02:44

Tristan1900 force-pushed the concurent-restore branch from 973c93a to 2854159 Compare March 27, 2025 22:54

Leavrth requested a review from Copilot April 11, 2025 08:27

Copilot AI reviewed Apr 11, 2025

View reviewed changes

Leavrth reviewed Apr 11, 2025

View reviewed changes

Tristan1900 force-pushed the concurent-restore branch from cfed383 to 9d64c1d Compare May 21, 2025 20:05

Tristan1900 added 2 commits June 1, 2025 23:42

one final fix

ab5cc78

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

looks like a lot more unit test are impacted by adding a new table

c360107

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

ti-chi-bot bot added the sig/planner SIG: Planner label Jun 2, 2025

Tristan1900 added 4 commits June 2, 2025 11:39

one more

90fa6fa

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

one more...

8ddcc12

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

uncomment test, minor tweaks

7117a46

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

cleanup

48bf9da

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

benmeadowcroft suggested changes Jun 2, 2025

View reviewed changes

Tristan1900 force-pushed the concurent-restore branch from d189487 to b27fd04 Compare June 3, 2025 00:12

address naming comments

026aa8c

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

Tristan1900 force-pushed the concurent-restore branch from b27fd04 to 026aa8c Compare June 3, 2025 00:18

windtalker approved these changes Jun 3, 2025

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jun 3, 2025

missed change to rename, and make test stable

d283e4b

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

benmeadowcroft approved these changes Jun 3, 2025

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jun 3, 2025

Leavrth approved these changes Jun 3, 2025

View reviewed changes

winoros approved these changes Jun 3, 2025

View reviewed changes

ti-chi-bot bot added the approved label Jun 3, 2025

ti-chi-bot bot merged commit fa3c79b into pingcap:master Jun 3, 2025
34 of 39 checks passed

Tristan1900 added a commit to Tristan1900/tidb that referenced this pull request Jun 6, 2025

br: enable parallel restore (pingcap#58724)

9c9ee6e

close pingcap#58725

Leavrth mentioned this pull request Jun 6, 2025

br: cherry pick to hot fix release 8.5 20250114 v8.5.0 second batch | tidb-test=release-8.5.1 #61534

Merged

13 tasks

joechenrh pushed a commit to joechenrh/tidb that referenced this pull request Jun 10, 2025

br: enable parallel restore (pingcap#58724)

9205605

close pingcap#58725

br: enable parallel restore #58724

br: enable parallel restore #58724

Uh oh!

Conversation

Tristan1900 commented Jan 6, 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!

ti-chi-bot bot commented Jan 6, 2025

Uh oh!

tiprow bot commented Jan 6, 2025

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Tristan1900 commented Mar 27, 2025

Uh oh!

tiprow bot commented Mar 27, 2025

Uh oh!

Tristan1900 commented Mar 28, 2025

Uh oh!

tiprow bot commented Mar 28, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tristan1900 commented Jun 2, 2025

Uh oh!

tiprow bot commented Jun 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

windtalker left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Jun 3, 2025

[LGTM Timeline notifier]

Uh oh!

D3Hunter commented Jun 3, 2025

Uh oh!

ti-chi-bot bot commented Jun 3, 2025

Uh oh!

winoros commented Jun 3, 2025

Uh oh!

Uh oh!

Uh oh!

Tristan1900 commented Jan 6, 2025 •

edited

Loading

codecov bot commented Jan 6, 2025 •

edited

Loading