br: copy full backup to pitr storage #57716

YuJuncen · 2024-11-26T09:50:22Z

What problem does this PR solve?

Issue Number: close #58685

Problem Summary:
Now, a log backup task cannot back full restorations happen in the cluster up.

What changed and how does it work?

This PR makes BR upload the SSTs it imported to the log backup storage.

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: hillium <yujuncen@pingcap.com>

tiprow · 2024-11-26T09:50:39Z

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

Signed-off-by: hillium <yujuncen@pingcap.com>

BornChanger · 2025-01-15T11:06:33Z

/retest

tiprow · 2025-01-15T11:06:56Z

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

BornChanger · 2025-01-15T11:20:55Z

br/pkg/restore/snap_client/pitr_collector.go

@@ -0,0 +1,487 @@
+package snapclient


missing license header？

BornChanger · 2025-01-15T11:21:11Z

br/pkg/restore/snap_client/pitr_collector_test.go

@@ -0,0 +1,261 @@
+package snapclient


missing license header？

Signed-off-by: hillium <yu745514916@live.com>

ti-chi-bot · 2025-01-15T14:36:43Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [3pointer,BornChanger]
~~br/OWNERS~~ [3pointer,BornChanger]
~~br/pkg/storage/OWNERS~~ [3pointer,BornChanger]

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-01-15T14:36:47Z

[LGTM Timeline notifier]

Timeline:

2025-01-15 09:32:53.752043993 +0000 UTC m=+111045.207090143: ☑️ agreed by BornChanger.
2025-01-15 14:36:45.94279874 +0000 UTC m=+129277.397844889: ☑️ agreed by 3pointer.

tiprow · 2025-01-15T15:05:02Z

@YuJuncen: 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
fast_test_tiprow	`50b70a4`	link	true	`/test fast_test_tiprow`

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.

Signed-off-by: hillium <yu745514916@live.com>

BornChanger · 2025-01-15T15:47:08Z

/retest

tiprow · 2025-01-15T15:47:39Z

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

Tristan1900

lgtm overall, just a bunch of nit comments

Tristan1900 · 2025-01-13T17:00:12Z

pkg/metrics/br.go

+		Subsystem: "br",
+		Name:      "restore_import_file_seconds",
+
+		Help: "The time cost for importing a file. (including the time costed in queuing)",


typo: cost is same for present and past tense.

Tristan1900 · 2025-01-13T17:13:57Z

br/pkg/summary/summary.go

 	collector.SetSuccessStatus(success)
 }

+// Succeed returns whether the last call to `SetSuccessStatus` passes `true`.
+func Succeed() bool {


typo? success

Tristan1900 · 2025-01-15T16:39:11Z

br/pkg/errors/errors.go

@@ -40,6 +40,7 @@ var (
 	ErrEnvNotSpecified              = errors.Normalize("environment variable not found", errors.RFCCodeText("BR:Common:ErrEnvNotSpecified"))
 	ErrUnsupportedOperation         = errors.Normalize("the operation is not supported", errors.RFCCodeText("BR:Common:ErrUnsupportedOperation"))
 	ErrInvalidRange                 = errors.Normalize("invalid restore range", errors.RFCCodeText("BR:Common:ErrInvalidRange"))
+	ErrMigrationNotFound            = errors.Normalize("no migrtion found", errors.RFCCodeText("BR:Common:ErrMigrationNotFound"))


typo migrtion in error message

Tristan1900 · 2025-01-15T16:49:05Z

br/pkg/glue/glue.go

@@ -82,3 +83,22 @@ type Progress interface {
 	// called.
 	Close()
 }
+
+type CounterProgress struct {


looks like this is only used in test, should we move it to the test file instead?

Tristan1900 · 2025-01-15T16:50:46Z

br/pkg/gluetidb/glue.go

@@ -55,6 +55,10 @@ type Glue struct {
 	startDomainMu *sync.Mutex
 }

+func WrapSession(se sessiontypes.Session) glue.Session {


also only used in test, should we move it?

We cannot move this to test file because this function uses the private structure tidbSession. This function is somehow a public constructor of tidbSession.

Tristan1900 · 2025-01-15T17:00:40Z

pkg/metrics/metrics.go

@@ -98,6 +98,10 @@ func InitMetrics() {
 	InitInfoSchemaV2Metrics()
 	timermetrics.InitTimerMetrics()

+	// For now, those metrics are initialized but not registered.


it's a bit weird to print metrics instead of integrating it into the existing dashboard, is it because br cannot publish stats to prometheus?

Yes, perhaps we need a Push Gateway... Raw Prometheus isn't pretty friendly to batch operations...

Tristan1900 · 2025-01-15T17:21:21Z

br/pkg/storage/locking.go

+	const JitterMs = 5000
+
+	retry := utils.InitialRetryState(math.MaxInt, 1*time.Second, 60*time.Second)
+	jitter := time.Duration(rand.Uint32()%JitterMs+(JitterMs/2)) * time.Millisecond


the jitter is a bit weird, looks like it's always going to be above 2500? I guess why don't we set jitter to be 2500 and just do rand.Uint32()%JitterMs

also can move this to a method in the retry module, feels like going to be needed in many case so don't need to rewrite the jitter calculation every time

Tristan1900 · 2025-01-15T17:38:49Z

br/pkg/restore/log_client/migration.go

@@ -242,3 +252,18 @@ func (wm *WithMigrations) Compactions(ctx context.Context, s storage.ExternalSto
 		return Subcompactions(ctx, name, s)
 	})
 }
+
+func (wm *WithMigrations) IngestedSSTs(ctx context.Context, s storage.ExternalStorage) iter.TryNextor[*backuppb.IngestedSSTs] {


one question, IDE has warning about the style

Struct WithMigrations has methods on both value and pointer receivers. Such usage is not recommended by the Go Documentation.

and for many other structs as well. I'm wondering if we are doing impl for both on purpose or by accident?

Most of times are by accident. A typical scenario is that a structure was treated as value type (which has only value receivers) at the beginning, but with developing, we have noticed that sometimes it need to be a reference type (say, needing mutate its content by a method), then a method with pointer was added.

Tristan1900 · 2025-01-15T17:45:51Z

br/pkg/restore/snap_client/pitr_collector.go

+	}
+}
+
+type pitrCollector struct {


can add some comment to this struct since its logic is important to the whole feature.

Tristan1900 · 2025-01-15T17:54:53Z

br/pkg/restore/log_client/ssts.go

+	s.SstOutputs = files
+}
+
+type AddedSSTs struct {


maybe add some comment to this struct.
also from the logic looks like it represents one sst? can we drop the s in the name as it's suggesting the other way

Signed-off-by: hillium <yu745514916@live.com>

YuJuncen · 2025-01-16T01:57:04Z

/hold

Signed-off-by: hillium <yu745514916@live.com>

YuJuncen · 2025-01-16T03:06:32Z

/unhold

Signed-off-by: hillium <yu745514916@live.com>

close pingcap#58685

YuJuncen added 3 commits November 25, 2024 16:51

initial commit

a9861df

Signed-off-by: hillium <yujuncen@pingcap.com>

added basic implementation

f70831f

Signed-off-by: hillium <yujuncen@pingcap.com>

initial commit

addb8f7

Signed-off-by: hillium <yujuncen@pingcap.com>

ti-chi-bot bot added do-not-merge/needs-linked-issue do-not-merge/needs-tests-checked release-note-none Denotes a PR that doesn't merit a release note. labels Nov 26, 2024

YuJuncen marked this pull request as draft November 26, 2024 09:50

ti-chi-bot bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 26, 2024

YuJuncen and others added 16 commits November 27, 2024 14:41

added basic test toolkit

e00d5c1

Signed-off-by: hillium <yujuncen@pingcap.com>

added test cases

f91c7ca

Signed-off-by: hillium <yujuncen@pingcap.com>

added force-flush to speed up testing

651b9b5

Signed-off-by: hillium <yujuncen@pingcap.com>

added checksum table

ec98b3e

make checksum work

2dfc800

Signed-off-by: hillium <yujuncen@pingcap.com>

adapt the new proto

c16d21b

make it compile (but not work)

d02c216

fix migration not uploaded

afad018

Signed-off-by: hillium <yujuncen@pingcap.com>

try filter out full backup

2dfdb30

persist progress when exit

9fc057e

initial try to hook at importer

a512330

fix incorrect state

a734f24

shouldn't put finished when failed to commit

eac139e

added more test cases

9a0659d

make names more readable

ed34ea5

reject restore if there is a task for full restore

6d464c3

Signed-off-by: hillium <yujuncen@pingcap.com>

YuJuncen mentioned this pull request Dec 10, 2024

import: allow log backup to load ingested SSTs. tikv/tikv#17819

Closed

9 tasks

YuJuncen added 3 commits December 12, 2024 16:00

added more test cases

6dd3946

try not allow user when local encryption enabled, add progress for SSTs

547e192

make a more friendly error message

9e0dfd7

YuJuncen added 2 commits January 15, 2025 17:56

added license

9454133

fix test

b94efc4

pingcap deleted a comment from tiprow bot Jan 15, 2025

BornChanger reviewed Jan 15, 2025

View reviewed changes

br/pkg/restore/snap_client/pitr_collector.go

@@ -0,0 +1,487 @@

package snapclient

Copy link

Contributor

BornChanger Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing license header？

BornChanger reviewed Jan 15, 2025

View reviewed changes

br/pkg/restore/snap_client/pitr_collector_test.go

@@ -0,0 +1,261 @@

package snapclient

Copy link

Contributor

BornChanger Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing license header？

YuJuncen added 2 commits January 15, 2025 20:52

make license checker happy

00c516f

fix more cases

50b70a4

Signed-off-by: hillium <yu745514916@live.com>

3pointer approved these changes Jan 15, 2025

View reviewed changes

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

fix real tikv test

7b66379

Signed-off-by: hillium <yu745514916@live.com>

Tristan1900 reviewed Jan 15, 2025

View reviewed changes

fixed test

74ceaa1

Signed-off-by: hillium <yu745514916@live.com>

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

address comments

7b2989c

Signed-off-by: hillium <yu745514916@live.com>

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

fix test (really)

9ce1a98

Signed-off-by: hillium <yu745514916@live.com>

ti-chi-bot bot merged commit c9215ec into pingcap:master Jan 16, 2025
29 checks passed

YuJuncen added a commit to YuJuncen/tidb that referenced this pull request Jan 16, 2025

br: copy full backup to pitr storage (pingcap#57716)

fac1ffe

close pingcap#58685

YuJuncen mentioned this pull request Jan 16, 2025

br: Release 8.5 20250114 v8.5.0 #58981

Merged

13 tasks

YuJuncen added a commit to YuJuncen/tidb that referenced this pull request Jan 17, 2025

br: copy full backup to pitr storage (pingcap#57716)

e5113d2

close pingcap#58685

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

br: copy full backup to pitr storage #57716

br: copy full backup to pitr storage #57716

YuJuncen commented Nov 26, 2024 •

edited

Loading

tiprow bot commented Nov 26, 2024

BornChanger commented Jan 15, 2025

tiprow bot commented Jan 15, 2025

BornChanger Jan 15, 2025

BornChanger Jan 15, 2025

ti-chi-bot bot commented Jan 15, 2025

ti-chi-bot bot commented Jan 15, 2025

tiprow bot commented Jan 15, 2025

BornChanger commented Jan 15, 2025

tiprow bot commented Jan 15, 2025

Tristan1900 left a comment •

edited

Loading

Tristan1900 Jan 13, 2025

Tristan1900 Jan 13, 2025

Tristan1900 Jan 15, 2025

Tristan1900 Jan 15, 2025

Tristan1900 Jan 15, 2025

YuJuncen Jan 16, 2025

Tristan1900 Jan 15, 2025

YuJuncen Jan 16, 2025

Tristan1900 Jan 15, 2025

Tristan1900 Jan 15, 2025

YuJuncen Jan 16, 2025

Tristan1900 Jan 15, 2025

Tristan1900 Jan 15, 2025

YuJuncen commented Jan 16, 2025

YuJuncen commented Jan 16, 2025

br: copy full backup to pitr storage #57716

br: copy full backup to pitr storage #57716

Conversation

YuJuncen commented Nov 26, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Nov 26, 2024

BornChanger commented Jan 15, 2025

tiprow bot commented Jan 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Jan 15, 2025

ti-chi-bot bot commented Jan 15, 2025

[LGTM Timeline notifier]

tiprow bot commented Jan 15, 2025

BornChanger commented Jan 15, 2025

tiprow bot commented Jan 15, 2025

Tristan1900 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YuJuncen commented Jan 16, 2025

YuJuncen commented Jan 16, 2025

YuJuncen commented Nov 26, 2024 •

edited

Loading

Tristan1900 left a comment •

edited

Loading