checker(dm): make `start-task/resume-task/check-task` return pre-check result only warning #4118

okJiang · 2021-12-29T02:43:58Z

What problem does this PR solve?

Issue Number: ref #3608

What is changed and how it works?

as title

Check List

Tests

Integration test

Related changes

Need to update the documentation

Release note

Please add a release note.
If you don't think this PR needs a release note then fill it with `None`.

ti-chi-bot · 2021-12-29T02:43:59Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

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

okJiang · 2022-01-04T09:16:40Z

dm/tests/dmctl_basic/run.sh

+	run_dm_ctl $WORK_DIR "127.0.0.1:$MASTER_PORT" \
+		"start-task $cur/conf/only_warning.yaml" \
+		"\"state\": \"warn\"" 1


How about this format? @lance6716

{ "result": true, "msg": "fail to check synchronization configuration with type: no errors but some warnings detail: { "results": [ { "id": 7, "name": "table structure compatibility check", "desc": "check compatibility of table structure", "state": "warn", "errors": [ { "severity": "warn", "short_error": "table `dmctl`.`only_warning` Foreign Key only_warning_ibfk_1 is parsed but ignored by TiDB.", "instruction": "please ref document: https://docs.pingcap.com/tidb/stable/mysql-compatibility#unsupported-features" } ], "extra": "address of db instance - 127.0.0.1:3306" } ], "summary": { "passed": true, "total": 8, "successful": 7, "failed": 0, "warning": 1 } }", "sources": [ { "result": true, "msg": "", "source": "mysql-replica-01", "worker": "worker1" } ] }

okJiang · 2022-01-06T07:53:53Z

/run-dm-integration-test

okJiang · 2022-01-07T01:52:03Z

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

okJiang · 2022-01-07T02:14:28Z

/run-dm-integration-test

okJiang · 2022-01-07T02:25:13Z

/run-dm-integration-test

okJiang · 2022-01-07T02:46:28Z

/run-dm-integration-test

okJiang · 2022-01-07T08:01:06Z

dm/.golangci.yml

@@ -81,6 +81,9 @@ linters:
  # - maligned

 linters-settings:
+  dupl:


see details, golangci/golangci-lint#1372

okJiang · 2022-01-07T08:16:08Z

/run-all-tests

okJiang · 2022-01-07T08:53:27Z

/run-dm-integration-test
/run-integration-test
/run-kafka-integration-test

okJiang · 2022-01-10T01:33:43Z

/run-all-tests

okJiang · 2022-01-10T02:05:23Z

this pr is ready to review. /cc @lance6716 @Ehco1996

lance6716

rest lgtm (I'm checking output of dmctl)

lance6716 · 2022-01-11T03:20:17Z

dm/checker/cmd.go

 	}
 	defer c.Close()

 	pr := make(chan pb.ProcessResult, 1)
 	c.Process(ctx, pr)
-	for len(pr) > 0 {
+	if len(pr) > 0 {
 		r := <-pr


how about we always assume there is one result in pr, as stated in

tiflow/dm/dm/unit/unit.go

Lines 44 to 47 in d8dab8d

// Process does the main logic and its returning must send a result to pr channel.

// When ctx.Done, stops the process and returns, otherwise the DM-worker will be blocked forever

// When not in processing, call Process to continue or resume the process

Process(ctx context.Context, pr chan pb.ProcessResult)

It can be seen that checker is designed according to the unit at the beginning. But it is not in fact(not complete unit interface). And I think we don't plan to treat him as a unit in the future yet. So it is a semi-finished product. This is the reason that it looks a lot unreasonable.

I think we can modify these unreasonable places after the end of this feature. Result is also. @Ehco1996

actully it depends on requirements, i am not sure if we have a plan to merge full-time/sync-time sync_diff in to checker? if so i thinks it's better to treat checker as a unit

dm/checker/checker.go

dm/checker/cmd.go

Ehco1996 · 2022-01-12T02:00:07Z

dm/dm/master/openapi.go

@@ -445,7 +445,7 @@ func (s *Server) DMAPIStartTask(c *gin.Context) {
 	for i := range subTaskConfigList {
 		subTaskConfigPList[i] = &subTaskConfigList[i]
 	}
-	if err = checker.CheckSyncConfigFunc(newCtx, subTaskConfigPList,
+	if _, err = checker.CheckSyncConfigFunc(newCtx, subTaskConfigPList,


so user use openapi to start-task can not see the warnings ?, i thinks we need log theso these warnnings at leaset

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

dm/dm/master/openapi.go

Ehco1996 · 2022-01-12T03:12:50Z

/merge

ti-chi-bot · 2022-01-12T03:12:52Z

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

Commit hash: ce09d4c

ti-chi-bot · 2022-01-12T03:13:05Z

@okJiang: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

codecov-commenter · 2022-01-12T04:34:31Z

Codecov Report

Merging #4118 (a64ef81) into master (08da001) will increase coverage by 0.2059%.
The diff coverage is 48.5294%.

Flag	Coverage Δ
cdc	`59.2320% <63.1722%> (+0.5872%)`	⬆️
dm	`52.1387% <35.3086%> (-0.1158%)`	⬇️

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

@@               Coverage Diff                @@
##             master      #4118        +/-   ##
================================================
+ Coverage   55.1722%   55.3781%   +0.2059%     
================================================
  Files           485        495        +10     
  Lines         59829      61015      +1186     
================================================
+ Hits          33009      33789       +780     
- Misses        23484      23830       +346     
- Partials       3336       3396        +60

…k result only warning (pingcap#4118) ref pingcap#3608

okJiang added 2 commits December 28, 2021 20:14

save work

e8d865f

save work

cb7d6ef

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

okJiang added 2 commits December 29, 2021 16:47

save work

85a0fd4

save work

d13e6ff

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 Jan 4, 2022

okJiang added 2 commits January 4, 2022 17:04

save work

d09ecab

revert error type

c000cd5

okJiang commented Jan 4, 2022

View reviewed changes

fix ut

1edde50

okJiang added 2 commits January 7, 2022 12:02

fix lint

0574595

fix false positive golingci-lint(dupl)

c41787d

okJiang commented Jan 7, 2022

View reviewed changes

okJiang added 2 commits January 7, 2022 17:50

Merge branch 'master' into pre-check-return-warnings

fb60433

Merge branch 'master' into pre-check-return-warnings

e16f98d

okJiang marked this pull request as ready for review January 10, 2022 02:04

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 10, 2022

lance6716 reviewed Jan 11, 2022

View reviewed changes

lance6716 approved these changes Jan 11, 2022

View reviewed changes

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

Ehco1996 reviewed Jan 12, 2022

View reviewed changes

dm/checker/checker.go Show resolved Hide resolved

Ehco1996 reviewed Jan 12, 2022

View reviewed changes

dm/checker/cmd.go Outdated Show resolved Hide resolved

Ehco1996 reviewed Jan 12, 2022

View reviewed changes

okJiang and others added 3 commits January 12, 2022 10:36

Update dm/checker/cmd.go

0c1792a

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

add warning log in openapi

b64d32a

Merge branch 'master' into pre-check-return-warnings

ce09d4c

Ehco1996 reviewed Jan 12, 2022

View reviewed changes

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

Ehco1996 approved these changes Jan 12, 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 Jan 12, 2022

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

Merge branch 'master' into pre-check-return-warnings

a64ef81

ti-chi-bot merged commit 750816f into pingcap:master Jan 12, 2022

zhaoxinyu pushed a commit to zhaoxinyu/ticdc that referenced this pull request Jan 20, 2022

checker(dm): make start-task/resume-task/check-task return pre-chec…

e210926

…k result only warning (pingcap#4118) ref pingcap#3608

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

checker(dm): make `start-task/resume-task/check-task` return pre-check result only warning #4118

checker(dm): make `start-task/resume-task/check-task` return pre-check result only warning #4118

okJiang commented Dec 29, 2021 •

edited

Loading

ti-chi-bot commented Dec 29, 2021 •

edited

Loading

okJiang Jan 4, 2022

okJiang commented Jan 6, 2022

okJiang commented Jan 7, 2022

okJiang commented Jan 7, 2022

okJiang commented Jan 7, 2022

okJiang commented Jan 7, 2022

okJiang Jan 7, 2022

okJiang commented Jan 7, 2022

okJiang commented Jan 7, 2022

okJiang commented Jan 10, 2022

okJiang commented Jan 10, 2022

lance6716 left a comment

lance6716 Jan 11, 2022

okJiang Jan 12, 2022

Ehco1996 Jan 12, 2022

Ehco1996 Jan 12, 2022

Ehco1996 commented Jan 12, 2022

ti-chi-bot commented Jan 12, 2022

ti-chi-bot commented Jan 12, 2022

codecov-commenter commented Jan 12, 2022

	// Process does the main logic and its returning must send a result to pr channel.
	// When ctx.Done, stops the process and returns, otherwise the DM-worker will be blocked forever
	// When not in processing, call Process to continue or resume the process
	Process(ctx context.Context, pr chan pb.ProcessResult)

checker(dm): make start-task/resume-task/check-task return pre-check result only warning #4118

checker(dm): make start-task/resume-task/check-task return pre-check result only warning #4118

Conversation

okJiang commented Dec 29, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

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

okJiang Jan 4, 2022

Choose a reason for hiding this comment

okJiang commented Jan 6, 2022

okJiang commented Jan 7, 2022

okJiang commented Jan 7, 2022

okJiang commented Jan 7, 2022

okJiang commented Jan 7, 2022

okJiang Jan 7, 2022

Choose a reason for hiding this comment

okJiang commented Jan 7, 2022

okJiang commented Jan 7, 2022

okJiang commented Jan 10, 2022

okJiang commented Jan 10, 2022

lance6716 left a comment

Choose a reason for hiding this comment

lance6716 Jan 11, 2022

Choose a reason for hiding this comment

okJiang Jan 12, 2022

Choose a reason for hiding this comment

Ehco1996 Jan 12, 2022

Choose a reason for hiding this comment

Ehco1996 Jan 12, 2022

Choose a reason for hiding this comment

Ehco1996 commented Jan 12, 2022

ti-chi-bot commented Jan 12, 2022

ti-chi-bot commented Jan 12, 2022

codecov-commenter commented Jan 12, 2022

Codecov Report

checker(dm): make `start-task/resume-task/check-task` return pre-check result only warning #4118

checker(dm): make `start-task/resume-task/check-task` return pre-check result only warning #4118

okJiang commented Dec 29, 2021 •

edited

Loading

ti-chi-bot commented Dec 29, 2021 •

edited

Loading