Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

config: fix empty flavor for source #1385

Merged
merged 7 commits into from Jan 21, 2021
Merged

config: fix empty flavor for source #1385

merged 7 commits into from Jan 21, 2021

Conversation

GMHDBJD
Copy link
Collaborator

@GMHDBJD GMHDBJD commented Jan 18, 2021

What problem does this PR solve?

close #1380 #1379

What is changed and how it works?

  • adjust source config before verify
  • try mysql/mariadb if flavor is empty

Check List

Tests

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

@GMHDBJD GMHDBJD added type/bug-fix Bug fix needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated 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 status/WIP This PR is still work in progress labels Jan 18, 2021
@GMHDBJD GMHDBJD added status/PTAL This PR is ready for review. Add this label back after committing new changes and removed status/WIP This PR is still work in progress labels Jan 18, 2021
@lance6716
Copy link
Collaborator

/lgtm

@ti-srebot ti-srebot added the status/LGT1 One reviewer already commented LGTM label Jan 18, 2021
@lance6716
Copy link
Collaborator

/lgtm cancel

@ti-srebot ti-srebot removed the status/LGT1 One reviewer already commented LGTM label Jan 18, 2021
@ti-srebot
Copy link

@lance6716,cancel success.

@lance6716
Copy link
Collaborator

/lgtm

@ti-srebot ti-srebot added the status/LGT1 One reviewer already commented LGTM label Jan 18, 2021
lance6716
lance6716 reviewed Jan 18, 2021
View reviewed changes
dm/worker/source.yaml
enable-gtid: false
enable-gtid: true

relay-binlog-gtid: "e68f6068-53ec-11eb-9c5f-0242ac110003:1-50"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's a case that user didn't specify flavor or gtid, expecting starting relay at position of SHOW MASTER STATUS. Will SHOW MASTER STATUS returned empty Executed_Gtid_Set? If so, error message of "empty flavor with empty gtid is invalid" is misleading because he could only left them empty to use position of SHOW MASTER STATUS

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The flavor should not be empty since we have adjusted it or specify it, otherwise it is a bug. So empty flavor with empty gtid is invalid should not happen.

@pingcap pingcap deleted a comment from ti-srebot Jan 18, 2021
@GMHDBJD GMHDBJD added this to the v2.0.2 milestone Jan 19, 2021
@GMHDBJD
Copy link
Collaborator Author

GMHDBJD commented Jan 19, 2021

/run-all-tests

5 similar comments
@GMHDBJD
Copy link
Collaborator Author

GMHDBJD commented Jan 19, 2021

/run-all-tests

@GMHDBJD
Copy link
Collaborator Author

GMHDBJD commented Jan 20, 2021

/run-all-tests

@GMHDBJD
Copy link
Collaborator Author

GMHDBJD commented Jan 20, 2021

/run-all-tests

@GMHDBJD
Copy link
Collaborator Author

GMHDBJD commented Jan 20, 2021

/run-all-tests

@zhouqiang-cl
Copy link
Contributor

/run-all-tests

@lance6716
Copy link
Collaborator

PTAL @lichunzhu @3pointer

3pointer
3pointer approved these changes Jan 21, 2021
View reviewed changes
Copy link
Contributor

@3pointer 3pointer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@GMHDBJD GMHDBJD merged commit 65d3962 into pingcap:master Jan 21, 2021
ti-srebot pushed a commit to ti-srebot/dm that referenced this pull request Jan 21, 2021
@GMHDBJD @ti-srebot
cherry pick pingcap#1385 to release-2.0
be6a3c9 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Jan 21, 2021
Merged
@ti-srebot
Copy link

cherry pick to release-2.0 in PR #1395

@ti-srebot ti-srebot added already-cherry-pick-2.0 The related PR is already cherry-picked to release-2.0. Add this label once the PR is cherry-picked and removed 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 labels Jan 21, 2021
@GMHDBJD GMHDBJD deleted the ParseGTID branch January 21, 2021 07:53
GMHDBJD pushed a commit that referenced this pull request Jan 21, 2021
@ti-srebot
cherry pick #1385 to release-2.0 (#1395)
fd608ec
@lance6716 lance6716 mentioned this pull request Feb 3, 2021
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lance6716 lance6716 lance6716 left review comments

@3pointer 3pointer 3pointer approved these changes

Assignees
No one assigned
Labels
already-cherry-pick-2.0 The related PR is already cherry-picked to release-2.0. Add this label once the PR is cherry-picked needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated status/LGT1 One reviewer already commented LGTM status/PTAL This PR is ready for review. Add this label back after committing new changes type/bug-fix Bug fix
Projects
None yet
Milestone
v2.0.2
Development

Successfully merging this pull request may close these issues.

dmctl fail to operate-source when flavor is not specified and GTID is set
5 participants
@GMHDBJD @lance6716 @ti-srebot @zhouqiang-cl @3pointer