pkg/gtid: remove gtid.Set interface #5246

lance6716 · 2022-04-22T07:49:46Z

What problem does this PR solve?

Issue Number: close #4287

What is changed and how it works?

compared with mysql.GTIDSet, gtid.Set has two more methods.

Replace is no longer used
Truncate is used but does not take effects, see relay: fix GTID recover after relay log recovered dm#335 (comment)

Check List

Tests

Unit test

Code changes

Has exported function/method change
Has interface methods change

Related changes

Need to cherry-pick to the release branch

Release note

 `None`.

ti-chi-bot · 2022-04-22T07:49:47Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

Ehco1996
lichunzhu

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

lance6716 · 2022-04-22T07:50:49Z

I'll write a better copy-on-write API for GTID set after binlog streamer refactor (binlog streamer is the only writer for location under the syncer, things will be more simple then).

Signed-off-by: lance6716 <[email protected]>

lance6716 · 2022-04-22T10:00:59Z

/run-dm-integration-test

lance6716 · 2022-04-24T03:18:08Z

/run-dm-integration-test

Signed-off-by: lance6716 <[email protected]>

lance6716 · 2022-04-24T04:14:21Z

/run-dm-integration-test

codecov-commenter · 2022-04-24T04:41:37Z

Codecov Report

Merging #5246 (4c6889a) into master (687e248) will decrease coverage by 0.1939%.
The diff coverage is 52.0242%.

Flag	Coverage Δ
cdc	`60.8658% <52.0242%> (+0.2671%)`	⬆️
dm	`52.0171% <ø> (-0.4525%)`	⬇️

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

@@               Coverage Diff                @@
##             master      #5246        +/-   ##
================================================
- Coverage   56.1240%   55.9300%   -0.1940%     
================================================
  Files           522        520         -2     
  Lines         65325      67858      +2533     
================================================
+ Hits          36663      37953      +1290     
- Misses        25094      26184      +1090     
- Partials       3568       3721       +153

lichunzhu

LGTM

lichunzhu · 2022-04-25T05:07:04Z

dm/pkg/binlog/position.go

-				return cmp <= 0
-			}
-			// not supposed to happen, for safety here.
-			if location1.gtidSet != nil && location1.gtidSet.String() != "" {
+			if cmp > 0 {
 				return false
 			}
+			// should not happen
+			if cmp < 0 {
+				return true
+			}
 			// empty GTIDSet, then compare by position
-			log.L().Warn("both gtidSets are empty, will compare by position", zap.Stringer("location1", location1), zap.Stringer("location2", location2))
+			log.L().Warn("given gtidSets is empty, will compare by position", zap.Stringer("location", location))


Nit: use switch here?

lichunzhu · 2022-04-25T06:30:02Z

dm/syncer/checkpoint.go

+		return ""
+	}
+	var buf strings.Builder
+	buf.WriteString(fmt.Sprintf("location(%s)", b.location))


Suggested change

buf.WriteString(fmt.Sprintf("location(%s)", b.location))

fmt.Fprintf(&buf, "location(%s)", b.location)

lichunzhu · 2022-04-25T06:31:33Z

dm/syncer/checkpoint.go

+	var buf strings.Builder
+	buf.WriteString(fmt.Sprintf("location(%s)", b.location))
+	if b.ti != nil {
+		buf.WriteString(fmt.Sprintf(", tableInfo(ID: %d, Name:%s, ColNum: %d, IdxNum: %d, PKIsHandle: %t)", b.ti.ID, b.ti.Name, len(b.ti.Columns), len(b.ti.Indices), b.ti.PKIsHandle))


lichunzhu · 2022-04-25T06:31:39Z

dm/syncer/checkpoint.go

@@ -183,7 +192,7 @@ func (b *binlogPoint) String() string {
 	b.RLock()
 	defer b.RUnlock()

-	return fmt.Sprintf("%v(flushed %v)", b.savedPoint, b.flushedPoint)
+	return fmt.Sprintf("%s(flushed %s)", b.savedPoint.String(), b.flushedPoint.String())


Ehco1996

lgtm

dm/pkg/binlog/position.go

purelind · 2022-04-28T03:49:09Z

/run-dm-integration-test

purelind · 2022-04-28T03:49:40Z

/run-verify

purelind · 2022-04-28T03:50:25Z

/run-dm-compatibility-tests

purelind · 2022-04-28T03:51:22Z

/run-dm-integration-tests

purelind · 2022-04-28T03:52:56Z

/run-dm-compatibility-tests
/run-dm-integration-tests

lance6716 · 2022-04-29T06:15:14Z

ptal @Ehco1996 @GMHDBJD

lance6716 · 2022-05-23T02:27:56Z

I think this change will reduce efforts on other PRs which are related to GTID, so I change the base branch to master. cc @Ehco1996 @lichunzhu @GMHDBJD

will resolve conflict later.

lance6716 · 2022-05-23T03:27:24Z

/run-dm-integration-test

lance6716 · 2022-05-23T04:39:22Z

/run-dm-integration-test

lance6716 · 2022-05-23T06:01:20Z

/run-verify

Ehco1996

LGTM ~ nice work

lance6716 · 2022-05-23T06:59:50Z

/merge

ti-chi-bot · 2022-05-23T06:59:52Z

This pull request has been accepted and is ready to merge.

Commit hash: e680bc8

lance6716 · 2022-05-23T09:01:19Z

/run-dm-integration-test

ti-chi-bot · 2022-05-23T09:11:00Z

@lance6716: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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 ti-community-infra/tichi repository.

--wip-- [skip ci]

52d7667

ti-chi-bot added the release-note-none Denotes a PR that doesn't merit a release note. label Apr 22, 2022

lance6716 added the area/dm Issues or PRs related to DM. label Apr 22, 2022

ti-chi-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 22, 2022

resolve some TODO

a39c629

Signed-off-by: lance6716 <[email protected]>

lance6716 changed the title ~~[DNM] pkg/gtid: remove gtid.Set interface~~ pkg/gtid: remove gtid.Set interface Apr 22, 2022

lance6716 changed the title ~~pkg/gtid: remove gtid.Set interface~~ [WIP]pkg/gtid: remove gtid.Set interface Apr 22, 2022

ti-chi-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 22, 2022

try fix CI, left more TODO

03a119a

lance6716 added 2 commits April 24, 2022 11:42

check if Ut is fixed

0453b29

add some TODO

4c6889a

Signed-off-by: lance6716 <[email protected]>

lance6716 changed the title ~~[WIP]pkg/gtid: remove gtid.Set interface~~ pkg/gtid: remove gtid.Set interface Apr 24, 2022

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

lance6716 requested review from lichunzhu and GMHDBJD April 24, 2022 06:04

lichunzhu approved these changes Apr 25, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 25, 2022

address comment

813d2bc

Ehco1996 reviewed Apr 27, 2022

View reviewed changes

dm/pkg/binlog/position.go Show resolved Hide resolved

lance6716 changed the base branch from master to refactor-syncer April 28, 2022 03:22

lance6716 changed the base branch from refactor-syncer to master May 23, 2022 02:25

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

Merge branch 'master' of github.com:pingcap/tiflow into refactor-gtidset

dead44c

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

fix use old method

7ea52c3

try fix UT

e680bc8

Ehco1996 approved these changes May 23, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 23, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label May 23, 2022

Merge branch 'master' into refactor-gtidset

5bfafbc

Merge branch 'master' into refactor-gtidset

1913cc7

ti-chi-bot merged commit 066e082 into pingcap:master May 23, 2022

lance6716 deleted the refactor-gtidset branch October 13, 2022 08:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/gtid: remove gtid.Set interface #5246

pkg/gtid: remove gtid.Set interface #5246

lance6716 commented Apr 22, 2022 •

edited

Loading

ti-chi-bot commented Apr 22, 2022 •

edited

Loading

lance6716 commented Apr 22, 2022 •

edited

Loading

lance6716 commented Apr 22, 2022

lance6716 commented Apr 24, 2022

lance6716 commented Apr 24, 2022

codecov-commenter commented Apr 24, 2022 •

edited

Loading

lichunzhu left a comment

lichunzhu Apr 25, 2022

lichunzhu Apr 25, 2022

lichunzhu Apr 25, 2022

lichunzhu Apr 25, 2022

Ehco1996 left a comment

purelind commented Apr 28, 2022

purelind commented Apr 28, 2022

purelind commented Apr 28, 2022

purelind commented Apr 28, 2022

purelind commented Apr 28, 2022

lance6716 commented Apr 29, 2022

lance6716 commented May 23, 2022

lance6716 commented May 23, 2022

lance6716 commented May 23, 2022

lance6716 commented May 23, 2022

Ehco1996 left a comment

lance6716 commented May 23, 2022

ti-chi-bot commented May 23, 2022

lance6716 commented May 23, 2022

ti-chi-bot commented May 23, 2022

	buf.WriteString(fmt.Sprintf("location(%s)", b.location))
	fmt.Fprintf(&buf, "location(%s)", b.location)

pkg/gtid: remove gtid.Set interface #5246

pkg/gtid: remove gtid.Set interface #5246

Conversation

lance6716 commented Apr 22, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Apr 22, 2022 • edited Loading

lance6716 commented Apr 22, 2022 • edited Loading

lance6716 commented Apr 22, 2022

lance6716 commented Apr 24, 2022

lance6716 commented Apr 24, 2022

codecov-commenter commented Apr 24, 2022 • edited Loading

Codecov Report

lichunzhu left a comment

Choose a reason for hiding this comment

lichunzhu Apr 25, 2022

Choose a reason for hiding this comment

lichunzhu Apr 25, 2022

Choose a reason for hiding this comment

lichunzhu Apr 25, 2022

Choose a reason for hiding this comment

lichunzhu Apr 25, 2022

Choose a reason for hiding this comment

Ehco1996 left a comment

Choose a reason for hiding this comment

purelind commented Apr 28, 2022

purelind commented Apr 28, 2022

purelind commented Apr 28, 2022

purelind commented Apr 28, 2022

purelind commented Apr 28, 2022

lance6716 commented Apr 29, 2022

lance6716 commented May 23, 2022

lance6716 commented May 23, 2022

lance6716 commented May 23, 2022

lance6716 commented May 23, 2022

Ehco1996 left a comment

Choose a reason for hiding this comment

lance6716 commented May 23, 2022

ti-chi-bot commented May 23, 2022

lance6716 commented May 23, 2022

ti-chi-bot commented May 23, 2022

lance6716 commented Apr 22, 2022 •

edited

Loading

ti-chi-bot commented Apr 22, 2022 •

edited

Loading

lance6716 commented Apr 22, 2022 •

edited

Loading

codecov-commenter commented Apr 24, 2022 •

edited

Loading