Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

relay: align with MySQL when using GTID #1390

Merged
merged 17 commits into from
Jan 26, 2021
Merged

relay: align with MySQL when using GTID #1390

merged 17 commits into from
Jan 26, 2021

Conversation

GMHDBJD
Copy link
Collaborator

@GMHDBJD GMHDBJD commented Jan 19, 2021 •
edited
Loading

What problem does this PR solve?

close #1383

What is changed and how it works?

  • getPosByGTID always return the begin pos of the collect relay log file (filename, 4)
  • startSyncByGTID should replace event to heartbeat event which already be contained for specified gtid set
  • adjust gtid for relay

Check List

Tests

  • Unit test

@GMHDBJD GMHDBJD added status/WIP This PR is still work in progress type/bug-fix Bug fix needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated labels Jan 19, 2021
@GMHDBJD GMHDBJD added this to the v2.0.2 milestone Jan 19, 2021
@GMHDBJD
Copy link
Collaborator Author

GMHDBJD commented Jan 19, 2021

/run-all-tests

@GMHDBJD
Copy link
Collaborator Author

GMHDBJD commented Jan 19, 2021

/run-all-tests

@GMHDBJD GMHDBJD added status/PTAL This PR is ready for review. Add this label back after committing new changes and removed status/WIP This PR is still work in progress labels Jan 20, 2021
@GMHDBJD
Copy link
Collaborator Author

GMHDBJD commented Jan 20, 2021

/run-all-tests

1 similar comment
@GMHDBJD
Copy link
Collaborator Author

GMHDBJD commented Jan 20, 2021

/run-all-tests

@GMHDBJD GMHDBJD added status/WIP This PR is still work in progress and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Jan 20, 2021
@GMHDBJD GMHDBJD added status/PTAL This PR is ready for review. Add this label back after committing new changes 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 and removed status/WIP This PR is still work in progress labels Jan 21, 2021
lance6716
lance6716 reviewed Jan 21, 2021
View reviewed changes
Copy link
Collaborator

@lance6716 lance6716 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will review later

pkg/binlog/event/event.go
// modify header
headerClone := *header // do a copy
headerClone.Flags = 0
headerClone.EventSize = 39
Copy link
Collaborator

@lance6716 lance6716 Jan 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does MySQL choose not keeping size? I think this may cause positions in query-status mismatched which may confuse user

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It keeps the LogPos but not EventSize.

Copy link
Collaborator Author

@GMHDBJD GMHDBJD Jan 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That meets problem when we calculate startPosition, but we don't calculate it for heartbeat event.

lance6716
lance6716 reviewed Jan 21, 2021
View reviewed changes
Copy link
Collaborator

@lance6716 lance6716 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will review later

pkg/streamer/reader.go Outdated Show resolved Hide resolved
pkg/streamer/reader.go Outdated
err2 := r.advanceCurrentGtidSet(fmt.Sprintf("%s:%d", u.String(), ev.GNO))
if err2 != nil {
return errors.Trace(err2)
r.replaceWithHeartbeat, err = r.advanceCurrentGtidSet(fmt.Sprintf("%s:%d", u.String(), ev.GNO))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we use a local variable instead of a member of BinlogReader? BTW, I think we could reset replaceWithHeartbeat before switch in L446

Copy link
Collaborator Author

@GMHDBJD GMHDBJD Jan 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to local variable. We should not reset since if a GTIDEvent is replaced with heartbeat event, that means later event such QueryEvent, RowsEvent,TableMapEvent should also be replaced because they are in the same transaction.

pkg/streamer/reader.go Show resolved Hide resolved
lance6716
lance6716 reviewed Jan 25, 2021
View reviewed changes
Copy link
Collaborator

@lance6716 lance6716 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rest LGTM

(I'll commit the changes since author is leaving)

pkg/streamer/reader.go Outdated Show resolved Hide resolved
pkg/streamer/reader.go Outdated Show resolved Hide resolved
@lance6716
Copy link
Collaborator

/lgtm

@ti-srebot ti-srebot added the status/LGT1 One reviewer already commented LGTM label Jan 25, 2021
lichunzhu
lichunzhu approved these changes Jan 26, 2021
View reviewed changes
Copy link
Contributor

@lichunzhu lichunzhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lichunzhu
Copy link
Contributor

/lgtm

@ti-srebot ti-srebot removed the status/LGT1 One reviewer already commented LGTM label Jan 26, 2021
@ti-srebot ti-srebot added the status/LGT2 Two reviewers already commented LGTM, ready for merge label Jan 26, 2021
@lance6716 lance6716 merged commit 3cb465b into pingcap:master Jan 26, 2021
ti-srebot pushed a commit to ti-srebot/dm that referenced this pull request Jan 26, 2021
@GMHDBJD @ti-srebot
cherry pick pingcap#1390 to release-2.0
971c2ad 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Jan 26, 2021
Merged
3 tasks
@ti-srebot
Copy link

cherry pick to release-2.0 in PR #1403

@ti-srebot ti-srebot added already-cherry-pick-2.0 The related PR is already cherry-picked to release-2.0. Add this label once the PR is cherry-picked and removed 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 labels Jan 26, 2021
lance6716 pushed a commit that referenced this pull request Jan 27, 2021
@ti-srebot
cherry pick #1390 to release-2.0 (#1403)
fc323d8
@lance6716 lance6716 mentioned this pull request Mar 30, 2022
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lance6716 lance6716 lance6716 left review comments

@ti-srebot ti-srebot ti-srebot approved these changes

@lichunzhu lichunzhu lichunzhu approved these changes

Assignees
No one assigned
Labels
already-cherry-pick-2.0 The related PR is already cherry-picked to release-2.0. Add this label once the PR is cherry-picked needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated status/LGT2 Two reviewers already commented LGTM, ready for merge status/PTAL This PR is ready for review. Add this label back after committing new changes type/bug-fix Bug fix
Projects
None yet
Milestone
v2.0.2
Development

Successfully merging this pull request may close these issues.

relay's behaviour should align with MySQL when using GTID
4 participants
@GMHDBJD @lance6716 @lichunzhu @ti-srebot