*: add etcd key for `start-relay` without worker name #2249

lance6716 · 2021-10-21T10:15:45Z

What problem does this PR solve?

part of #2241. The rest work will be done in another PR after #2219 is merged to reduce conflict.

What is changed and how it works?

as title, this key is just a marker like enable-relay in source config. But we'll dynamically change the value, and we should not modify the original input source config, so I persist it in etcd.

Check List

Tests

Unit test

Code changes

Has exported function/method change
Has exported variable/fields change
Has persistent data change

Side effects

Breaking backward compatibility (this etcd key can not be recgonized when downgrade, but I think that is expected)

Related changes

Need to cherry-pick to the release branch

ti-chi-bot · 2021-10-21T10:15:46Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

Ehco1996

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 · 2021-10-21T10:18:18Z

pkg/upgrade/upgrade.go

@@ -275,3 +275,5 @@ func upgradeToVer3(ctx context.Context, cli *clientv3.Client) error {
 func upgradeToVer4(cli *clientv3.Client, uctx Context) error {
 	return nil
 }
+
+// TODO: should we set UpstreamEnableRelayKeyAdapter when upgrading from <2.0.2


I prefer not, user will not aware of this behaviour if we automatically do for him, so when he adds new sources he will not meet expectation.

Maybe it's OK if we automatically start-relay for source added before upgrading. And for newly added sources, operate-source will return a message to notify user 🤔

@sunzhaoyang

If a source with enable-relay: true has to be automatically start-relayed after the upgrade.
If the source is added later, the enable-relay configuration is no longer valid, so you should give him an error and indicate that the configuration has been replaced with the start-relay command. Tell him to remove enable-relay from the configuration and create it again.

Please check the expected behaviour:

case 1)

in v2.0.1, user creates enable-relay: true source

upgrade to v2.1.0. That source should automatically turn on relay log when bound to any workers

newly added sources should has an ERROR message failing operate-source create (QuestionA: WARN message and not failing the command is enough?)

case 2)

in v2.0.1, user creates enable-relay: true source

upgrade to v2.0.7. That source has not automatically turn on relay log

further upgrade to v2.1.0. That source should also not automatically turn on relay log. And dmctl export will write enable-relay: true when exporting. (QuestionB: should we write enable-relay: false, so that these source config files can be used for this version?)

case 3)

in v2.0.1, user creates enable-relay: true source

upgrade to v2.0.7. That source has not automatically turn on relay log

user turns on relay log by start-relay -s <source-id> workerA

upgrade to v2.1.0. That source should only turn on relay log when bound to workerA

please check if this is expected, and check we don't choose behaviour of questions.

@sunzhaoyang

case 1) QuestionA: report a warning is enough, consider changing to error in future versions
case 2) QuestionB: exported source config should not include enable-relay, it is invalid.

rest LGTM

QuestionB: exported source config should not include enable-relay, it is invalid.

dmctl export is mainly used for downgrading, if we don't write enable-relay, user will have to adjust the content of every files when downgrading to v2.0.1 to keep relay enabled

I did not consider this scenario, but I still find it strange that the new version of the software exports the currently obsolete parameters, and there are very few degradations, which may not be a big problem.

OK, i'll implement it in next PR.

Ehco1996 · 2021-10-25T00:40:28Z

we should not modify the original input source config

can you tell me the reason ? actually we already update the user input source-cfg when they start-relay through openapi😅

dm/dm/master/openapi.go

Line 164 in 4327e5c

if err := s.scheduler.UpdateSourceCfg(sourceCfg); err != nil {

lance6716 · 2021-10-25T02:18:23Z

we should not modify the original input source config

can you tell me the reason ? actually we already update the user input source-cfg when they start-relay through openapi😅

currently we don't want to start relay for enable-relay in source config. If we write enable-relay to source config, after a new master elected leader, it will not distinguish the two scenarios only from one field.

Ehco1996

i am not sure if we need to add some adapter logic with this key when pause/resume relay ?

rest LGTM

dm/common/common.go

Ehco1996 · 2021-10-26T01:07:19Z

pkg/upgrade/upgrade.go

@@ -275,3 +275,5 @@ func upgradeToVer3(ctx context.Context, cli *clientv3.Client) error {
 func upgradeToVer4(cli *clientv3.Client, uctx Context) error {
 	return nil
 }
+
+// TODO: should we set UpstreamEnableRelayKeyAdapter when upgrading from <2.0.2


dm/master/scheduler/scheduler.go

Ehco1996 · 2021-10-26T01:11:29Z

dm/master/scheduler/scheduler.go

 func (s *Scheduler) recoverRelayConfigs(cli *clientv3.Client) error {
 	relayWorkers, _, err := ha.GetAllRelayConfig(cli)
 	if err != nil {
 		return err
 	}
 	s.relayWorkers = relayWorkers
+	sources, _, err := ha.GetAllEnabledRelaySources(cli)


can we add a test when Scheduler start ?

in next PR, when this field can really take effect

lance6716

i am not sure if we need to add some adapter logic with this key when pause/resume relay ?

The pause/resume action will change the status of all relay workers, and this key helps to create relay worker, not sure what logic you want to add

dm/master/scheduler/scheduler.go

lance6716 · 2021-10-26T02:17:25Z

dm/master/scheduler/scheduler.go

 func (s *Scheduler) recoverRelayConfigs(cli *clientv3.Client) error {
 	relayWorkers, _, err := ha.GetAllRelayConfig(cli)
 	if err != nil {
 		return err
 	}
 	s.relayWorkers = relayWorkers
+	sources, _, err := ha.GetAllEnabledRelaySources(cli)


in next PR, when this field can really take effect

lance6716 · 2021-10-26T02:20:35Z

pkg/upgrade/upgrade.go

@@ -275,3 +275,5 @@ func upgradeToVer3(ctx context.Context, cli *clientv3.Client) error {
 func upgradeToVer4(cli *clientv3.Client, uctx Context) error {
 	return nil
 }
+
+// TODO: should we set UpstreamEnableRelayKeyAdapter when upgrading from <2.0.2


Maybe it's OK if we automatically start-relay for source added before upgrading. And for newly added sources, operate-source will return a message to notify user 🤔

@sunzhaoyang

lance6716 · 2021-10-26T04:26:03Z

/run-integration-test

lance6716 · 2021-10-26T04:26:18Z

/run-unit-test

lance6716 · 2021-10-29T06:34:15Z

/cc @GMHDBJD

lance6716 · 2021-11-15T11:44:13Z

replaced by https://github.com/pingcap/ticdc/pull/3190

*: add etcd key for start-relay without worker name

b024ddd

Merge branch 'master' into auto-relay

a518aa0

ti-chi-bot requested review from Ehco1996 and GMHDBJD October 21, 2021 10:15

ti-chi-bot added the size/L label Oct 21, 2021

lance6716 commented Oct 21, 2021

View reviewed changes

lance6716 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 Oct 21, 2021

Ehco1996 reviewed Oct 26, 2021

View reviewed changes

lance6716 added 2 commits October 26, 2021 10:11

Merge branch 'master' of github.com:pingcap/dm into auto-relay

e35a413

address comment

782fa25

lance6716 commented Oct 26, 2021

View reviewed changes

Ehco1996 approved these changes Oct 26, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 One reviewer already commented LGTM label Oct 26, 2021

lance6716 mentioned this pull request Oct 27, 2021

config: don't write enable-relay when exporting #2268

Merged

ti-chi-bot mentioned this pull request Oct 27, 2021

config: don't write enable-relay when exporting (#2268) #2272

Merged

lance6716 mentioned this pull request Oct 28, 2021

upgrade, scheduler: recover memory status of relay when upgrade from <2.0.2 #2274

Closed

lance6716 added this to the v2.1.0 milestone Oct 28, 2021

lance6716 closed this Nov 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: add etcd key for `start-relay` without worker name #2249

*: add etcd key for `start-relay` without worker name #2249

lance6716 commented Oct 21, 2021 •

edited

Loading

ti-chi-bot commented Oct 21, 2021 •

edited

Loading

lance6716 Oct 21, 2021

Ehco1996 Oct 26, 2021

lance6716 Oct 26, 2021

sunzhaoyang Oct 26, 2021

lance6716 Oct 26, 2021

sunzhaoyang Oct 26, 2021

lance6716 Oct 26, 2021 •

edited

Loading

sunzhaoyang Oct 26, 2021

lance6716 Oct 26, 2021

Ehco1996 commented Oct 25, 2021

lance6716 commented Oct 25, 2021 •

edited

Loading

Ehco1996 left a comment

Ehco1996 Oct 26, 2021

Ehco1996 Oct 26, 2021

lance6716 Oct 26, 2021

lance6716 left a comment

lance6716 Oct 26, 2021

lance6716 Oct 26, 2021

lance6716 commented Oct 26, 2021

lance6716 commented Oct 26, 2021

lance6716 commented Oct 29, 2021

lance6716 commented Nov 15, 2021

*: add etcd key for start-relay without worker name #2249

*: add etcd key for start-relay without worker name #2249

Conversation

lance6716 commented Oct 21, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance6716 Oct 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ehco1996 commented Oct 25, 2021

lance6716 commented Oct 25, 2021 • edited Loading

Ehco1996 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance6716 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance6716 commented Oct 26, 2021

lance6716 commented Oct 26, 2021

lance6716 commented Oct 29, 2021

lance6716 commented Nov 15, 2021

*: add etcd key for `start-relay` without worker name #2249

*: add etcd key for `start-relay` without worker name #2249

lance6716 commented Oct 21, 2021 •

edited

Loading

ti-chi-bot commented Oct 21, 2021 •

edited

Loading

lance6716 Oct 26, 2021 •

edited

Loading

lance6716 commented Oct 25, 2021 •

edited

Loading