Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

dm-relay: create binlog reader from relay, refactor relay #3327

Merged
merged 37 commits into from
Nov 15, 2021
Merged

dm-relay: create binlog reader from relay, refactor relay #3327

merged 37 commits into from
Nov 15, 2021

Conversation

D3Hunter
Copy link
Contributor

@D3Hunter D3Hunter commented Nov 8, 2021 •
edited by lance6716
Loading

What problem does this PR solve?

part 2 of subtask 4 in #2214 of DM

i ever thought to split this pr, but it's too late after fixing all the test cases.

What is changed and how it works?

  • relay writer shouldn't be used concurrently and it's not a service, so remove unnecessary lock(except some status) and stage state
  • remove unnecessary stage in binlog writer; reuse the same binlog writer in relay writer
  • reuse relay writer in relay
  • move recover logic from relay writer to relay
  • remove unnecessary connection kill logic(already resolved in this pr, and current used version contains it)
  • binlog reader created using relay; register listeners implicitly
  • remove the transformer entity, make it a method in relay

Check List

Tests

  • Unit test
  • Integration test

Release note 

`None`.

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Nov 8, 2021 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • lance6716
  • 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.

@ti-chi-bot ti-chi-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. contribution This PR is from a community contributor. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 8, 2021
@D3Hunter D3Hunter marked this pull request as ready for review November 8, 2021 07:03
@ti-chi-bot 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 Nov 8, 2021
@D3Hunter
Copy link
Contributor Author

D3Hunter commented Nov 8, 2021

/cc @lichunzhu

@lichunzhu lichunzhu added the area/dm Issues or PRs related to DM. label Nov 8, 2021
@ti-chi-bot ti-chi-bot added the release-note-none Denotes a PR that doesn't merit a release note. label Nov 10, 2021
lichunzhu
lichunzhu approved these changes Nov 11, 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

dm/relay/relay.go Outdated Show resolved Hide resolved
dm/relay/relay.go Outdated Show resolved Hide resolved
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 11, 2021
lance6716
lance6716 reviewed Nov 12, 2021
View reviewed changes
Copy link
Contributor

@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, please address existing comments

@D3Hunter
Copy link
Contributor Author

@lance6716 PTAL

@ti-chi-bot 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 Nov 15, 2021
@lance6716
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: c617888

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 15, 2021
@D3Hunter
Copy link
Contributor Author

/run-kafka-integration-test

2 similar comments
@D3Hunter
Copy link
Contributor Author

/run-kafka-integration-test

@D3Hunter
Copy link
Contributor Author

/run-kafka-integration-test

@D3Hunter
Copy link
Contributor Author

/run-leak-test

@D3Hunter
Copy link
Contributor Author

/run-dm-compatibility-test
/run-dm-integration-test
/run-kafka-integration-test

@ti-chi-bot ti-chi-bot merged commit c1a8fa2 into pingcap:master Nov 15, 2021
@D3Hunter D3Hunter deleted the refactor-relay branch November 16, 2021 02:09
@D3Hunter D3Hunter added this to the v5.4.0 milestone Jan 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lance6716 lance6716 lance6716 approved these changes

@lichunzhu lichunzhu lichunzhu approved these changes

Assignees
No one assigned
Labels
area/dm Issues or PRs related to DM. contribution This PR is from a community contributor. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
v5.4.0
Development

Successfully merging this pull request may close these issues.
4 participants
@D3Hunter @ti-chi-bot @lance6716 @lichunzhu