master/worker(dm): suppport update config when task not run #4760

Ehco1996 · 2022-03-03T08:37:54Z

What problem does this PR solve?

Issue Number: ref #4484

What is changed and how it works?

for master:

~~add enable filed in source cfg for later use~~ this will added in next pr
can update source cfg when no running tasks and no relay workers
can update task cfg when no running tasks and all subtasks is in sync unit, also only limited fields can be updated

for worker

provide a new rpc :CheckSubtasksCanUpdate for master
refresh source cfg when enable relay or resume subtask
refresh subtask cfg when resume subtask

but there is no call now, just inter logic

Check List

Tests

Unit test
Integration test

Code changes

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

Side effects

Possible performance regression
Increased code complexity
Breaking backward compatibility

Related changes

Release note

`None`.

ti-chi-bot · 2022-03-03T08:37:55Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

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

dm/dm/config/source_config.go

dm/dm/master/scheduler/scheduler.go

dm/dm/master/scheduler/scheduler_test.go

dm/dm/worker/source_worker.go

Ehco1996 · 2022-03-03T09:10:54Z

dm/dm/worker/source_worker.go

-		}
-		w.cfg = sourceCfg
+	// we need update config from etcd first in case this cfg is updated by master
+	if refreshErr := w.refreshSourceCfg(); refreshErr != nil {


i think we only need to refresh when user want to enable-relay or restart task for source

Ehco1996 · 2022-03-03T09:12:04Z

dm/dm/worker/source_worker.go

+	failpoint.Inject("SkipRefreshFromETCDInUT", func(_ failpoint.Value) {
+		failpoint.Goto("bypassRefresh")
+	})
+	if op == pb.TaskOp_Resume || op == pb.TaskOp_AutoResume {


when user want to restart or start a stopped task, use the newest cfg from etcd.

dynamic update will impl later

dm/dm/master/scheduler/scheduler.go

Ehco1996 · 2022-03-03T10:14:28Z

dm/dm/master/scheduler/scheduler.go

@@ -1039,6 +1027,63 @@ func (s *Scheduler) RemoveSubTasks(task string, sources ...string) error {
 	return nil
 }

+// UpdateSubTasks update the information of one or more subtasks for one task.
+// now only support update a task that not in running stage.
+func (s *Scheduler) UpdateSubTasks(cfgs ...config.SubTaskConfig) error {


need check if this field is safe to updated, also need to check current unit with this subtask check this in worker or master? if checked in worekr i need to added new rpc

~~actully we can check this just on master side~~
no we need send request to worker to check the current task unit anyway.... so we better just do this check on worker side

codecov-commenter · 2022-03-04T01:36:23Z

Codecov Report

Merging #4760 (e2aa469) into master (9607554) will increase coverage by 0.0736%.
The diff coverage is 53.4869%.

Flag	Coverage Δ
cdc	`59.8003% <53.4869%> (-0.1219%)`	⬇️
dm	`52.3426% <ø> (+0.3137%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@@               Coverage Diff                @@
##             master      #4760        +/-   ##
================================================
+ Coverage   55.6402%   55.7138%   +0.0736%     
================================================
  Files           494        521        +27     
  Lines         61283      64930      +3647     
================================================
+ Hits          34098      36175      +2077     
- Misses        23750      25201      +1451     
- Partials       3435       3554       +119

Ehco1996 · 2022-03-04T05:28:09Z

/run-dm-integration-tests

Ehco1996 · 2022-03-04T06:17:00Z

/run-dm-integration-tests

Ehco1996 · 2022-03-04T06:36:10Z

/run-dm-integration-tests

Ehco1996 · 2022-03-04T07:35:33Z

/run-dm-integration-tests

Ehco1996 · 2022-03-04T07:38:13Z

/run-dm-integration-tests

Ehco1996 · 2022-03-04T08:35:52Z

/run-dm-integration-tests

Ehco1996 · 2022-03-04T08:36:30Z

/run-dm-integration-tests

lance6716 · 2022-03-09T02:55:06Z

dm/dm/master/scheduler/scheduler.go

-	// 3. check this cfg is ok to update.
-	if !checkSourceCfgCanUpdated(s.sourceCfgs[cfg.SourceID], cfg) {
-		return terror.ErrSchedulerSourceCfgUpdate.Generate()
+	// 3. check if there is relay workers for this source


I remember since v5.4, source config with enable-relay: true will not record a relay worker. Should we also allow update for this case?

now use expectRelayStages to check if this source is enable relay, seems this can avoid this case

7aa818a

lance6716 · 2022-03-09T02:57:18Z

dm/dm/master/scheduler/scheduler.go

+		return terror.ErrSchedulerNotStarted.Generate()
+	}
+	if len(cfgs) == 0 {
+		return nil // no subtasks need to add, this should not happen.


you can change the function parameter to (context, SubTaskConfig, ...SubTaskConfig) to tell the compiler there must be at least one SubTaskConfig.

OK to not change

i can't find the example of how to use unnamed paramaters , how can i refer the unnamed stcfgs?

seems the only use of unnamed parameters is when you have to define a function with a specific
https://stackoverflow.com/questions/24000305/how-to-refer-to-an-unnamed-function-argument-in-go

(context, ...SubTaskConfig) -> (context, SubTaskConfig, ...SubTaskConfig)

dm/dm/master/scheduler/scheduler.go

dm/dm/worker/source_worker.go

Ehco1996 · 2022-03-09T05:36:06Z

/run-dm-integration-tests

D3Hunter

rest lgtm

dm/syncer/syncer.go

Ehco1996 · 2022-03-09T07:23:54Z

/run-dm-integration-tests

dm/dm/master/scheduler/scheduler_test.go

Ehco1996 · 2022-03-10T00:33:23Z

/run-verify

dm/dm/worker/source_worker.go

dm/dm/master/scheduler/scheduler.go

dm/pkg/terror/error_list.go

lance6716 · 2022-03-10T03:26:18Z

dm/syncer/syncer.go

+func (s *Syncer) CheckCanUpdateCfg(newCfg *config.SubTaskConfig) error {
+	s.RLock()
+	defer s.RUnlock()
+	// can't update when in sharding merge


in some optimistic case, like partially drop column, BAlist should also not be updated

@GMHDBJD PTAL, can you list the forbidden cases?

i think it's hard to list or check all the case ... maybe let the user to bear the risk of changing config

also remember to add this limitation to doc

dm/syncer/syncer.go

Co-authored-by: lance6716 <lance6716@gmail.com>

Ehco1996 · 2022-03-10T05:37:59Z

/run-dm-integration-tests

lance6716 · 2022-03-10T09:56:56Z

dm/dm/worker/server_test.go

@@ -640,3 +664,80 @@ func loadSourceConfigWithoutPassword(c *C) *config.SourceConfig {
 	sourceCfg.From.Password = "" // no password set
 	return sourceCfg
 }
+
+func (t *testServer) testSourceWorker(c *C) {


why move it? the content of this function is related to source worker, so it's natural to put it in source_worker_test.go

Ehco1996 · 2022-03-10T10:14:39Z

/merge

ti-chi-bot · 2022-03-10T10:14:42Z

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

Commit hash: 6074849

save work

dbe67dc

Ehco1996 added the area/dm Issues or PRs related to DM. label Mar 3, 2022

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 3, 2022

Ehco1996 commented Mar 3, 2022

View reviewed changes

dm/dm/config/source_config.go Outdated Show resolved Hide resolved

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

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

dm/dm/worker/source_worker.go Outdated Show resolved Hide resolved

address comment

587efbb

ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 3, 2022

Ehco1996 commented Mar 3, 2022

View reviewed changes

dm/dm/worker/source_worker.go Outdated Show resolved Hide resolved

Ehco1996 commented Mar 3, 2022

View reviewed changes

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

Ehco1996 commented Mar 3, 2022

View reviewed changes

Ehco1996 added 2 commits March 3, 2022 18:27

add more ut

ff8bcc6

check relay worekrs

0be36f6

ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 4, 2022

worker side check

f13457b

revcert about source enable

7cd1276

just log error when update failed

a24ba25

fix an test

7c446a7

lance6716 reviewed Mar 9, 2022

View reviewed changes

Ehco1996 added 2 commits March 9, 2022 13:30

address comment

7aa818a

revert GetRelayWorkers

7749eac

D3Hunter approved these changes Mar 9, 2022

View reviewed changes

dm/syncer/syncer.go Outdated Show resolved Hide resolved

dm/syncer/syncer.go Show resolved Hide resolved

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 9, 2022

Ehco1996 added 2 commits March 9, 2022 15:19

Merge branch 'master' into feature-#4484-update-cfg

dd8c699

address comment

1e0082c

lance6716 reviewed Mar 9, 2022

View reviewed changes

fix ut

6f1d1b2

Merge branch 'master' into feature-#4484-update-cfg

385d49b

lance6716 reviewed Mar 10, 2022

View reviewed changes

Ehco1996 and others added 3 commits March 10, 2022 13:17

Apply suggestions from code review

f80b6b9

Co-authored-by: lance6716 <lance6716@gmail.com>

address comment

3965843

copy code from another branch

e2aa469

lance6716 approved these changes Mar 10, 2022

View reviewed changes

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 Mar 10, 2022

Ehco1996 added 2 commits March 10, 2022 18:13

revert test func move

2aeb520

Merge branch 'master' into feature-#4484-update-cfg

6074849

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

ti-chi-bot added 3 commits March 10, 2022 19:28

Merge branch 'master' into feature-#4484-update-cfg

889cd62

Merge branch 'master' into feature-#4484-update-cfg

522800a

Merge branch 'master' into feature-#4484-update-cfg

26c5a55

ti-chi-bot merged commit 65db3c5 into pingcap:master Mar 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

master/worker(dm): suppport update config when task not run #4760

master/worker(dm): suppport update config when task not run #4760

Ehco1996 commented Mar 3, 2022 •

edited

Loading

ti-chi-bot commented Mar 3, 2022 •

edited

Loading

Ehco1996 Mar 3, 2022

Ehco1996 Mar 3, 2022

Ehco1996 Mar 3, 2022 •

edited

Loading

Ehco1996 Mar 4, 2022 •

edited

Loading

codecov-commenter commented Mar 4, 2022 •

edited

Loading

Ehco1996 commented Mar 4, 2022

Ehco1996 commented Mar 4, 2022

Ehco1996 commented Mar 4, 2022

Ehco1996 commented Mar 4, 2022

Ehco1996 commented Mar 4, 2022

Ehco1996 commented Mar 4, 2022

Ehco1996 commented Mar 4, 2022

lance6716 Mar 9, 2022

Ehco1996 Mar 9, 2022

lance6716 Mar 9, 2022

Ehco1996 Mar 9, 2022

lance6716 Mar 9, 2022

Ehco1996 commented Mar 9, 2022

D3Hunter left a comment

Ehco1996 commented Mar 9, 2022

Ehco1996 commented Mar 10, 2022

lance6716 Mar 10, 2022

Ehco1996 Mar 10, 2022

lance6716 Mar 10, 2022

Ehco1996 commented Mar 10, 2022

lance6716 Mar 10, 2022

Ehco1996 commented Mar 10, 2022

ti-chi-bot commented Mar 10, 2022

master/worker(dm): suppport update config when task not run #4760

master/worker(dm): suppport update config when task not run #4760

Conversation

Ehco1996 commented Mar 3, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ehco1996 Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

Ehco1996 Mar 4, 2022 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Mar 4, 2022 • edited Loading

Codecov Report

Ehco1996 commented Mar 4, 2022

Ehco1996 commented Mar 4, 2022

Ehco1996 commented Mar 4, 2022

Ehco1996 commented Mar 4, 2022

Ehco1996 commented Mar 4, 2022

Ehco1996 commented Mar 4, 2022

Ehco1996 commented Mar 4, 2022

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

Ehco1996 commented Mar 9, 2022

D3Hunter left a comment

Choose a reason for hiding this comment

Ehco1996 commented Mar 9, 2022

Ehco1996 commented Mar 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ehco1996 commented Mar 10, 2022

Choose a reason for hiding this comment

Ehco1996 commented Mar 10, 2022

ti-chi-bot commented Mar 10, 2022

Ehco1996 commented Mar 3, 2022 •

edited

Loading

ti-chi-bot commented Mar 3, 2022 •

edited

Loading

Ehco1996 Mar 3, 2022 •

edited

Loading

Ehco1996 Mar 4, 2022 •

edited

Loading

codecov-commenter commented Mar 4, 2022 •

edited

Loading