DM: support inject at dml when open GTID #4848

WizardXiao · 2022-03-11T01:39:56Z

What problem does this PR solve?

Issue Number: ref #4260

What is changed and how it works?

As inject handle will add the original dml to the last event to be inject, we check weather dml was executed will not check position but gtid and suffix when open gitd, but the last event's suffix will set to be 0, and it will be ignore.
Change the location saved table checkpoint or global checkpoint to be a cloned location which suffix is 0, so when check the last event's location will not ignore it, because it will be equal and dml will execute.
Change the compare when flush checkpoint to just use compare position, because there is no suffix and the gtid may be equal when handle error.

Check List

Tests

Unit test
Integration test

Release note

None

…d-inject-test

ti-chi-bot · 2022-03-11T01:39:57Z

[REVIEW NOTIFICATION]

This pull request has not been approved.

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.

WizardXiao · 2022-03-11T01:40:54Z

/run-all-tests

buchuitoudegou

typos😀

buchuitoudegou · 2022-03-11T09:14:13Z

dm/syncer/err-operator/operator.go

@@ -177,6 +177,8 @@ func (h *Holder) MatchAndApply(startLocation, endLocation binlog.Location, curre
 	h.mu.Lock()
 	defer h.mu.Unlock()

+	h.logger.Info("try to match and apply a operator", zap.Stringer("startlocation", startLocation), zap.Stringer("endlocation", endLocation), zap.Any("currentEvent", currentEvent))


Suggested change

h.logger.Info("try to match and apply a operator", zap.Stringer("startlocation", startLocation), zap.Stringer("endlocation", endLocation), zap.Any("currentEvent", currentEvent))

h.logger.Info("try to match and apply an operator", zap.Stringer("startlocation", startLocation), zap.Stringer("endlocation", endLocation), zap.Any("currentEvent", currentEvent))

buchuitoudegou · 2022-03-11T09:14:53Z

dm/syncer/err-operator/operator.go

@@ -215,7 +217,7 @@ func (h *Holder) MatchAndApply(startLocation, endLocation binlog.Location, curre
 		}
 	}

-	h.logger.Info("match and apply a operator", zap.Stringer("startlocation", startLocation), zap.Stringer("endlocation", endLocation), zap.Stringer("operator", operator))
+	h.logger.Info("match and apply a operator", zap.Stringer("startlocation", startLocation), zap.Stringer("endlocation", endLocation), zap.Any("currentEvent", currentEvent), zap.Stringer("operator", operator))


Suggested change

h.logger.Info("match and apply a operator", zap.Stringer("startlocation", startLocation), zap.Stringer("endlocation", endLocation), zap.Any("currentEvent", currentEvent), zap.Stringer("operator", operator))

h.logger.Info("match and apply an operator", zap.Stringer("startlocation", startLocation), zap.Stringer("endlocation", endLocation), zap.Any("currentEvent", currentEvent), zap.Stringer("operator", operator))

…d-inject-test

WizardXiao · 2022-03-14T17:50:24Z

/run-all-tests

Ehco1996 · 2022-03-15T01:22:26Z

dm/pkg/binlog/position.go

@@ -274,6 +274,13 @@ func (l Location) Clone() Location {
 	return l.CloneWithFlavor("")
 }

+// Clone clones a same location, but reset suffix to 0.


please left a todo location whit suffix is hadr do understand, let's refine this logic later

yes, suffix logic is hard to understand and it is used in compare location too much. i think i will refine it in other pr.

Ehco1996 · 2022-03-15T01:25:05Z

dm/syncer/checkpoint.go

-
-	return binlog.CompareLocation(pos, b.flushedPoint.location, b.enableGTID) > 0
+	// locations in savedPoint have no suffix and may have same giid like handle-error, we can use position to check them
+	return binlog.CompareLocation(pos, b.flushedPoint.location, false) > 0


seems change the logic in binlog.CompareLocation is more reasonable ?, hardcode to enable gtid to false is too tricky

binlog.CompareLocation is called in many places and i think we may fix it when we refine suffix. outOfDateBy is used to check between table checkpoint and global checkpoint for flush, this kind of point should have right position, so we can use position to check.
bwt, i will add more tests about inject, but first of all, it should not affect other integration tests.

add more tests about inject

will you doing this in thie pr or, is this pr ready for review?

not yet. after some tests, it will be more complex when use sharding ddl.

WizardXiao · 2022-03-15T06:33:58Z

need fix inject command when it was called in sharding.

ti-chi-bot · 2022-03-26T03:05:41Z

@WizardXiao: PR needs rebase.

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.

WizardXiao added 2 commits March 11, 2022 09:26

commit-message: try to fix inject dml error when open gtid

da9d6c2

Merge branch 'master' of https://github.com/WizardXiao/tiflow into ad…

1afd668

…d-inject-test

ti-chi-bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Mar 11, 2022

WizardXiao added the area/dm Issues or PRs related to DM. label Mar 11, 2022

ti-chi-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 11, 2022

WizardXiao added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 11, 2022

buchuitoudegou reviewed Mar 11, 2022

View reviewed changes

WizardXiao added 3 commits March 13, 2022 18:49

Merge branch 'master' of https://github.com/WizardXiao/tiflow into ad…

8763cec

…d-inject-test

commit-message: fix inject at dml when open gtid

0041739

Merge branch 'master' of https://github.com/WizardXiao/tiflow into ad…

7d7bd0d

…d-inject-test

WizardXiao added status/ptal Could you please take a look? and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Mar 15, 2022

Ehco1996 reviewed Mar 15, 2022

View reviewed changes

WizardXiao added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. and removed status/ptal Could you please take a look? labels Mar 15, 2022

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

lance6716 mentioned this pull request Jun 13, 2022

Refactor some logic of syncer #5834

Open

6 tasks

WizardXiao closed this by deleting the head repository Mar 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM: support inject at dml when open GTID #4848

DM: support inject at dml when open GTID #4848

WizardXiao commented Mar 11, 2022 •

edited

Loading

ti-chi-bot commented Mar 11, 2022

WizardXiao commented Mar 11, 2022

buchuitoudegou left a comment

buchuitoudegou Mar 11, 2022

buchuitoudegou Mar 11, 2022

WizardXiao commented Mar 14, 2022

Ehco1996 Mar 15, 2022

WizardXiao Mar 15, 2022

Ehco1996 Mar 15, 2022

WizardXiao Mar 15, 2022 •

edited

Loading

Ehco1996 Mar 15, 2022

WizardXiao Mar 15, 2022 •

edited

Loading

WizardXiao commented Mar 15, 2022

ti-chi-bot commented Mar 26, 2022

	h.logger.Info("try to match and apply a operator", zap.Stringer("startlocation", startLocation), zap.Stringer("endlocation", endLocation), zap.Any("currentEvent", currentEvent))
	h.logger.Info("try to match and apply an operator", zap.Stringer("startlocation", startLocation), zap.Stringer("endlocation", endLocation), zap.Any("currentEvent", currentEvent))

	h.logger.Info("match and apply a operator", zap.Stringer("startlocation", startLocation), zap.Stringer("endlocation", endLocation), zap.Any("currentEvent", currentEvent), zap.Stringer("operator", operator))
	h.logger.Info("match and apply an operator", zap.Stringer("startlocation", startLocation), zap.Stringer("endlocation", endLocation), zap.Any("currentEvent", currentEvent), zap.Stringer("operator", operator))

DM: support inject at dml when open GTID #4848

DM: support inject at dml when open GTID #4848

Conversation

WizardXiao commented Mar 11, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Mar 11, 2022

WizardXiao commented Mar 11, 2022

buchuitoudegou left a comment

Choose a reason for hiding this comment

buchuitoudegou Mar 11, 2022

Choose a reason for hiding this comment

buchuitoudegou Mar 11, 2022

Choose a reason for hiding this comment

WizardXiao commented Mar 14, 2022

Ehco1996 Mar 15, 2022

Choose a reason for hiding this comment

WizardXiao Mar 15, 2022

Choose a reason for hiding this comment

Ehco1996 Mar 15, 2022

Choose a reason for hiding this comment

WizardXiao Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

Ehco1996 Mar 15, 2022

Choose a reason for hiding this comment

WizardXiao Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

WizardXiao commented Mar 15, 2022

ti-chi-bot commented Mar 26, 2022

WizardXiao commented Mar 11, 2022 •

edited

Loading

WizardXiao Mar 15, 2022 •

edited

Loading

WizardXiao Mar 15, 2022 •

edited

Loading