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/streamer: move binlog log reader into relay #3243

Merged
merged 11 commits into from
Nov 5, 2021
Merged

dm-relay/streamer: move binlog log reader into relay #3243

merged 11 commits into from
Nov 5, 2021

Conversation

D3Hunter
Copy link
Contributor

@D3Hunter D3Hunter commented Nov 2, 2021 •
edited
Loading

What problem does this PR solve?

it's first part of subtask 4 in #2214 of DM

no code logic change in this pr, to make review/merge easier

What is changed and how it works?

  • move binlog log reader into relay
  • move binlog/writer to relay
  • flat small sub packages
  • rename some structs to avoid conflict

Check List

no code logic change

Release note 

None

@ti-chi-bot
Copy link
Member

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

[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.

@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 2, 2021
@D3Hunter D3Hunter marked this pull request as ready for review November 2, 2021 09:09
@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 2, 2021
@D3Hunter
Copy link
Contributor Author

D3Hunter commented Nov 2, 2021

/cc @lichunzhu

@ti-chi-bot ti-chi-bot requested a review from lichunzhu November 2, 2021 09:10
@ti-chi-bot ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 2, 2021
@lance6716 lance6716 added the area/dm Issues or PRs related to DM. label Nov 2, 2021
Ehco1996
Ehco1996 approved these changes Nov 3, 2021
View reviewed changes
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

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 3, 2021
@D3Hunter
Copy link
Contributor Author

D3Hunter commented Nov 4, 2021

@lance6716 PTAL

@D3Hunter
Copy link
Contributor Author

D3Hunter commented Nov 4, 2021

@glorv @lichunzhu PTAL

lance6716
lance6716 reviewed Nov 4, 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.

will review after some more urgent PRs

lance6716
lance6716 reviewed Nov 5, 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

there're 2 cases that I can't decide.

  • we plan to move remote reader into relay package, so the retry logic of remote reader is also moved.
    Now retry logic is put in a separated file, that's quite small so I prefer we include it into remote_reader.go. (too many files under relay folder IMO 😂 )
  • we plan to move relay log purger into relay package, so the strategy of purger is also moved.
    The strategies are located in 4 files and they're not relevant to other files (except for purger), so I'd like to merge them into purger_strategy.go

@D3Hunter
Copy link
Contributor Author

D3Hunter commented Nov 5, 2021

too many files under relay folder IMO

not a problem to me as long as we can difference between them

@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 5, 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: 8a8c868

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

/run-all-tests

@D3Hunter
Copy link
Contributor Author

D3Hunter commented Nov 5, 2021

/run-unit-test

@ti-chi-bot ti-chi-bot merged commit 9a6bd36 into pingcap:master Nov 5, 2021
@D3Hunter D3Hunter deleted the merge-log-reader-relay branch November 5, 2021 10:18
@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

@Ehco1996 Ehco1996 Ehco1996 approved these changes

@lichunzhu lichunzhu Awaiting requested review from lichunzhu

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.
5 participants
@D3Hunter @ti-chi-bot @lance6716 @lichunzhu @Ehco1996