br/stream: Added toolkit for managing migrations #55665

YuJuncen · 2024-08-26T09:33:18Z

What problem does this PR solve?

Issue Number: close #55661

Problem Summary:
See the issue.

What changed and how does it work?

We added some new interfaces that helps to manipulate migrations. They are provided by a wrapper over ExternalStorage named MigrationExt.

MergeAndMigrateTo is the main interface for executing pending migrations.
Load loads the persisted migrations.

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

tiprow · 2024-08-26T09:33: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.

codecov · 2024-08-26T09:51:35Z

Codecov Report

Attention: Patch coverage is 9.33165% with 719 lines in your changes missing coverage. Please review.

Project coverage is 75.5147%. Comparing base (b520f61) to head (6ce016f).
Report is 16 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #55665        +/-   ##
================================================
+ Coverage   73.3867%   75.5147%   +2.1280%     
================================================
  Files          1620       1643        +23     
  Lines        447041     455667      +8626     
================================================
+ Hits         328069     344096     +16027     
+ Misses        98877      89986      -8891     
- Partials      20095      21585      +1490

Flag	Coverage Δ
integration	`32.5841% <9.3316%> (?)`

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

Components	Coverage Δ
dumpling	`52.9478% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`61.6396% <9.3316%> (+16.1103%)`	⬆️

Leavrth · 2024-09-10T10:17:44Z

br/pkg/storage/local.go

+	err := os.Remove(path)
+	if err != nil &&
+		l.IgnoreEnoentForDelete &&
+		stderrors.Is(err, syscall.ENOENT) {


local external storage also support windows, so can syscall.ENOENT also match the error occur in windows system?

br/pkg/storage/local.go

Leavrth · 2024-09-12T09:43:34Z

br/pkg/stream/stream_metas.go

+	HandingMetaEditDone()
+}
+
+func NewProgressBarHooks(console glue.ConsoleOperations) *ProgressBarHooks {


is it dead code? or will be called outside?

Leavrth · 2024-09-12T10:19:28Z

br/pkg/stream/stream_metas.go

+
+			me := new(pb.MetaEdit)
+			me.Path = path
+			for _, ds := range meta.FileGroups {


should we also append the me.DeleteLogicalFiles because m.applyMetaEditTo uses it?

Leavrth · 2024-09-12T10:26:39Z

br/pkg/stream/stream_metas.go

+
+// MergeAndMigrateTo will merge the migrations from BASE until the specified SN, then migrate to it.
+// Finally it writes the new BASE and remove stale migrations from the storage.
+func (m MigrationExt) MergeAndMigrateTo(


I found there are many functions only used in unit test, so I prefer to move these functions to another file with suffix _test.go to read and check codes for production more clearly.

YuJuncen · 2024-09-27T08:56:54Z

/retest

tiprow · 2024-09-27T08:57:18Z

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

YuJuncen · 2024-09-27T09:09:40Z

/ok-to-test

tiprow · 2024-09-27T09:12:56Z

@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	`6ce016f`	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.

ti-chi-bot · 2024-09-27T09:37:04Z

@YuJuncen: 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	`6ce016f`	link	true	`/test build`
idc-jenkins-ci-tidb/check_dev	`6ce016f`	link	true	`/test check-dev`
idc-jenkins-ci-tidb/unit-test	`6ce016f`	link	true	`/test unit-test`
idc-jenkins-ci-tidb/check_dev_2	`6ce016f`	link	true	`/test check-dev2`

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/test-infra repository. I understand the commands that are listed here.

Leavrth

rest LGTM. There are some functions used by another PR, so need to review again with the PR.

Leavrth · 2024-09-26T07:39:14Z

br/pkg/stream/stream_metas.go

+			if err != nil {
+				emitErr(cannotBeRetryByRerunBase(
+					errors.Annotatef(err, "failed to open meta %s", path)))
+				return


should we limit the error number?

Leavrth · 2024-09-29T06:08:31Z

br/pkg/stream/stream_metas.go

+	for _, edit := range s2 {
+		target, ok := edits[edit.GetPath()]
+		if !ok {
+			edits[edit.GetPath()] = edit


Should we also create a new &pb.MetaEdit the same as edit in s1?

Leavrth · 2024-09-29T06:41:13Z

br/pkg/stream/stream_metas.go

+			err = m.applyMetaEditTo(ctx, me, meta)
+			if err != nil {
+				updateResult(func(r *MigratedTo) {
+					r.Warnings = append(r.Warnings, errors.Annotatef(err, "during handling %s", me.Path))
+					r.NewBase.EditMeta = append(r.NewBase.EditMeta, me)
+				})
+			}
+			m.cleanUpFor(ctx, me, out)


How about clean up log files before clean up metadata? Maybe the log files will be left in the external storage forever.

ti-chi-bot · 2024-09-29T06:51:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Leavrth

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~~ [Leavrth]
~~br/pkg/storage/OWNERS~~ [Leavrth]

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 · 2024-09-29T06:51:55Z

[LGTM Timeline notifier]

Timeline:

2024-09-29 06:51:55.745135737 +0000 UTC m=+164871.165349008: ☑️ agreed by Leavrth.

ti-chi-bot bot added do-not-merge/invalid-title release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 26, 2024

YuJuncen changed the title ~~Added toolkit for managing~~ br/stream: Added toolkit for managing Aug 26, 2024

ti-chi-bot bot removed the do-not-merge/invalid-title label Aug 26, 2024

YuJuncen changed the title ~~br/stream: Added toolkit for managing~~ br/stream: Added toolkit for managing migrations Aug 26, 2024

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 6, 2024

YuJuncen force-pushed the mig-truncating-split2 branch from 095f623 to e183b20 Compare September 9, 2024 08:07

Leavrth reviewed Sep 10, 2024

View reviewed changes

Leavrth reviewed Sep 12, 2024

View reviewed changes

added basic migration support

6ce016f

YuJuncen force-pushed the mig-truncating-split2 branch from 35c734a to 6ce016f Compare September 27, 2024 07:58

ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 27, 2024

ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label Sep 27, 2024

Leavrth approved these changes Sep 29, 2024

View reviewed changes

ti-chi-bot bot added the approved label Sep 29, 2024

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Sep 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

br/stream: Added toolkit for managing migrations #55665

br/stream: Added toolkit for managing migrations #55665

YuJuncen commented Aug 26, 2024

tiprow bot commented Aug 26, 2024

codecov bot commented Aug 26, 2024 •

edited

Loading

Leavrth Sep 10, 2024

Leavrth Sep 12, 2024

Leavrth Sep 12, 2024

Leavrth Sep 12, 2024

YuJuncen commented Sep 27, 2024

tiprow bot commented Sep 27, 2024

YuJuncen commented Sep 27, 2024

tiprow bot commented Sep 27, 2024

ti-chi-bot bot commented Sep 27, 2024

Leavrth left a comment

Leavrth Sep 26, 2024

Leavrth Sep 29, 2024

Leavrth Sep 29, 2024

ti-chi-bot bot commented Sep 29, 2024

ti-chi-bot bot commented Sep 29, 2024

br/stream: Added toolkit for managing migrations #55665

Are you sure you want to change the base?

br/stream: Added toolkit for managing migrations #55665

Conversation

YuJuncen commented Aug 26, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Aug 26, 2024

codecov bot commented Aug 26, 2024 • edited Loading

Codecov Report

Leavrth Sep 10, 2024

Choose a reason for hiding this comment

Leavrth Sep 12, 2024

Choose a reason for hiding this comment

Leavrth Sep 12, 2024

Choose a reason for hiding this comment

Leavrth Sep 12, 2024

Choose a reason for hiding this comment

YuJuncen commented Sep 27, 2024

tiprow bot commented Sep 27, 2024

YuJuncen commented Sep 27, 2024

tiprow bot commented Sep 27, 2024

ti-chi-bot bot commented Sep 27, 2024

Leavrth left a comment

Choose a reason for hiding this comment

Leavrth Sep 26, 2024

Choose a reason for hiding this comment

Leavrth Sep 29, 2024

Choose a reason for hiding this comment

Leavrth Sep 29, 2024

Choose a reason for hiding this comment

ti-chi-bot bot commented Sep 29, 2024

ti-chi-bot bot commented Sep 29, 2024

[LGTM Timeline notifier]

codecov bot commented Aug 26, 2024 •

edited

Loading