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: turn on/off relay by source config #3190

Merged
merged 14 commits into from
Nov 15, 2021
Merged

dm: turn on/off relay by source config #3190

merged 14 commits into from
Nov 15, 2021

Conversation

lance6716
Copy link
Contributor

What problem does this PR solve?

we re-support old behavour. if user set enable-relay: true in source config, we'll use relay replication for the bound worker.

note that start-relay -s source workername (whether executed now or upgraded from metadata of old version) on a enable-relay: true source is undefined behaviour.

What is changed and how it works?

⬆️

Check List

Tests

  • Integration test

Code changes

  • Has exported variable/fields change

Side effects

  • Breaking backward compatibility

Related changes

  • Need to update the documentation

Release note 

`enable-relay: true` in source config takes effect again

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Oct 29, 2021 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • Ehco1996
  • 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 the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Oct 29, 2021
@ti-chi-bot ti-chi-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 29, 2021
@lance6716
Copy link
Contributor Author

/cc @GMHDBJD @glorv

@ti-chi-bot ti-chi-bot requested review from glorv and GMHDBJD October 29, 2021 09:50
@lance6716
Copy link
Contributor Author

lance6716 commented Oct 29, 2021 •
edited
Loading

/run-dm-integration-tests

let's see how many failed

@GMHDBJD
Copy link
Contributor

GMHDBJD commented Oct 29, 2021

So if the user started a task with relay, we need to ask them whether the cluster is upgraded from v2.0.1 or v2.0.1+? 😭

@lance6716
Copy link
Contributor Author

So if the user started a task with relay, we need to ask them whether the cluster is upgraded from v2.0.1 or v2.0.1+? 😭

only v2.0.1+ has start-relay command 🤔

@GMHDBJD
Copy link
Contributor

GMHDBJD commented Oct 29, 2021

If user upgrade from v2.0.1, they should stop relay by stop-source and update enable-relay: false. If user upgrade from v2.0.1+, they should stop relay by stop-relay command?

@GMHDBJD
Copy link
Contributor

GMHDBJD commented Oct 29, 2021

I prefer support enable-relay after we really add start-relay -s source, then the behavior of start-relay -s source and enable-relay is the same. If this is the case, it is difficult to explain their differences to users

@lance6716
Copy link
Contributor Author

If user upgrade from v2.0.1, they should stop relay by stop-source and update enable-relay: false. If user upgrade from v2.0.1+, they should stop relay by stop-relay command?

  • upgrade from v2.0.1, nothing to do if current enable-relay is their wanted.
  • upgrade from v2.0.1+, should stop-relay first (or we can test what will happen)

@GMHDBJD
Copy link
Contributor

GMHDBJD commented Oct 29, 2021 •
edited
Loading

upgrade from v2.0.1+, should stop-relay first

so stop-relay, upgrade cluster, then the relay will be started. 😂

@lance6716
Copy link
Contributor Author

upgrade from v2.0.1+, should stop-relay first

so stop-relay, upgrade cluster, then the relay will be started. 😂

if they need stop-relay, it means they has started relay. So it's not that supprise.

We can implement start-relay -s source and stop-relay -s source in next PR 😂

@ti-chi-bot ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 29, 2021
@lance6716
Copy link
Contributor Author

/run-dm-integration-tests

1 similar comment
@lance6716
Copy link
Contributor Author

/run-dm-integration-tests

@lance6716
Copy link
Contributor Author

/run-dm-integration-tests

1 similar comment
@lance6716
Copy link
Contributor Author

/run-dm-integration-tests

@lance6716
Copy link
Contributor Author

/run-dm-integration-tests

1 similar comment
@lance6716
Copy link
Contributor Author

/run-dm-integration-tests

Ehco1996
Ehco1996 approved these changes Nov 15, 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 🉑 please fix the confilcts

@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
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 15, 2021
@lance6716
Copy link
Contributor Author

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 3948407

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

/run-kafka-integration-test

@lance6716
Copy link
Contributor Author

/run-dm-integration-test

1 similar comment
@lance6716
Copy link
Contributor Author

/run-dm-integration-test

@ti-chi-bot ti-chi-bot merged commit 0df7bfb into pingcap:master Nov 15, 2021
@GMHDBJD
Copy link
Contributor

GMHDBJD commented Nov 15, 2021 •
edited
Loading

need update upstream_switch case https://github.com/pingcap/ticdc/runs/4209093582?check_suite_focus=true @lance6716

This was referenced Nov 15, 2021
Merged
Closed
Closed
@GMHDBJD GMHDBJD mentioned this pull request Nov 16, 2021
Closed
@lance6716 lance6716 mentioned this pull request Jan 14, 2022
Merged
14 tasks
@lance6716 lance6716 deleted the enable-relay branch October 13, 2022 08:37
@kennytm kennytm mentioned this pull request Sep 3, 2024
Merged
15 tasks
This was referenced Jan 22, 2025
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ehco1996 Ehco1996 Ehco1996 approved these changes

@lichunzhu lichunzhu lichunzhu approved these changes

@glorv glorv Awaiting requested review from glorv

@GMHDBJD GMHDBJD Awaiting requested review from GMHDBJD

Assignees
No one assigned
Labels
area/dm Issues or PRs related to DM. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 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
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lance6716 @ti-chi-bot @GMHDBJD @lichunzhu @Ehco1996