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

worker: generate index flavor automatically by querying version info from DB #323

Merged
merged 20 commits into from
Oct 21, 2019

Conversation

lichunzhu
Copy link
Contributor

@lichunzhu lichunzhu commented Oct 17, 2019

What problem does this PR solve?

Now, we need to set flavor for DM-worker manually, but it may be set incorrectly.

What is changed and how it works?

use dbutil.ShowVersion and check.IsMariaDB from tidb-tools to detect upstream MySQL/MariaDB server type and set flavor automatically.

Check List

Tests

  • Unit test
  • Integration test

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation
  • Need to be included in the release note

@lichunzhu lichunzhu added priority/normal Minor change, requires approval from ≥1 primary reviewer needs-update-docs Should update docs after this PR is merged. Remove this label once the docs are updated status/PTAL This PR is ready for review. Add this label back after committing new changes priority/release-blocker This PR blocks a release. Please review it ASAP. needs-cherry-pick-release-1.0 This PR should be cherry-picked to release-1.0. Remove this label after cherry-picked to release-1.0 needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated labels Oct 17, 2019
@lichunzhu
Copy link
Contributor Author

/run-all-tests

@codecov
Copy link

codecov bot commented Oct 18, 2019

Codecov Report

Merging #323 into master will increase coverage by 0.0254%.
The diff coverage is 53.125%.

@@               Coverage Diff                @@
##             master       #323        +/-   ##
================================================
+ Coverage   59.8923%   59.9177%   +0.0254%     
================================================
  Files           135        135                
  Lines         15047      15079        +32     
================================================
+ Hits           9012       9035        +23     
- Misses         5159       5163         +4     
- Partials        876        881         +5

dm/worker/config.go Outdated Show resolved Hide resolved
@lichunzhu
Copy link
Contributor Author

lichunzhu commented Oct 18, 2019

@csuzhangxc It works if we suppose customers only use mariaDB and mysql.
When we run select version seperately in mysql and mariaDB, mysql might be like:

+------------+
| @@version  |
+------------+
| 5.7.26-log |
+------------+

mariaDB might be like:

+--------------------------------------+
| @@version                            |
+--------------------------------------+
| 10.4.8-MariaDB-1:10.4.8+maria~bionic |
+--------------------------------------+

If there is a kind of database which supports select @@version command but is not neither mysql nor mariaDB, it would be judged as mysql.

@csuzhangxc
Copy link
Member

If there is a kind of database which supports select @@version command but is not neither mysql nor mariaDB, it would be judged as mysql.

I think it's fine now, if neither mysql nor mariadb, it will fail later.

@lichunzhu
Copy link
Contributor Author

So shall we change select @@version_comment to select @@version and use IsMariaDB?

@csuzhangxc
Copy link
Member

So shall we change select @@version_comment to select @@version and use IsMariaDB?

I think to change to version it better. @WangXiangUSTC what's your opinion?

dm/worker/config_test.go Outdated Show resolved Hide resolved
dm/worker/config.go Outdated Show resolved Hide resolved
pkg/utils/db.go Outdated Show resolved Hide resolved
dm/worker/config.go Outdated Show resolved Hide resolved
pkg/utils/db.go Outdated Show resolved Hide resolved
Copy link
Member

@csuzhangxc csuzhangxc left a comment

Choose a reason for hiding this comment

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

rest LGTM

dm/worker/config.go Outdated Show resolved Hide resolved
Copy link
Contributor

@WangXiangUSTC WangXiangUSTC left a comment

Choose a reason for hiding this comment

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

LGTM

@WangXiangUSTC WangXiangUSTC added status/LGT1 One reviewer already commented LGTM and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Oct 21, 2019
dm/worker/config.go Outdated Show resolved Hide resolved
dm/worker/config.go Outdated Show resolved Hide resolved
dm/worker/config.go Outdated Show resolved Hide resolved
dm/worker/config.go Outdated Show resolved Hide resolved
Co-Authored-By: Xuecheng Zhang <[email protected]>
Copy link
Member

@csuzhangxc csuzhangxc left a comment

Choose a reason for hiding this comment

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

LGTM

@csuzhangxc csuzhangxc added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Oct 21, 2019
@lichunzhu lichunzhu merged commit 2b2ff73 into pingcap:master Oct 21, 2019
@lichunzhu lichunzhu deleted the czli/dm-worker/simplifyFlavor branch October 21, 2019 08:38
@sre-bot
Copy link

sre-bot commented Oct 21, 2019

cherry pick to release-1.0 failed

@lichunzhu lichunzhu added type/cherry-pick This PR is just a cherry-pick (backport) needs-cherry-pick-release-1.0 This PR should be cherry-picked to release-1.0. Remove this label after cherry-picked to release-1.0 and removed needs-cherry-pick-release-1.0 This PR should be cherry-picked to release-1.0. Remove this label after cherry-picked to release-1.0 type/cherry-pick This PR is just a cherry-pick (backport) labels Oct 21, 2019
@csuzhangxc csuzhangxc added already-update-release-note The release note is updated. Add this label once the release note is updated and removed needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated labels Nov 1, 2019
@csuzhangxc csuzhangxc added already-cherry-pick-1.0 The related PR is already cherry-picked to release-1.0. Add this label once the PR is cherry-picked already-update-docs The docs related to this PR already updated. Add this label once the docs are updated and removed needs-cherry-pick-release-1.0 This PR should be cherry-picked to release-1.0. Remove this label after cherry-picked to release-1.0 needs-update-docs Should update docs after this PR is merged. Remove this label once the docs are updated labels Mar 13, 2020
lichunzhu added a commit to lichunzhu/dm that referenced this pull request Apr 6, 2020
…from DB (pingcap#323)

use dbutil.ShowVersion and check.IsMariaDB from tidb-tools to detect upstream MySQL/MariaDB server type and set flavor automatically.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
already-cherry-pick-1.0 The related PR is already cherry-picked to release-1.0. Add this label once the PR is cherry-picked already-update-docs The docs related to this PR already updated. Add this label once the docs are updated already-update-release-note The release note is updated. Add this label once the release note is updated priority/normal Minor change, requires approval from ≥1 primary reviewer priority/release-blocker This PR blocks a release. Please review it ASAP. status/LGT2 Two reviewers already commented LGTM, ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants