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

Project coverage is 76.1217%. Comparing base (1e24d39) to head (16a6fa1).
Report is 7 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #55665        +/-   ##
================================================
+ Coverage   73.3721%   76.1217%   +2.7495%     
================================================
  Files          1626       1649        +23     
  Lines        448978     458651      +9673     
================================================
+ Hits         329425     349133     +19708     
+ Misses        99368      87722     -11646     
- Partials      20185      21796      +1611

Flag	Coverage Δ
integration	`51.7662% <12.0145%> (?)`
unit	`72.4829% <65.6553%> (+0.0003%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.9478% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`63.1976% <65.6553%> (+17.6076%)`	⬆️

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?

I guess for now TiDB doesn't have any compatibility promise for windows?

I have checked that this won't make TiDB failed to compile in Windows. As this only affects test cases, I think it could be fine to don't specially handle this 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

Leavrth

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

br/pkg/stream/stream_metas.go

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?

I guess no, we are copying the first one because we may modify the content of Delete[Physical|Logical]Files then. In this case we won't edit them.

br/pkg/stream/stream_metas.go

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.

…ncating-split2

YuJuncen · 2024-10-10T08:47:41Z

/test pull-br-integration-test

Some Unavailable...

ti-chi-bot · 2024-10-10T08:47:45Z

@YuJuncen: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test build
/test check-dev
/test check-dev2
/test mysql-test
/test pull-br-integration-test
/test pull-integration-ddl-test
/test pull-lightning-integration-test
/test pull-mysql-client-test
/test unit-test

The following commands are available to trigger optional jobs:

/test pingcap/tidb/canary_ghpr_unit_test
/test pull-common-test
/test pull-e2e-test
/test pull-integration-common-test
/test pull-integration-copr-test
/test pull-integration-jdbc-test
/test pull-integration-mysql-test
/test pull-integration-nodejs-test
/test pull-sqllogic-test
/test pull-tiflash-test
/test pull-unit-test-ddlv1

Use /test all to run the following jobs that were automatically triggered:

pingcap/tidb/ghpr_build
pingcap/tidb/ghpr_check
pingcap/tidb/ghpr_check2
pingcap/tidb/ghpr_mysql_test
pingcap/tidb/ghpr_unit_test
pingcap/tidb/pull_br_integration_test
pingcap/tidb/pull_integration_ddl_test
pingcap/tidb/pull_lightning_integration_test
pingcap/tidb/pull_mysql_client_test

In response to this:

/test pull-br-integration-test

Some Unavailable...

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.

tiprow · 2024-10-10T08:48:04Z

@YuJuncen: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test fast_test_tiprow
/test fast_test_tiprow_ddlargsv1
/test tidb_parser_test

Use /test all to run the following jobs that were automatically triggered:

fast_test_tiprow
tidb_parser_test

In response to this:

/test pull-br-integration-test

Some Unavailable...

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-10-10T08:57:23Z

/retest-required

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

YuJuncen added 6 commits October 9, 2024 14:10

make clippy happy

d521209

fix test cases

dbd8d58

restrict the number of total errors

431304f

Merge branch 'master' of https://github.com/pingcap/tidb into mig-tru…

fe77342

…ncating-split2

fix lint

c43fe37

fix panic in test

c2daadf

fix test case

16a6fa1

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

YuJuncen Oct 9, 2024

YuJuncen Oct 9, 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

Leavrth left a comment

Leavrth Sep 29, 2024

YuJuncen Oct 9, 2024

ti-chi-bot bot commented Sep 29, 2024

ti-chi-bot bot commented Sep 29, 2024

YuJuncen commented Oct 10, 2024

ti-chi-bot bot commented Oct 10, 2024

tiprow bot commented Oct 10, 2024

YuJuncen commented Oct 10, 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

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 Sep 27, 2024

tiprow bot commented Sep 27, 2024

YuJuncen commented Sep 27, 2024

Leavrth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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]

YuJuncen commented Oct 10, 2024

ti-chi-bot bot commented Oct 10, 2024

tiprow bot commented Oct 10, 2024

YuJuncen commented Oct 10, 2024

codecov bot commented Aug 26, 2024 •

edited

Loading