syncer: fix syncer gtid auto switch from off to on #1723

lichunzhu · 2021-05-25T12:20:06Z

What problem does this PR solve?

#1586 part 1

What is changed and how it works?

Add auto adjust gtid before we start a task.

Check List

Tests

Unit test
Integration test

Code changes

Has exported function/method change
Has exported variable/fields change

Side effects

Possible performance regression
Increased code complexity

Related changes

Need to cherry-pick to the release branch
Need to be included in the release note

lance6716

will review later

Is the test case needed to be added to other_integrations.txt?

lance6716 · 2021-05-26T10:32:44Z

dm/worker/server.go

@@ -254,8 +254,10 @@ func (s *Server) doStartKeepAlive() {
 }

 func (s *Server) stopKeepAlive() {
-	s.kaCancel()
-	s.kaWg.Wait()
+	if s.kaCancel != nil {


in which case stopKeepAlive is called before or concurrent with startKeepAlive? we may need a lock if so 😂

dm-worker/main.go s.Start()

Start() return an error before s.startKeepAlive(). e.g.: worker port already in use

dm-worker/main.go s.Close() -> s.stopKeepAlive(), report a nil pointer info in stdout

GMHDBJD · 2021-05-27T01:38:40Z

pkg/binlog/position.go

+			if cmp = compareIndex(location1.Suffix, location2.Suffix); cmp != 0 {
+				return cmp
+			}
+			if location1.gtidSet != nil && location1.gtidSet.String() != "" {
+				return cmp
+			}


What if both gtid sets are empty, but suffix are not? e.g. location1:(pos1,"",1), location2:(pos2,"",0)

I think Suffix is only used in handle-error?

GMHDBJD · 2021-05-27T01:46:05Z

tests/adjust_gtid/run.sh

+	check_checkpoint $SOURCE_ID1 $name1 $pos1 $gtid1
+	check_checkpoint $SOURCE_ID2 $name2 $pos2 $gtid2
+	dmctl_stop_task_with_retry $TASK_NAME $MASTER_PORT
+	clean_gtid


Maybe start a source without gtid and than restart with gtid closer to the user scenario? Moreover, adjust gtid in relay seems not be implemented and tested in this test

I think cleaning gtid data is the same. Operation source again is a little complicated so I use this way to simplify the test.

adjust GTID in relay will be implemented in the next PR

GMHDBJD · 2021-05-27T01:46:30Z

tests/adjust_gtid/run.sh

+	dmctl_stop_task_with_retry $TASK_NAME $MASTER_PORT
+	clean_gtid
+
+	export GO_FAILPOINTS='github.com/pingcap/dm/syncer/AdjustGTIDExit=return(true)'


dumplicate export

…u/dm into fixSyncerGTIDAutoSwitch

purelind · 2021-05-27T09:59:23Z

/run-all-tests

lance6716 · 2021-05-29T09:51:59Z

cmd/dm-worker/main_test.go

@@ -56,6 +56,7 @@ func TestRunMain(_ *testing.T) {
 	defer func() { utils.OsExit = oldOsExit }()
 	utils.OsExit = func(code int) {
 		log.L().Info("os exits", zap.Int("exit code", code))
+		_ = utils.KillMySelf()


I can't remember why we need this helper function now. maybe the process won't exit after this test function returns?

if so, I think the reason may be that lthe test function should be named TestMain not TestRunMain, please check if this works.

ref https://golang.org/pkg/testing/#hdr-Main

lance6716 · 2021-05-31T02:51:01Z

cmd/dm-worker/main_test.go

@@ -56,6 +56,7 @@ func TestRunMain(_ *testing.T) {
 	defer func() { utils.OsExit = oldOsExit }()
 	utils.OsExit = func(code int) {
 		log.L().Info("os exits", zap.Int("exit code", code))
+		_ = utils.KillMySelf()


ref https://golang.org/pkg/testing/#hdr-Main

pkg/binlog/position.go

pkg/binlog/reader/reader.go

lance6716

rest lgtm

lance6716 · 2021-06-01T02:14:01Z

pkg/binlog/reader/util.go

-	defer r.Close()
-
+// GetGTIDsForPosStreamer tries to get GTID sets for the specified binlog position (for the corresponding txn) from a Streamer.
+func GetGTIDsForPosStreamer(ctx context.Context, r Streamer, endPos gmysql.Position) (gtid.Set, error) {


Suggested change

func GetGTIDsForPosStreamer(ctx context.Context, r Streamer, endPos gmysql.Position) (gtid.Set, error) {

func GetGTIDsForPosFromStreamer(ctx context.Context, r Streamer, endPos gmysql.Position) (gtid.Set, error) {

lance6716 · 2021-06-01T02:17:21Z

syncer/checkpoint.go

@@ -781,7 +781,8 @@ func (cp *RemoteCheckPoint) Load(tctx *tcontext.Context) error {
 			gset,
 		)
 		if isGlobal {
-			if binlog.CompareLocation(location, binlog.NewLocation(cp.cfg.Flavor), cp.cfg.EnableGTID) > 0 {
+			// Use CompareLocationAsPossible here to make sure checkpoint can be updated if gset is empty
+			if binlog.CompareLocationAsPossible(location, binlog.NewLocation(cp.cfg.Flavor), cp.cfg.EnableGTID) > 0 {


Does this function always called with location2 as empty location? If so, we could not be bothered to add an unit test, just simplifying the logic and tighten function name

syncer/syncer.go

tests/adjust_gtid/run.sh

…u/dm into fixSyncerGTIDAutoSwitch

GMHDBJD · 2021-06-03T01:44:27Z

/lgtm

ti-chi-bot · 2021-06-03T01:44:28Z

@GMHDBJD: Please use GitHub review feature instead of /lgtm [cancel] when you want to submit review to the pull request.
For how to use GitHub review feature, see also this document provided by GitHub.

For the reason we drop support to the commands, see also this page.
This reply is being used as a temporary reply during the migration of review process and will be removed on July 1st.

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.

GMHDBJD

/lgtm

ti-chi-bot · 2021-06-03T03:27:52Z

@GMHDBJD: Please use GitHub review feature instead of /lgtm [cancel] when you want to submit review to the pull request.
For how to use GitHub review feature, see also this document provided by GitHub.

For the reason we drop support to the commands, see also this page.
This reply is being used as a temporary reply during the migration of review process and will be removed on July 1st.

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.

ti-chi-bot · 2021-06-03T04:15:10Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

lance6716

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 · 2021-06-03T04:15:53Z

add require-LGT1 for GMHDBJD

lance6716 · 2021-06-03T04:16:04Z

/merge

ti-chi-bot · 2021-06-03T04:16:06Z

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

Commit hash: 4c39ddf

GMHDBJD

LGTM

ti-chi-bot · 2021-06-03T04:17:03Z

In response to a cherrypick label: new pull request created: #1745.

lichunzhu added 3 commits May 21, 2021 10:32

finish development part

b234975

tmp

0e0f8b3

fix test

785e52a

lichunzhu added the needs-cherry-pick-release-2.0 This PR should be cherry-picked to release-2.0. Remove this label after cherry-picked to release-2.0 label May 25, 2021

ti-chi-bot requested review from GMHDBJD and lance6716 May 25, 2021 12:20

ti-chi-bot added the size/XXL label May 25, 2021

lichunzhu added status/PTAL This PR is ready for review. Add this label back after committing new changes and removed size/XXL labels May 25, 2021

Merge branch 'master' into fixSyncerGTIDAutoSwitch

4eb2f7a

ti-chi-bot added the size/XXL label May 25, 2021

lichunzhu added 3 commits May 26, 2021 10:51

fix shfmt

9e21c00

fix check

3ce2322

fix check

3dda7f4

lance6716 reviewed May 26, 2021

View reviewed changes

update others_integration.txt

deeed45

GMHDBJD reviewed May 27, 2021

View reviewed changes

lichunzhu added 4 commits May 27, 2021 15:35

try fix

4780ff5

Merge branch 'master' into fixSyncerGTIDAutoSwitch

88fe058

try fix

a4be61c

Merge branch 'fixSyncerGTIDAutoSwitch' of https://github.com/lichunzh…

06daec1

…u/dm into fixSyncerGTIDAutoSwitch

lance6716 reviewed May 29, 2021

View reviewed changes

lance6716 reviewed May 31, 2021

View reviewed changes

lichunzhu and others added 2 commits May 31, 2021 14:57

address comments

63caa4d

Merge branch 'master' into fixSyncerGTIDAutoSwitch

a743fae

lance6716 added the needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated label Jun 1, 2021

lance6716 added this to the v2.0.4 milestone Jun 1, 2021

lance6716 reviewed Jun 1, 2021

View reviewed changes

address comments

69981e7

lichunzhu and others added 4 commits June 1, 2021 17:09

Merge branch 'fixSyncerGTIDAutoSwitch' of https://github.com/lichunzh…

e8cb982

…u/dm into fixSyncerGTIDAutoSwitch

fix make check

6872516

update go 1.16

ab53df5

Merge branch 'master' into fixSyncerGTIDAutoSwitch

4c39ddf

GMHDBJD reviewed Jun 3, 2021

View reviewed changes

lance6716 approved these changes Jun 3, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 One reviewer already commented LGTM label Jun 3, 2021

lance6716 added the require-LGT1 for small PR, LGT1 is enough label Jun 3, 2021

ti-chi-bot added the status/can-merge label Jun 3, 2021

ti-chi-bot merged commit 5c1377e into pingcap:master Jun 3, 2021

GMHDBJD approved these changes Jun 3, 2021

View reviewed changes

ti-chi-bot mentioned this pull request Jun 3, 2021

syncer: fix syncer gtid auto switch from off to on (#1723) #1745

Merged

lichunzhu deleted the fixSyncerGTIDAutoSwitch branch June 3, 2021 04:22

ti-chi-bot added a commit that referenced this pull request Jun 9, 2021

syncer: fix syncer gtid auto switch from off to on (#1723) (#1745)

5269b0a

lance6716 mentioned this pull request Jun 16, 2021

can't smoothly continue sync when switch GTID from on to off #1586

Closed

lichunzhu mentioned this pull request Sep 6, 2021

syncer: fix the gtid purged error when switching pos replication to gtid #2094

Merged

ti-chi-bot mentioned this pull request Sep 8, 2021

syncer: fix the gtid purged error when switching pos replication to gtid (#2094) #2104

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

syncer: fix syncer gtid auto switch from off to on #1723

syncer: fix syncer gtid auto switch from off to on #1723

lichunzhu commented May 25, 2021

lance6716 left a comment

lance6716 May 26, 2021

lichunzhu May 26, 2021 •

edited

Loading

GMHDBJD May 27, 2021

lichunzhu May 27, 2021

GMHDBJD May 27, 2021

lichunzhu May 27, 2021

GMHDBJD May 27, 2021

purelind commented May 27, 2021

lance6716 May 29, 2021

lance6716 May 31, 2021

lance6716 May 31, 2021

lance6716 left a comment

lance6716 Jun 1, 2021

lance6716 Jun 1, 2021

GMHDBJD commented Jun 3, 2021

ti-chi-bot commented Jun 3, 2021

GMHDBJD left a comment

ti-chi-bot commented Jun 3, 2021

ti-chi-bot commented Jun 3, 2021

lance6716 commented Jun 3, 2021

lance6716 commented Jun 3, 2021

ti-chi-bot commented Jun 3, 2021

GMHDBJD left a comment

ti-chi-bot commented Jun 3, 2021

	func GetGTIDsForPosStreamer(ctx context.Context, r Streamer, endPos gmysql.Position) (gtid.Set, error) {
	func GetGTIDsForPosFromStreamer(ctx context.Context, r Streamer, endPos gmysql.Position) (gtid.Set, error) {

syncer: fix syncer gtid auto switch from off to on #1723

syncer: fix syncer gtid auto switch from off to on #1723

Conversation

lichunzhu commented May 25, 2021

What problem does this PR solve?

What is changed and how it works?

Check List

lance6716 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lichunzhu May 26, 2021 • 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

purelind commented May 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance6716 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GMHDBJD commented Jun 3, 2021

ti-chi-bot commented Jun 3, 2021

GMHDBJD left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Jun 3, 2021

ti-chi-bot commented Jun 3, 2021

lance6716 commented Jun 3, 2021

lance6716 commented Jun 3, 2021

ti-chi-bot commented Jun 3, 2021

GMHDBJD left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Jun 3, 2021

lichunzhu May 26, 2021 •

edited

Loading