dmctl: fix handle err commands #1746

lichunzhu · 2021-06-03T06:38:49Z

What problem does this PR solve?

#1692 part 1

What is changed and how it works?

Add binlog, shard-ddl-locks, binlog-schema, config commands and update relative integration tests.

Check List

Tests

Unit test
Integration test

Code changes

Has exported function/method change

Related changes

Need to cherry-pick to the release branch
Need to update the documentation

ti-chi-bot · 2021-06-03T06:38:50Z

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

…ErrCommands

Ehco1996

wiil review later

okJiang · 2021-07-27T02:14:11Z

dm/ctl/master/config.go

+	cmd := &cobra.Command{
+		Use:   "task [task-name]",
+		Short: "manage or show task configs",
+		RunE:  configTaskList,


Maybe use anonymous functions in the same format as above?

which format?

here?

https://github.com/pingcap/dm/pull/1746/files/4380557bc6f5ccd932934bd92325d6b6ea6be9ef#diff-fd7922b00c9e44d86f571af7fd5bd503e038c8576214ae84fab6970c5684ed49R78-R84

use func(cmd *cobra.Command, args []string) error instead of configTaskList

…into fixHandleErrCommands

lance6716 · 2021-08-10T07:03:10Z

@lichunzhu new comments added, PTAL

glorv

rest LGTM

glorv · 2021-08-11T03:06:20Z

dm/ctl/master/config.go

+		newExportCfgsCmd(),
+		newImportCfgsCmd(),
+	)
+	cmd.PersistentFlags().StringP("path", "d", "", "specify the file path to export`")


Suggested change

cmd.PersistentFlags().StringP("path", "d", "", "specify the file path to export`")

cmd.PersistentFlags().StringP("path", "p", "", "specify the file path to export`")

glorv · 2021-08-11T03:08:51Z

dm/ctl/master/config.go

+}
+
+func configMasterList(cmd *cobra.Command, args []string) error {
+	if len(args) == 0 || len(args) > 1 {


Suggested change

if len(args) == 0 || len(args) > 1 {

if len(args) != 1 {

glorv · 2021-08-11T03:09:52Z

dm/ctl/master/config.go

 		RunE:  exportCfgsFunc,
 	}
-	cmd.Flags().StringP("dir", "d", "configs", "specify the output directory, default is `./configs`")
+	cmd.Flags().String("dir", "", "specify the configs directory, default is `./configs`")


why remove the short parameter -d

glorv · 2021-08-11T03:13:16Z

dm/ctl/master/config.go

 	return cmd
 }

 // exportCfgsFunc exports configs.
 func exportCfgsFunc(cmd *cobra.Command, args []string) error {
-	dir, err := cmd.Flags().GetString("dir")
+	filePath, err := cmd.Flags().GetString("path")


I'm a bit confused by this path parameter. It seems like a single file path, but actually it means the path to a directory. Then why we need to support both path and dir while there is no difference

We used to support dir parameter. To keep compatible with older version we support both now but mark dir as hidden.

glorv · 2021-08-11T03:16:21Z

dm/ctl/master/shard_ddl_lock.go

+)
+
+// NewShardDDLLockCmd creates a ShardDDLLock command.
+func NewShardDDLLockCmd() *cobra.Command {


Not sure if we can merge shard-ddl-lock and show-ddl-lock into one command

Any suggestion?

Maybe shard-ddl-lock show?

lance6716 · 2021-08-12T07:48:50Z

@lichunzhu new comments added, PTAL

lichunzhu · 2021-08-16T08:53:14Z

@GMHDBJD @glorv PTAL again

GMHDBJD

LGTM

Ehco1996 · 2021-08-17T00:55:14Z

@lichunzhu please merge master and resolve conflits

…ErrCommands

lance6716 · 2021-08-18T04:55:47Z

/merge

ti-chi-bot · 2021-08-18T04:55:49Z

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

Commit hash: 72226d1

ti-chi-bot · 2021-08-18T05:10:35Z

In response to a cherrypick label: new pull request created: #2011.

lichunzhu added 3 commits June 2, 2021 19:27

first commit

f245398

merge master and resolve conflicts

4e173c8

update tests and ctl code

cefdcf6

lichunzhu added the needs-cherry-pick-release-2.0 This PR should be cherry-picked to release-2.0. Remove this label after cherry-picked to release-2.0 label Jun 3, 2021

ti-chi-bot added the do-not-merge/work-in-progress label Jun 3, 2021

ti-chi-bot requested review from GMHDBJD and lance6716 June 3, 2021 06:39

ti-chi-bot added the size/XXL label Jun 3, 2021

lichunzhu added 4 commits June 3, 2021 01:39

Merge branch 'master' into fixHandleErrCommands

bc631b0

try to fix tests

cb9c6f8

Merge branch 'master' of https://github.com/pingcap/dm into fixHandle…

e32ea81

…ErrCommands

fix help_cnt

f7616d8

lichunzhu marked this pull request as ready for review June 4, 2021 12:16

ti-chi-bot removed the do-not-merge/work-in-progress label Jun 4, 2021

lichunzhu added the status/PTAL This PR is ready for review. Add this label back after committing new changes label Jun 4, 2021

Ehco1996 reviewed Jun 4, 2021

View reviewed changes

lance6716 added this to the v2.0.5 milestone Jun 7, 2021

Merge branch 'master' into fixHandleErrCommands

4380557

ti-chi-bot added the needs-rebase label Jun 15, 2021

lichunzhu mentioned this pull request Jul 20, 2021

downgrade: add import/export-configs command #1877

Merged

lichunzhu removed this from the v2.0.5 milestone Jul 23, 2021

okJiang reviewed Jul 27, 2021

View reviewed changes

lance6716 removed the status/PTAL This PR is ready for review. Add this label back after committing new changes label Jul 29, 2021

lichunzhu added 2 commits July 30, 2021 10:37

resolve conflicts

48301b4

Merge branch 'fixHandleErrCommands' of https://github.com/lichunzhu/dm …

5f8ea2f

…into fixHandleErrCommands

ti-chi-bot removed the needs-rebase label Jul 30, 2021

Merge branch 'master' into fixHandleErrCommands

573e62a

lance6716 added this to the v2.0.6 milestone Jul 30, 2021

lance6716 added the needs-update-docs Should update docs after this PR is merged. Remove this label once the docs are updated label Jul 30, 2021

glorv reviewed Aug 11, 2021

View reviewed changes

GMHDBJD modified the milestones: v2.0.6, v2.1.0 Aug 13, 2021

ti-chi-bot added the needs-rebase label Aug 13, 2021

lichunzhu added 2 commits August 16, 2021 16:47

address comments

12d4c42

merge master and resolve conflicts

a38dbaa

ti-chi-bot removed the needs-rebase label Aug 16, 2021

GMHDBJD approved these changes Aug 16, 2021

View reviewed changes

ti-chi-bot added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Aug 16, 2021

fix tests

fe19681

ti-chi-bot added the needs-rebase label Aug 17, 2021

Merge branch 'master' of https://github.com/pingcap/dm into fixHandle…

8989ba6

…ErrCommands

ti-chi-bot removed the needs-rebase label Aug 17, 2021

Merge branch 'master' into fixHandleErrCommands

72226d1

ti-chi-bot added the status/can-merge label Aug 18, 2021

Merge branch 'master' into fixHandleErrCommands

b949151

ti-chi-bot merged commit a9bd623 into pingcap:master Aug 18, 2021

ti-chi-bot pushed a commit to ti-chi-bot/dm that referenced this pull request Aug 18, 2021

dmctl: fix handle err commands (pingcap#1746)

381b20b

ti-chi-bot mentioned this pull request Aug 18, 2021

dmctl: fix handle err commands (#1746) #2011

Merged

lichunzhu deleted the fixHandleErrCommands branch August 18, 2021 05:18

ti-chi-bot added a commit that referenced this pull request Aug 18, 2021

dmctl: fix handle err commands (#1746) (#2011)

af7f8d8

lance6716 mentioned this pull request Dec 30, 2021

syncer(dm): enhance error handling about binlog and binlog-schema command pingcap/tiflow#4017

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dmctl: fix handle err commands #1746

dmctl: fix handle err commands #1746

lichunzhu commented Jun 3, 2021 •

edited

Loading

ti-chi-bot commented Jun 3, 2021 •

edited

Loading

Ehco1996 left a comment •

edited

Loading

okJiang Jul 27, 2021

lichunzhu Jul 30, 2021

okJiang Aug 2, 2021

lance6716 commented Aug 10, 2021

glorv left a comment

glorv Aug 11, 2021

glorv Aug 11, 2021

glorv Aug 11, 2021

glorv Aug 11, 2021

lichunzhu Aug 16, 2021

glorv Aug 11, 2021

lichunzhu Aug 16, 2021

GMHDBJD Aug 16, 2021

lance6716 commented Aug 12, 2021

lichunzhu commented Aug 16, 2021

GMHDBJD left a comment

Ehco1996 commented Aug 17, 2021

lance6716 commented Aug 18, 2021

ti-chi-bot commented Aug 18, 2021

ti-chi-bot commented Aug 18, 2021

	cmd.PersistentFlags().StringP("path", "d", "", "specify the file path to export`")
	cmd.PersistentFlags().StringP("path", "p", "", "specify the file path to export`")

dmctl: fix handle err commands #1746

dmctl: fix handle err commands #1746

Conversation

lichunzhu commented Jun 3, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

ti-chi-bot commented Jun 3, 2021 • edited Loading

Ehco1996 left a comment • 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

lance6716 commented Aug 10, 2021

glorv 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

lance6716 commented Aug 12, 2021

lichunzhu commented Aug 16, 2021

GMHDBJD left a comment

Choose a reason for hiding this comment

Ehco1996 commented Aug 17, 2021

lance6716 commented Aug 18, 2021

ti-chi-bot commented Aug 18, 2021

ti-chi-bot commented Aug 18, 2021

lichunzhu commented Jun 3, 2021 •

edited

Loading

ti-chi-bot commented Jun 3, 2021 •

edited

Loading

Ehco1996 left a comment •

edited

Loading