validator(dm): support start from time & fix & integration test #5534

D3Hunter · 2022-05-23T09:19:45Z

What problem does this PR solve?

Issue Number: close #5449

What is changed and how it works?

this pr is large most due to change to .proto and test

support validation start --start-time
refactor and add more check to validation start, validation start can be used in 2 ways:
- enable validator on the fly(like create+start in task operation), in this case all validator of target subtask should not have been enabled before;
- start stopped validator, all validator of target subtask should have been enabled, and cannot add explicit mode and start-time flag
add mode to validation status
fix bug that we set an not-completely-initialized subtaskConfig into subtask on validator start. now we only update needed config.
fix bug that we initialized validator when expect stage is stopped(happens on failover), which may cause resource leak.
extract real binlog from relay log binlog pos
reset FirstValidateTS when loading pending row changes to avoid failed row changes be marked as error row immediately if validator stopped for a long time.
add more integration test

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Questions

Will it cause performance regression or break compatibility?

Do you need to update user documentation, design documentation or monitoring documentation?

Release note

`None`.

ti-chi-bot · 2022-05-23T09:19:46Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

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

D3Hunter · 2022-05-23T09:20:09Z

/run-all-tests

D3Hunter · 2022-05-23T09:20:27Z

/cc @lance6716 @Ehco1996 @buchuitoudegou

lance6716

will review later

dm/pkg/binlog/filename.go

dm/dm/ctl/master/start_stop_validation.go

lance6716 · 2022-05-24T04:00:52Z

dm/dm/master/scheduler/scheduler.go

+// OperateValidationTask operate validator of subtask.
+// 	tasks: tasks need to operate
+// 	validatorStages: stage info of subtask validators
+// 	changedSubtaskCfgs: length should be 0 or equal to validatorStages


seems changedSubtaskCfgs can tolerate not the same length as validatorStages 🤔

caller should and has make sure about this

dm/dm/master/server.go

buchuitoudegou

Will take a look at IT later.

buchuitoudegou · 2022-05-24T06:34:59Z

dm/dm/config/task_cli_args.go

-				return terror.Annotate(err, "error while parse start-time, expected in the format like '2006-01-02 15:04:05' or '2006-01-02T15:04:05'")
-			}
+		if _, err := utils.ParseStartTime(t.StartTime); err != nil {
+			return terror.Annotate(err, "error while parse start-time, expected in the format like '2006-01-02 15:04:05' or '2006-01-02T15:04:05'")


Suggested change

return terror.Annotate(err, "error while parse start-time, expected in the format like '2006-01-02 15:04:05' or '2006-01-02T15:04:05'")

return terror.Annotate(err, "error while parsing start-time, expected formats are like '2006-01-02 15:04:05' or '2006-01-02T15:04:05'")

there are 2 other message below using error while parse style which doesn't meets english grammar, will not change them in this pr. maybe during refactor.

I'll note it in the issue. (this error message will be presented to the users, it would better be grammatical valid and make sense).

those string it not introduced in this pr, will move it into #5143

dm/dm/master/server.go

buchuitoudegou · 2022-05-24T06:52:26Z

dm/dm/master/server.go

+	// if user set mode or start-time explicitly, then we do start operation.
+	// none of the target validators should have enabled
+	if explicitModeOrStartTime && !noneEnabled {
+		msg := fmt.Sprintf("some of target validator(%s) has already enabled.", enabledValidator)


Should we list all the validators that have been enabled? Or maybe leave a workaround here (e.g. "please check validator's statuses by dmctl query-status"). Otherwise, users might have to try over and over again.

Should we list all the validators that have been enabled

not sure how often user start validator only on some subtask, we can enhance it later.

D3Hunter · 2022-05-24T08:29:56Z

/run-all-tests

D3Hunter · 2022-05-24T08:31:18Z

ptal of fc1f258 too.

fix bug that we initialized validator when expect stage is stopped(happens on failover), which may cause resource leak.

D3Hunter · 2022-05-24T08:34:20Z

/run-all-tests

D3Hunter · 2022-05-24T09:40:05Z

/run-all-tests

D3Hunter · 2022-05-24T10:03:09Z

/run-verify

codecov-commenter · 2022-05-24T10:16:21Z

Codecov Report

Merging #5534 (01d3a3b) into master (fcea4d5) will increase coverage by 0.0993%.
The diff coverage is 59.9280%.

Flag	Coverage Δ
cdc	`61.4505% <62.1827%> (+0.3024%)`	⬆️
dm	`52.0594% <54.4329%> (+0.0167%)`	⬆️

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

@@               Coverage Diff                @@
##             master      #5534        +/-   ##
================================================
+ Coverage   56.0768%   56.1761%   +0.0993%     
================================================
  Files           535        528         -7     
  Lines         70143      69857       -286     
================================================
- Hits          39334      39243        -91     
+ Misses        27078      26883       -195     
  Partials       3731       3731

D3Hunter · 2022-05-26T03:18:30Z

ptal

lance6716

leave for lunch

lance6716 · 2022-05-26T04:14:00Z

dm/dm/ctl/master/start_stop_validation.go

+	}
+
+	if op == StartValidationOp {
+		args.explicitMode = cmd.Flags().Changed("mode")


since the default value of mode is "", we in fact don't need this check. Also for start-time.

I'm both OK with these two styles: if explicitMode {checkMode} or if mode != "" {checkMode}

will leave it.

lance6716 · 2022-05-26T04:21:54Z

dm/dm/master/server.go

+	allEnabled, noneEnabled := true, true
+	for taskName := range subTaskCfgs {
+		for sourceID := range subTaskCfgs[taskName] {
+			if s.scheduler.ValidatorEnabled(taskName, sourceID) {


the check for all subtasks is not atomic, is it safe that other RPC change the status?

seems it will break "changedSubtaskCfgs: length should be 0 or equal to validatorStages"

changed to acquire subtask latch before check

dm/dm/master/server.go

D3Hunter · 2022-05-26T10:23:47Z

/run-dm-integration-test

D3Hunter · 2022-05-26T11:35:54Z

/run-verify

D3Hunter · 2022-05-26T11:36:12Z

ptal @lance6716 @buchuitoudegou

buchuitoudegou

rest LGTM

dm/tests/dmctl_command/run.sh

buchuitoudegou · 2022-05-27T02:50:05Z

dm/tests/dmctl_command/run.sh

+
+	echo "--> (success) validation start: start validation without explicit mode for source 1"
+	run_dm_ctl $WORK_DIR "127.0.0.1:$MASTER_PORT" \
+		"validation start -s $SOURCE_ID1 test" \


Could you add a test case that specifies multiple sources? More than one source is valid too.

buchuitoudegou · 2022-05-27T02:52:40Z

dm/dm/config/task_cli_args.go

-				return terror.Annotate(err, "error while parse start-time, expected in the format like '2006-01-02 15:04:05' or '2006-01-02T15:04:05'")
-			}
+		if _, err := utils.ParseStartTime(t.StartTime); err != nil {
+			return terror.Annotate(err, "error while parse start-time, expected in the format like '2006-01-02 15:04:05' or '2006-01-02T15:04:05'")


I'll note it in the issue. (this error message will be presented to the users, it would better be grammatical valid and make sense).

lance6716 · 2022-05-27T03:22:22Z

dm/tests/dmctl_command/run.sh

+		"\"unit\": \"Sync\"" 2
+
+	echo "--> (fail) validation start: missing mode value"
+	run_dm_ctl $WORK_DIR "127.0.0.1:$MASTER_PORT" \


maybe also check the help message of validation xxx is readable, like "pause-task \[-s source ...\] \[task-name | task-file\] \[flags\]"

no gains, cobra will just print cobra.Command.Use & cobra.Command.Short

lance6716 · 2022-05-27T03:24:58Z

/hold

please unhold after addresss comments

D3Hunter · 2022-05-27T03:47:46Z

/unhold

D3Hunter · 2022-05-27T03:48:08Z

/merge

ti-chi-bot · 2022-05-27T03:48:10Z

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

Commit hash: fed53e2

D3Hunter · 2022-05-27T04:24:09Z

/run-dm-integration-test

D3Hunter added 10 commits May 23, 2022 16:40

fix relay binlog name

0fe6d22

support start-time for validator

a0638cf

fix lint; add start-time

0f0ad8e

add comments

008bfb2

add mode to validation status; add i t

362e950

fix error msg

6fdb1d8

fix; add i t

979930c

use different tz on upstream and tidb

05f6c53

reset first-fail-ts on load

7781f1c

fix lint

a9305f2

ti-chi-bot added 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. labels May 23, 2022

ti-chi-bot requested review from buchuitoudegou, Ehco1996 and lance6716 May 23, 2022 09:20

lance6716 reviewed May 24, 2022

View reviewed changes

buchuitoudegou reviewed May 24, 2022

View reviewed changes

D3Hunter added 2 commits May 24, 2022 16:02

fix bug & ut

fc1f258

fix comments

8cb3416

fix comments

9901a6f

fix lint

8011c27

lance6716 added the area/dm Issues or PRs related to DM. label May 26, 2022

lance6716 reviewed May 26, 2022

View reviewed changes

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

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

acquire subtask latch before check; unify naming

cc708a6

buchuitoudegou approved these changes May 27, 2022

View reviewed changes

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

buchuitoudegou mentioned this pull request May 27, 2022

dm: continuous data validation #4426

Open

38 tasks

lance6716 approved these changes May 27, 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 May 27, 2022

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

add case for multiple source

fed53e2

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

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

Merge branch 'master' into start-from-time

01d3a3b

ti-chi-bot merged commit d957fe8 into pingcap:master May 27, 2022

D3Hunter deleted the start-from-time branch May 27, 2022 05:22

	return terror.Annotate(err, "error while parse start-time, expected in the format like '2006-01-02 15:04:05' or '2006-01-02T15:04:05'")
	return terror.Annotate(err, "error while parsing start-time, expected formats are like '2006-01-02 15:04:05' or '2006-01-02T15:04:05'")

validator(dm): support start from time & fix & integration test #5534

validator(dm): support start from time & fix & integration test #5534

Conversation

D3Hunter commented May 23, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Tests

Questions

Will it cause performance regression or break compatibility?

Do you need to update user documentation, design documentation or monitoring documentation?

Release note

ti-chi-bot commented May 23, 2022 • edited Loading

D3Hunter commented May 23, 2022

D3Hunter commented May 23, 2022

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

buchuitoudegou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

D3Hunter May 24, 2022 • 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

D3Hunter commented May 24, 2022

D3Hunter commented May 24, 2022

D3Hunter commented May 24, 2022

D3Hunter commented May 24, 2022

D3Hunter commented May 24, 2022

codecov-commenter commented May 24, 2022 • edited Loading

Codecov Report

D3Hunter commented May 26, 2022

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 May 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

D3Hunter commented May 26, 2022

D3Hunter commented May 26, 2022

D3Hunter commented May 26, 2022

buchuitoudegou 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance6716 commented May 27, 2022

D3Hunter commented May 27, 2022

D3Hunter commented May 27, 2022

ti-chi-bot commented May 27, 2022

D3Hunter commented May 27, 2022

D3Hunter commented May 23, 2022 •

edited

Loading

ti-chi-bot commented May 23, 2022 •

edited

Loading

D3Hunter May 24, 2022 •

edited

Loading

codecov-commenter commented May 24, 2022 •

edited

Loading

lance6716 May 26, 2022 •

edited

Loading