pkg(dm): improve the backward compatibility of “SHOW SLAVE HOSTS“ command #7372

lyzx2001 · 2022-10-14T14:14:10Z

What problem does this PR solve?

Issue Number: close #5017

What is changed and how it works?

I used export.GetSpecifiedColumnValueAndClose and export.GetSpecifiedColumnValuesAndClose to retrieve the requested columns, instead of restricting the return format of the columns, which is not robust.

Check List

Tests

Unit test

Questions

Will it cause performance regression or break compatibility?

No.

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

No.

Release note

None

ti-chi-bot · 2022-10-14T14:14:11Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

lance6716
lichunzhu

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.

lyzx2001 · 2022-10-14T14:15:37Z

/cc @lance6716

lyzx2001 · 2022-10-14T14:39:42Z

/run-all-tests

lance6716 · 2022-10-15T11:35:39Z

DM unit test is failed

ti-chi-bot · 2022-10-19T09:21:37Z

@lyzx2001: PR needs rebase.

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 kubernetes/test-infra repository.

lyzx2001 · 2022-10-26T11:16:10Z

/run-verify

lyzx2001 · 2022-10-26T12:13:46Z

/run-verify

lance6716 · 2022-10-27T01:39:54Z

the failed case is

[2022-10-26T13:29:09.902Z]     server_test.go:2888: 
[2022-10-26T13:29:09.902Z]         	Error Trace:	/tmp/TestMaster1418414956/001/server_test.go:2888
[2022-10-26T13:29:09.902Z]         	Error:      	Should be true
[2022-10-26T13:29:09.902Z]         	Test:       	TestMaster

https://github.com/lyzx2001/tiflow/blob/904eaf05927564427df3952c640e59d80fc5c043/dm/master/server_test.go#L2888

lyzx2001 · 2022-10-27T10:58:08Z

/run-verify

lyzx2001 · 2022-10-27T12:39:08Z

/run-dm-integration-test

lyzx2001 · 2022-10-27T12:39:47Z

/run-verify

dm/pkg/conn/utils.go

codecov-commenter · 2022-10-28T13:56:48Z

Codecov Report

Merging #7372 (1c100ab) into master (f4754e1) will increase coverage by 0.0434%.
The diff coverage is 35.7894%.

Additional details and impacted files

Flag	Coverage Δ
cdc	`67.5508% <ø> (+0.1033%)`	⬆️
dm	`51.9823% <51.6666%> (-0.0259%)`	⬇️
engine	`60.9714% <8.5714%> (+0.0028%)`	⬆️

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

@@               Coverage Diff                @@
##             master      #7372        +/-   ##
================================================
+ Coverage   59.8150%   59.8584%   +0.0434%     
================================================
  Files           803        803                
  Lines         92129      92291       +162     
================================================
+ Hits          55107      55244       +137     
- Misses        32213      32239        +26     
+ Partials       4809       4808         -1

lance6716 · 2022-11-04T02:56:09Z

dm/pkg/utils/db_test.go

+		// For MariaDB, when invalid conversion of Server_id (converting int32 in the upper-half of uint32 values to uint32)
+		{
+			sqlmock.NewRows([]string{"Server_id", "Host", "Port", "Master_id"}).
+				AddRow(-1, "iconnect2", 3306, 192168011).


-1 is not a valid server ID?

https://dev.mysql.com/doc/refman/5.7/en/replication-options.html#sysvar_server_id

lance6716 · 2022-11-04T02:57:51Z

dm/pkg/utils/db.go

+	}
+	for _, serverID := range rowsResult {
+		// serverID will not be null
+		serverIDInt, err := strconv.Atoi(serverID)


You can use https://pkg.go.dev/strconv#ParseUint, don't use int because its range is undetermined

lyzx2001 · 2022-11-09T07:40:58Z

/cc @GMHDBJD

lyzx2001 · 2022-11-10T07:12:36Z

/cc @lichunzhu

lichunzhu

LGTM

dm/pkg/conn/utils_test.go

lichunzhu · 2022-11-11T08:43:47Z

/merge

ti-chi-bot · 2022-11-11T08:43:50Z

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

Commit hash: 94f530a

ti-chi-bot · 2022-11-11T08:44:03Z

@lyzx2001: 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.

lyzx2001 · 2022-11-11T09:08:04Z

/run-verify

improve backward compatibility of 'SHOW SLAVE HOSTS' command

899f734

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 14, 2022

ti-chi-bot assigned lance6716 and unassigned lance6716 Oct 14, 2022

ti-chi-bot requested a review from lance6716 October 14, 2022 14:15

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 19, 2022

Merge branch 'master' into issue5017

cc3266f

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

improve backward compatibility of 'SHAOW SLAVE HOSTS' command

2dcd1fa

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2022

improve backward compatibility of 'SHAOW SLAVE HOSTS' command

49f4a58

improve backward compatibility of 'SHAOW SLAVE HOSTS' command

904eaf0

improve backward compatibility of 'SHAOW SLAVE HOSTS' command

b654076

improve backward compatibility of 'SHAOW SLAVE HOSTS' command

c5c9e8d

lance6716 reviewed Oct 28, 2022

View reviewed changes

dm/pkg/conn/utils.go Outdated Show resolved Hide resolved

dm/pkg/conn/utils.go Show resolved Hide resolved

improve backward compatibility of 'SHAOW SLAVE HOSTS' command

1a72e6a

lyzx2001 added 4 commits November 3, 2022 22:13

improve backward compatibility of 'SHAOW SLAVE HOSTS' command

bf3e38b

improve backward compatibility of 'SHAOW SLAVE HOSTS' command

c255c95

improve backward compatibility of 'SHAOW SLAVE HOSTS' command

4510e80

improve backward compatibility of 'SHAOW SLAVE HOSTS' command

cfe9a0c

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 Nov 3, 2022

improve backward compatibility of 'SHAOW SLAVE HOSTS' command

18519ad

lance6716 reviewed Nov 4, 2022

View reviewed changes

improve backward compatibility of 'SHAOW SLAVE HOSTS' command

ceb4210

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

lance6716 approved these changes Nov 7, 2022

View reviewed changes

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

ti-chi-bot requested a review from GMHDBJD November 9, 2022 07:40

ti-chi-bot requested a review from lichunzhu November 10, 2022 07:12

lichunzhu approved these changes Nov 10, 2022

View reviewed changes

dm/pkg/conn/utils_test.go Show resolved Hide resolved

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

lyzx2001 added 2 commits November 10, 2022 21:48

improve backward compatibility of 'SHOW SLAVE HOSTS' command

1c100ab

pkg(dm): improve backward compatibility of 'SHOW SLAVE HOSTS' command

94f530a

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

Merge branch 'master' into issue5017

afa3f8f

ti-chi-bot merged commit 04173f6 into pingcap:master Nov 11, 2022

lance6716 mentioned this pull request Jun 9, 2023

(dm) [bugfix] backward compatibility with show command #5017 #5018

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg(dm): improve the backward compatibility of “SHOW SLAVE HOSTS“ command #7372

pkg(dm): improve the backward compatibility of “SHOW SLAVE HOSTS“ command #7372

lyzx2001 commented Oct 14, 2022

ti-chi-bot commented Oct 14, 2022 •

edited

Loading

lyzx2001 commented Oct 14, 2022

lyzx2001 commented Oct 14, 2022

lance6716 commented Oct 15, 2022

ti-chi-bot commented Oct 19, 2022

lyzx2001 commented Oct 26, 2022

lyzx2001 commented Oct 26, 2022

lance6716 commented Oct 27, 2022 •

edited

Loading

lyzx2001 commented Oct 27, 2022

lyzx2001 commented Oct 27, 2022

lyzx2001 commented Oct 27, 2022

codecov-commenter commented Oct 28, 2022 •

edited

Loading

lance6716 Nov 4, 2022

lance6716 Nov 4, 2022

lyzx2001 commented Nov 9, 2022

lyzx2001 commented Nov 10, 2022

lichunzhu left a comment

lichunzhu commented Nov 11, 2022

ti-chi-bot commented Nov 11, 2022

ti-chi-bot commented Nov 11, 2022

lyzx2001 commented Nov 11, 2022

pkg(dm): improve the backward compatibility of “SHOW SLAVE HOSTS“ command #7372

pkg(dm): improve the backward compatibility of “SHOW SLAVE HOSTS“ command #7372

Conversation

lyzx2001 commented Oct 14, 2022

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 Oct 14, 2022 • edited Loading

lyzx2001 commented Oct 14, 2022

lyzx2001 commented Oct 14, 2022

lance6716 commented Oct 15, 2022

ti-chi-bot commented Oct 19, 2022

lyzx2001 commented Oct 26, 2022

lyzx2001 commented Oct 26, 2022

lance6716 commented Oct 27, 2022 • edited Loading

lyzx2001 commented Oct 27, 2022

lyzx2001 commented Oct 27, 2022

lyzx2001 commented Oct 27, 2022

codecov-commenter commented Oct 28, 2022 • edited Loading

Codecov Report

lance6716 Nov 4, 2022

Choose a reason for hiding this comment

lance6716 Nov 4, 2022

Choose a reason for hiding this comment

lyzx2001 commented Nov 9, 2022

lyzx2001 commented Nov 10, 2022

lichunzhu left a comment

Choose a reason for hiding this comment

lichunzhu commented Nov 11, 2022

ti-chi-bot commented Nov 11, 2022

ti-chi-bot commented Nov 11, 2022

lyzx2001 commented Nov 11, 2022

ti-chi-bot commented Oct 14, 2022 •

edited

Loading

lance6716 commented Oct 27, 2022 •

edited

Loading

codecov-commenter commented Oct 28, 2022 •

edited

Loading