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?

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.34816% with 205 lines in your changes missing coverage. Please review.

Project coverage is 74.6872%. Comparing base (802eb47) to head (ad82903).
Report is 17 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #58724        +/-   ##
================================================
+ Coverage   73.1437%   74.6872%   +1.5435%     
================================================
  Files          1726       1728         +2     
  Lines        478033     484124      +6091     
================================================
+ Hits         349651     361579     +11928     
+ Misses       106948     100423      -6525     
- Partials      21434      22122       +688

Flag	Coverage Δ
integration	`45.6031% <60.3481%> (?)`

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

Components	Coverage Δ
dumpling	`52.7804% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`59.8912% <60.3481%> (+12.3270%)`	⬆️

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

Leavrth · 2025-04-11T08:47:19Z

br/pkg/registry/registration.go

@@ -0,0 +1,523 @@
+// Copyright 2025 PingCAP, Inc.


It is better to add unit tests for the package

let me see what I can do, it's calling executor so not sure unit test is a good choice and is testing what we want
use testKit

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

Leavrth · 2025-04-11T09:49:39Z

br/pkg/registry/registration.go

+
+// ResumeOrCreateRegistration first looks for an existing registration with the given parameters.
+// If found and paused, it tries to resume it. Otherwise, it creates a new registration.
+func (r *Registry) ResumeOrCreateRegistration(ctx context.Context, info RegistrationInfo) (uint64, error) {


wrap by

r.se.ExecuteInternal("begin pessimistic;") ... r.se.ExecuteInternal("commit/rollback;")

That was my initial attempt but had to fall back to current version of using multiple transaction, maybe we can discuss offline cuz based on my observation ExecRestrictedSQL doesn't work with nested transaction, it appears to be a single transaction for admin purpose. But maybe I'm wrong.

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]>

ti-chi-bot · 2025-05-21T20:06:03Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign leavrth for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

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

Needs approval from an approver in each of these files:

OWNERS
br/OWNERS

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

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

ti-chi-bot · 2025-05-23T01:10:53Z

@Tristan1900: The following tests 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/build	`ad82903`	link	true	`/test build`
idc-jenkins-ci-tidb/unit-test	`ad82903`	link	true	`/test unit-test`
pull-br-integration-test	`ad82903`	link	true	`/test pull-br-integration-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.

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

first version

4940c6f

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

Tristan1900 added 6 commits May 20, 2025 13:43

fix tests

39f9667

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

fix test

4dc2fe5

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

some more fix

216ba1f

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

fix unit test

692e052

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

some more fix

98ed4d9

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

fix unit test

0381199

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

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

BornChanger mentioned this pull request May 22, 2025

br: add table filter for Pitr restore #57613

Open

15 tasks

fix some test issues, add heartbeat, use one single trasnaction

ad82903

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

Tristan1900 force-pushed the concurent-restore branch from 9d64c1d to ad82903 Compare May 23, 2025 00:14

br: enable parallel restore #58724

Are you sure you want to change the base?

br: enable parallel restore #58724

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!

Choose a reason for hiding this comment

Uh oh!

Tristan1900 Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tristan1900 Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ti-chi-bot bot commented May 21, 2025

Uh oh!

ti-chi-bot bot commented May 23, 2025

Uh oh!

Uh oh!

Tristan1900 commented Jan 6, 2025 •

edited

Loading

codecov bot commented Jan 6, 2025 •

edited

Loading

Tristan1900 Apr 14, 2025 •

edited

Loading

Tristan1900 Apr 14, 2025 •

edited

Loading