master(dm): specify worker when create upstream source #4167

jerrylisl · 2021-12-30T10:10:13Z

What problem does this PR solve?

Issue Number: close #4169

command "dmctl operate-source create" add param -w(worker) to specify a DM-worker to be bound.
when using this param, the command can't create multiple source

What is changed and how it works?

1.dmctl parses param "-w(worker)"
2.if the worker is specified, scheduler binds source to it

Check List

Tests

Unit test
Integration test

Code changes

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

Side effects

Related changes

Need to update the documentation

Release note

dmctl operate-source create specify DM-worker to be bound

ti-chi-bot · 2021-12-30T10:10:14Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

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

CLAassistant · 2021-12-30T10:10:19Z

All committers have signed the CLA.

ti-chi-bot · 2021-12-30T10:10:24Z

Welcome @jerrylisl!

It looks like this is your first PR to pingcap/tiflow 🎉.

I'm the bot to help you request reviewers, add labels and more, See available commands.

We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to pingcap/tiflow. 😃

dm/dm/master/scheduler/scheduler.go

lance6716 · 2022-01-05T02:49:30Z

Hi, do you need some help? we can discuss here or in slack or in asktug

lance6716 · 2022-01-07T06:35:46Z

/run-dm-integration-test

lance6716 · 2022-01-07T06:35:51Z

/run-verify

lance6716 · 2022-01-10T02:36:34Z

You can find our CI reports some test is broken with this feature, please take a look at idc-jenkins-ci-ticdc/dm-integration-test and jenkins-ticdc/verify (ignore the CDC part)

lance6716

thanks, please address my comments and fix the git conflict.

dm/dm/master/scheduler/scheduler.go

dm/tests/dmctl_command/run.sh

lance6716 · 2022-01-11T05:43:49Z

dm/dm/master/scheduler/scheduler_test.go

+	t.sourceCfgNotExist(c, s, sourceID2)
+	t.workerFree(c, s, workerName2)
+	c.Assert(s.AddSourceCfgWithWorker(&sourceCfg2, workerName1), IsNil)
+	// source2 is unbound because expected worker1 is already bound


I'm not sure this behaviour is expected. In other words, if user want to add a source to a specific DM-worker, and the worker is not available (offline or already bound), should we bind the source to another worker, left the source unbound or remove the source?
Maybe you need this feature in your scenario, could you explain why you choose this bevaiour?

BTW, when DM-master failover, DM-master will try to bind ~~and~~any unbound sources.

specific DM-worker only allow single source, user can do a transfer or remove the source quickly if the specific worker is not available, if we do something like bind the source to another worker (or remove it) can cause some extra "scheduling cost". And i think a "specify-worker" cmd bind a not specific worker may not be expected.

a "specify-worker" cmd bind a not specific worker may not be expected

I got this.

How about we don't add the source when the specified worker is not available? I can list two reason:

The command is more atomic, the effects all happen or nothing happen

When the DM-master failovers, currently we will try to bind any unbound source, so the source may be bound to a unwanted worker.

ok, i'll modify this part and ci later.

dm/pkg/terror/error_list.go

jerrylisl · 2022-01-14T10:04:18Z

/run-dm-integration-test

lance6716 · 2022-01-17T04:08:14Z

@jerrylisl There are some unit test and linters failed, PTAL

and feel free to ask for help

dm/dm/ctl/master/operate_source.go

dm/dm/master/scheduler/scheduler.go

jerrylisl · 2022-01-18T10:46:25Z

i've used "unit_test_pkg" in dm Makefile, is there any substitute in this repository?

jerrylisl · 2022-01-18T11:09:22Z

/run-dm-integration-test

lance6716 · 2022-01-18T11:33:18Z

lint error: https://ci.pingcap.net/blue/organizations/jenkins/atom-lint/detail/atom-lint/5808/pipeline/

you can run make check and git add then commit repeatly, until make check passed.

one integration test fail: https://ci2.pingcap.net/blue/organizations/jenkins/dm_ghpr_integration_test/detail/dm_ghpr_integration_test/2936/pipeline

this is because we have a pattern matching for dmctl output, you should also update it

tiflow/dm/tests/dmctl_basic/check_list/operate_source.sh

Line 6 in 3ab3c7d

    
           "operate-source <operate-type> \[config-file ...\] \[--print-sample-config\] \[flags\]" 1

unit test fail: https://ci.pingcap.net/blue/organizations/jenkins/atom-ut/detail/atom-ut/9927/tests/

you can cd into that directory and run commands like go test -check.f "testScheduler.TestWatchWorkerEventEtcdCompact" to run one test case. If the unit test uses failpoint, you should make failpoint-enable before run tests and make failpoint-disable after run tests

lance6716 · 2022-01-18T11:39:37Z

also, please sign the CLA #4167 (comment)

lance6716 · 2022-01-19T10:59:16Z

/run-dm-integration-test

lance6716 · 2022-01-21T02:17:02Z

/run-dm-integration-test

worker param optional Co-authored-by: lance6716 <[email protected]>

Co-authored-by: lance6716 <[email protected]>

lance6716 · 2022-01-27T02:17:19Z

/merge

ti-chi-bot · 2022-01-27T02:17:21Z

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

Commit hash: 53c0c3b

lance6716 · 2022-01-27T03:10:17Z

/run-dm-integration-test
/run-verify

lance6716 · 2022-01-27T03:37:27Z

dm/tests/dmctl_command/run.sh

+		"list-member --name worker1" \
+		"\"stage\": \"bound\"" 1 \
+		"\"source\": \"mysql-replica-01\"" 1
+	dmctl_operate_source create $WORK_DIR/source2.yaml $SOURCE_ID2 -w $WORKER1_NAME


CI failed,

[2022-01-27T03:23:23.958Z] dmctl test cmd: "operate-source create /tmp/dm_test/dmctl_command/source2.yaml" [2022-01-27T03:23:25.335Z] dmctl test cmd: "operate-source create /tmp/dm_test/dmctl_command/source2.yaml -w wrong-worker" [2022-01-27T03:23:25.335Z] dmctl test cmd: "operate-source create /tmp/dm_test/dmctl_command/source2.yaml -w worker1" [2022-01-27T03:23:25.595Z] dmctl test cmd: "operate-source create /tmp/dm_test/dmctl_command/source2.yaml" [2022-01-27T03:23:25.595Z] command: operate-source create /tmp/dm_test/dmctl_command/source2.yaml "result": true count: 0 != expected: 2 [2022-01-27T03:23:25.595Z] { [2022-01-27T03:23:25.595Z] "result": false, [2022-01-27T03:23:25.595Z] "msg": "[code=46007:class=scheduler:scope=internal:level=medium], Message: source config with ID mysql-replica-02 already exists", [2022-01-27T03:23:25.595Z] "sources": [ [2022-01-27T03:23:25.595Z] ] [2022-01-27T03:23:25.595Z] }

for line 89, I think create source2 on worker1 should failed because worker1 is bound to source1, but the script didn't detect the fail until it goes to line 98. Please fix it

lance6716 · 2022-01-27T03:37:44Z

/hold

lance6716 · 2022-01-27T12:32:53Z

/run-dm-integration-test

jerrylisl · 2022-01-28T02:19:31Z

/run-dm-integration-test

lance6716 · 2022-01-28T02:43:58Z

/unhold
/merge

ti-chi-bot · 2022-01-28T02:44:01Z

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

Commit hash: 317dd7d

lance6716 · 2022-01-28T03:10:25Z

/run-kafka-integration-test

close pingcap#4169

ti-chi-bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Dec 30, 2021

ti-chi-bot added the contribution This PR is from a community contributor. label Dec 30, 2021

ti-chi-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 30, 2021

lance6716 added the area/dm Issues or PRs related to DM. label Dec 30, 2021

lance6716 reviewed Dec 30, 2021

View reviewed changes

dm/dm/master/scheduler/scheduler.go Outdated Show resolved Hide resolved

lance6716 mentioned this pull request Dec 30, 2021

dmctl, scheduler: specify worker when create upstream source pingcap/dm#2287

Closed

lance6716 changed the title ~~dm: specify worker when create upstream source~~ master(dm): specify worker when create upstream source Dec 30, 2021

jerrylisl force-pushed the source_specify_worker branch from fbe3fe1 to 0e848f1 Compare January 7, 2022 04:06

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 8, 2022

lance6716 reviewed Jan 11, 2022

View reviewed changes

jerrylisl force-pushed the source_specify_worker branch from d5572cd to 9d6f538 Compare January 11, 2022 08:36

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 11, 2022

lance6716 reviewed Jan 13, 2022

View reviewed changes

dm/pkg/terror/error_list.go Outdated Show resolved Hide resolved

GMHDBJD reviewed Jan 18, 2022

View reviewed changes

dm/dm/ctl/master/operate_source.go Show resolved Hide resolved

dm/dm/ctl/master/operate_source.go Outdated Show resolved Hide resolved

dm/dm/master/scheduler/scheduler.go Outdated Show resolved Hide resolved

jerrylisl force-pushed the source_specify_worker branch from 0631eed to d1b984d Compare January 18, 2022 10:25

jerrylisl and others added 7 commits January 26, 2022 21:45

test: fix specify worker check

720c4f4

Update dm/dm/ctl/master/operate_source.go

c85cdc9

worker param optional Co-authored-by: lance6716 <[email protected]>

Update dm/tests/dmctl_basic/check_list/operate_source.sh

3919a1a

Co-authored-by: lance6716 <[email protected]>

Update dm/dm/master/scheduler/scheduler.go

435a513

Co-authored-by: lance6716 <[email protected]>

fix: remove bound check

fe1f7ba

fix: err order

64dbaa6

test: add test

53c0c3b

jerrylisl force-pushed the source_specify_worker branch from 1e72f7e to 53c0c3b Compare January 26, 2022 13:46

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 26, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 27, 2022

Merge branch 'master' into source_specify_worker

1b70fa2

lance6716 reviewed Jan 27, 2022

View reviewed changes

ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 27, 2022

fix: dm_ctl err

3360172

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Jan 27, 2022

fix: shell err

317dd7d

ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 28, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 28, 2022

Merge branch 'master' into source_specify_worker

a1931e3

ti-chi-bot merged commit 2b45869 into pingcap:master Jan 28, 2022

niubell pushed a commit to niubell/tiflow that referenced this pull request Jan 28, 2022

master(dm): specify worker when create upstream source (pingcap#4167)

4da1aba

close pingcap#4169

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

master(dm): specify worker when create upstream source #4167

master(dm): specify worker when create upstream source #4167

jerrylisl commented Dec 30, 2021 •

edited by lance6716

Loading

ti-chi-bot commented Dec 30, 2021 •

edited

Loading

CLAassistant commented Dec 30, 2021 •

edited

Loading

ti-chi-bot commented Dec 30, 2021

lance6716 commented Jan 5, 2022

lance6716 commented Jan 7, 2022

lance6716 commented Jan 7, 2022

lance6716 commented Jan 10, 2022

lance6716 left a comment

lance6716 Jan 11, 2022 •

edited

Loading

jerrylisl Jan 11, 2022

lance6716 Jan 11, 2022

jerrylisl Jan 11, 2022

jerrylisl commented Jan 14, 2022

lance6716 commented Jan 17, 2022

jerrylisl commented Jan 18, 2022

jerrylisl commented Jan 18, 2022

lance6716 commented Jan 18, 2022

lance6716 commented Jan 18, 2022

lance6716 commented Jan 19, 2022

lance6716 commented Jan 21, 2022

lance6716 commented Jan 27, 2022

ti-chi-bot commented Jan 27, 2022

lance6716 commented Jan 27, 2022

lance6716 Jan 27, 2022 •

edited

Loading

lance6716 commented Jan 27, 2022

lance6716 commented Jan 27, 2022

jerrylisl commented Jan 28, 2022

lance6716 commented Jan 28, 2022

ti-chi-bot commented Jan 28, 2022

lance6716 commented Jan 28, 2022

master(dm): specify worker when create upstream source #4167

master(dm): specify worker when create upstream source #4167

Conversation

jerrylisl commented Dec 30, 2021 • edited by lance6716 Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Dec 30, 2021 • edited Loading

CLAassistant commented Dec 30, 2021 • edited Loading

ti-chi-bot commented Dec 30, 2021

lance6716 commented Jan 5, 2022

lance6716 commented Jan 7, 2022

lance6716 commented Jan 7, 2022

lance6716 commented Jan 10, 2022

lance6716 left a comment

Choose a reason for hiding this comment

lance6716 Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

jerrylisl Jan 11, 2022

Choose a reason for hiding this comment

lance6716 Jan 11, 2022

Choose a reason for hiding this comment

jerrylisl Jan 11, 2022

Choose a reason for hiding this comment

jerrylisl commented Jan 14, 2022

lance6716 commented Jan 17, 2022

jerrylisl commented Jan 18, 2022

jerrylisl commented Jan 18, 2022

lance6716 commented Jan 18, 2022

lance6716 commented Jan 18, 2022

lance6716 commented Jan 19, 2022

lance6716 commented Jan 21, 2022

lance6716 commented Jan 27, 2022

ti-chi-bot commented Jan 27, 2022

lance6716 commented Jan 27, 2022

lance6716 Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

lance6716 commented Jan 27, 2022

lance6716 commented Jan 27, 2022

jerrylisl commented Jan 28, 2022

lance6716 commented Jan 28, 2022

ti-chi-bot commented Jan 28, 2022

lance6716 commented Jan 28, 2022

jerrylisl commented Dec 30, 2021 •

edited by lance6716

Loading

ti-chi-bot commented Dec 30, 2021 •

edited

Loading

CLAassistant commented Dec 30, 2021 •

edited

Loading

lance6716 Jan 11, 2022 •

edited

Loading

lance6716 Jan 27, 2022 •

edited

Loading