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

syncer, relay: avoid read duplicate event when retry binlog streamer #2047

Merged
merged 25 commits into from
Sep 8, 2021

Conversation

glorv
Copy link
Collaborator

@glorv glorv commented Aug 25, 2021

What problem does this PR solve?

avoiding read duplicate event in syncer and relay log.

What is changed and how it works?

  • Relay
    • discard the events that already written when reset binlog reader in GTID mode
    • return an error ErrorMaybeDuplicateEvent when the relay log is not complete(not end with RotateEvent).
  • Syncer
    • recording the read event index and drop the first n events when reset streamer in GTID mode.
    • drop the first n events when binlog streamer returns the error ErrorMaybeDuplicateEvent

Check List

Tests

  • Integration test

Code changes

Side effects

Related changes

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

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Aug 25, 2021

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • Ehco1996
  • 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 lance6716 changed the title syncer,relay: avoid read duplicate event when retry binlog streamer syncer, relay: avoid read duplicate event when retry binlog streamer Aug 26, 2021
@glorv glorv 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 Aug 26, 2021
@glorv glorv marked this pull request as ready for review August 26, 2021 12:09
@ti-chi-bot ti-chi-bot added size/XL and removed size/L labels Aug 27, 2021
@lance6716
Copy link
Collaborator

seems an unstable test

pkg/streamer/reader.go Outdated Show resolved Hide resolved
relay/relay.go Show resolved Hide resolved
@glorv
Copy link
Collaborator Author

glorv commented Aug 30, 2021

seems an unstable test

fixed 😅

syncer/syncer.go Outdated Show resolved Hide resolved
syncer/syncer.go Outdated Show resolved Hide resolved
syncer/syncer.go Outdated Show resolved Hide resolved
relay/relay.go Show resolved Hide resolved
relay/relay.go Outdated Show resolved Hide resolved
syncer/syncer.go Outdated Show resolved Hide resolved
tests/duplicate_event/run.sh Outdated Show resolved Hide resolved
syncer/syncer.go Outdated Show resolved Hide resolved
tests/duplicate_event/run.sh Outdated Show resolved Hide resolved
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.

weakly want to change the safe-mode behaviour

https://github.com/pingcap/dm/pull/2047/files#r699233692

tests/duplicate_event/run.sh Show resolved Hide resolved
Comment on lines 29 to 30
cp $cur/conf/source1.yaml $WORK_DIR/source1.yaml
dmctl_operate_source create $WORK_DIR/source1.yaml $SOURCE_ID1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
cp $cur/conf/source1.yaml $WORK_DIR/source1.yaml
dmctl_operate_source create $WORK_DIR/source1.yaml $SOURCE_ID1
dmctl_operate_source create $cur/conf/source1.yaml $SOURCE_ID1

if we don't modify $cur/conf/source1.yaml

and for source2

@ti-chi-bot ti-chi-bot added the status/LGT1 One reviewer already commented LGTM label Sep 7, 2021
@glorv
Copy link
Collaborator Author

glorv commented Sep 7, 2021

@lance6716 I change the safe-mode logic to also directly skip N event, PTAL again.

@lance6716
Copy link
Collaborator

lgtm

Copy link
Contributor

@Ehco1996 Ehco1996 left a comment

Choose a reason for hiding this comment

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

LGTM, please remember to fix lint error

@ti-chi-bot ti-chi-bot added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Sep 8, 2021
@glorv
Copy link
Collaborator Author

glorv commented Sep 8, 2021

/run-all-tests

@glorv
Copy link
Collaborator Author

glorv commented Sep 8, 2021

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 4b73e9c

@ti-chi-bot
Copy link
Member

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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 size/XL status/can-merge status/LGT2 Two reviewers already commented LGTM, ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants