DM/Openapi: support start task by some conditions #5349

WizardXiao · 2022-05-06T17:40:10Z

What problem does this PR solve?

Issue Number: close #5348

What is changed and how it works?

add start_time and safe_mode_time_duration in open api.

Check List

Tests

Unit test
Integration test

Release note

None

ti-chi-bot · 2022-05-06T17:40:11Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

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

WizardXiao · 2022-05-06T17:41:37Z

/run-all-tests

WizardXiao · 2022-05-07T01:40:10Z

/cc @Ehco1996 @lance6716

Ehco1996

rest LGTM

dm/dm/config/task_converters.go

Ehco1996 · 2022-05-07T02:15:58Z

dm/openapi/spec/dm.yaml

+        safe_mode_time_duration:
+          type: string
+          example: "10s"
+          description: time duration of safe mode


we also support WaitTimeOnStop in cliArgs now

WaitTimeOnStop is not in the requirement, maybe i will add later pr and add some integration test.

Ehco1996 · 2022-05-07T02:20:19Z

dm/syncer/syncer.go

@@ -2185,7 +2185,7 @@ func (s *Syncer) initSafeModeExitTS(firstBinlogTS int64) error {
 		return err
 	}
 	s.firstMeetBinlogTS = &firstBinlogTS
-	exitTS := firstBinlogTS + int64(duration.Seconds())
+	exitTS := firstBinlogTS + int64(duration.Seconds())*1000


why need *1000?

yes, firstBinlogTS which comes from Timestamp is the value of millisecond，but duration is second. i find it when i run integration test.

you can use duration.Milliseconds() then

https://dev.mysql.com/doc/internals/en/binlog-event-header.html this should be the unit of second, not millisecond 🤔

sorry, I have tried again, the value is uint32, i will fix back. I may modify this because I encounter another problem，the ts may be 0 when the event type is ROTATE_EVENT and then i modify here and avoid 0, but here is wrong.

Ehco1996 · 2022-05-07T02:21:55Z

dm/dm/config/task_converters.go

@@ -684,3 +681,18 @@ func genFilterRuleName(sourceName string, idx int) string {
 	// NOTE that we don't have user input filter rule name in sub task config, so we make one by ourself
 	return fmt.Sprintf("%s-filter-rule-%d", sourceName, idx)
 }
+
+func OpenAPIStartTaskReqToTaskCliArgs(req openapi.StartTaskRequest) (*TaskCliArgs, error) {
+	cliArgs := &TaskCliArgs{}


how about return nil when all cli agrs are empty?

let me have a try.

…d-start-condition

…iflow into add-start-condition

…d-start-condition

WizardXiao · 2022-05-14T01:21:12Z

/run-all-tests

WizardXiao · 2022-05-14T03:26:51Z

/run-all-tests

WizardXiao · 2022-05-16T00:51:55Z

/run-all-tests

D3Hunter

will review test later

dm/dm/config/task_converters.go

dm/syncer/syncer.go

WizardXiao · 2022-05-16T08:31:29Z

dm/tests/openapi/run.sh

+	openapi_task_check "get_task_list" 0
+
+	# incremental task use gtid
+	prepare_database


test has source meta can exec

WizardXiao · 2022-05-16T08:32:17Z

dm/tests/openapi/run.sh

+	openapi_task_check "delete_task_with_force_success" "$task_name"
+	openapi_task_check "get_task_list" 0
+
+	# incremental task use start_time


test has no source meta but has start time can exec

WizardXiao · 2022-05-16T08:32:45Z

dm/tests/openapi/run.sh

+	openapi_task_check "get_task_list" 0
+
+	# incremental task both gtid and start_time, start_time first
+	prepare_database


test both has meta and start time, start time first.

WizardXiao · 2022-05-16T08:33:25Z

dm/tests/openapi/run.sh

+	openapi_task_check "get_task_list" 0
+
+	# incremental task no duration has error
+	export GO_FAILPOINTS='github.com/pingcap/tiflow/dm/syncer/SafeModeInitPhaseSeconds=return(0)'


test has no safe mode duration and has error

WizardXiao · 2022-05-16T08:33:51Z

dm/tests/openapi/run.sh

+		"query-status $task_name" \
+		"Duplicate entry" 2
+
+	# set duration and start again


test set duration and can exec

codecov-commenter · 2022-05-16T08:47:59Z

Codecov Report

Merging #5349 (fd8c8f9) into master (687e248) will decrease coverage by 0.4348%.
The diff coverage is 51.7090%.

Flag	Coverage Δ
cdc	`60.5260% <51.7090%> (-0.0726%)`	⬇️
dm	`51.9519% <ø> (-0.5177%)`	⬇️

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

@@               Coverage Diff                @@
##             master      #5349        +/-   ##
================================================
- Coverage   56.1240%   55.6891%   -0.4349%     
================================================
  Files           522        528         +6     
  Lines         65325      69430      +4105     
================================================
+ Hits          36663      38665      +2002     
- Misses        25094      27042      +1948     
- Partials       3568       3723       +155

Ehco1996

rest LGTM nice work

dm/dm/master/openapi_controller.go

dm/tests/openapi/run.sh

…d-start-condition

WizardXiao · 2022-05-16T10:26:09Z

dm/tests/openapi/run.sh

+	openapi_task_check "delete_task_with_force_success" "$task_name"
+	openapi_task_check "get_task_list" 0
+
+	# incremental task use start_time, but time is after create table


test has start time but time is too late and return error.

D3Hunter

rest lgtm

D3Hunter · 2022-05-16T10:36:25Z

dm/dm/master/openapi_controller.go

@@ -705,10 +731,34 @@ func (s *Server) stopTask(ctx context.Context, taskName string, req openapi.Stop
 		sourceNameList := openapi.SourceNameList(s.getTaskSourceNameList(taskName))
 		req.SourceNameList = &sourceNameList
 	}
-	// TODO(ehco): support stop req after https://github.com/pingcap/tiflow/pull/4601 merged
+	// handle task cli args
+	cliArgs, err := config.OpenAPIStopTasReqToTaskCliArgs(req)


Suggested change

cliArgs, err := config.OpenAPIStopTasReqToTaskCliArgs(req)

cliArgs, err := config.OpenAPIStopTaskReqToTaskCliArgs(req)

Ehco1996

LGTM plz fix typo before your merge

WizardXiao · 2022-05-16T12:37:23Z

/run-all-tests

WizardXiao · 2022-05-16T13:07:17Z

/run-dm-integration-test

WizardXiao · 2022-05-16T13:32:44Z

/run-dm-integration-test

WizardXiao · 2022-05-16T14:08:12Z

/merge

ti-chi-bot · 2022-05-16T14:08:15Z

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

Commit hash: dd14d08

WizardXiao · 2022-05-16T15:00:42Z

/run-dm-integration-test

WizardXiao · 2022-05-16T21:03:34Z

/run-dm-integration-test

WizardXiao · 2022-05-16T21:18:42Z

/run-dm-integration-test

commit-message: init

ade37a4

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 6, 2022

WizardXiao added area/dm Issues or PRs related to DM. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 6, 2022

Merge branch 'master' into add-start-condition

fbdda1f

ti-chi-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 6, 2022

WizardXiao added the status/ptal Could you please take a look? label May 6, 2022

ti-chi-bot requested review from Ehco1996 and lance6716 May 7, 2022 01:40

Ehco1996 reviewed May 7, 2022

View reviewed changes

WizardXiao added 10 commits May 7, 2022 15:18

commit-message: return nil when cli args is empty

903d115

Merge branch 'master' of https://github.com/WizardXiao/tiflow into ad…

20b7ab7

…d-start-condition

Merge branch 'add-start-condition' of https://github.com/WizardXiao/t…

2f3fbfc

…iflow into add-start-condition

commit-message: fix back duration ts caculate

dd630b9

commit-message: merge master

9fb6480

Merge branch 'master' of https://github.com/WizardXiao/tiflow into ad…

bf5ffec

…d-start-condition

commit-message: fix conflicts with master

503a294

Merge branch 'master' of https://github.com/WizardXiao/tiflow into ad…

7ba25be

…d-start-condition

commit-message: add integration test about wait stop

1feed83

Merge branch 'master' of https://github.com/WizardXiao/tiflow into ad…

406bb15

…d-start-condition

Merge branch 'master' into add-start-condition

8ed9422

Merge branch 'master' into add-start-condition

b656160

D3Hunter reviewed May 16, 2022

View reviewed changes

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

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

WizardXiao commented May 16, 2022

View reviewed changes

Ehco1996 reviewed May 16, 2022

View reviewed changes

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

dm/tests/openapi/run.sh Show resolved Hide resolved

WizardXiao added 4 commits May 16, 2022 17:17

commit-message: address some comments

7a1f4d7

Merge branch 'master' of https://github.com/WizardXiao/tiflow into ad…

3d20d7e

…d-start-condition

commit-message: add negative case

202e192

Merge branch 'master' of https://github.com/WizardXiao/tiflow into ad…

fd8c8f9

…d-start-condition

WizardXiao commented May 16, 2022

View reviewed changes

D3Hunter reviewed May 16, 2022

View reviewed changes

Ehco1996 approved these changes May 16, 2022

View reviewed changes

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

commit-message: fix function name

dd14d08

D3Hunter approved these changes May 16, 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 16, 2022

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

ti-chi-bot merged commit da4924f into pingcap:master May 16, 2022

WizardXiao mentioned this pull request May 17, 2022

Tracking issue for DM WebUI v3 #5442

Closed

5 tasks

	cliArgs, err := config.OpenAPIStopTasReqToTaskCliArgs(req)
	cliArgs, err := config.OpenAPIStopTaskReqToTaskCliArgs(req)

DM/Openapi: support start task by some conditions #5349

DM/Openapi: support start task by some conditions #5349

Conversation

WizardXiao commented May 6, 2022

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

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

WizardXiao commented May 6, 2022

WizardXiao commented May 7, 2022

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

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

WizardXiao commented May 14, 2022

WizardXiao commented May 14, 2022

WizardXiao commented May 16, 2022

D3Hunter 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

codecov-commenter commented May 16, 2022 • edited Loading

Codecov Report

Ehco1996 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

D3Hunter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ehco1996 left a comment

Choose a reason for hiding this comment

WizardXiao commented May 16, 2022

WizardXiao commented May 16, 2022

WizardXiao commented May 16, 2022

WizardXiao commented May 16, 2022

ti-chi-bot commented May 16, 2022

WizardXiao commented May 16, 2022

WizardXiao commented May 16, 2022

WizardXiao commented May 16, 2022

ti-chi-bot commented May 6, 2022 •

edited

Loading

codecov-commenter commented May 16, 2022 •

edited

Loading