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

config: support setting session variable #687

Merged
merged 8 commits into from
May 29, 2020
Merged

config: support setting session variable #687

merged 8 commits into from
May 29, 2020

Conversation

GMHDBJD
Copy link
Collaborator

@GMHDBJD GMHDBJD commented May 27, 2020

What problem does this PR solve?

Support setting session variables #677

What is changed and how it works?

add SessionConfig in TargetDB, set session variables in db.Apply.

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported function/method change
  • Has exported variable/fields change

@GMHDBJD GMHDBJD added status/WIP This PR is still work in progress type/feature New feature priority/normal Minor change, requires approval from ≥1 primary reviewer labels May 27, 2020
@codecov
Copy link

codecov bot commented May 27, 2020 •
edited
Loading

Codecov Report

Merging #687 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master       #687   +/-   ##
===========================================
  Coverage   56.9257%   56.9257%           
===========================================
  Files           205        205           
  Lines         20994      20994           
===========================================
  Hits          11951      11951           
  Misses         7884       7884           
  Partials       1159       1159

@GMHDBJD GMHDBJD added 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 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 May 27, 2020
csuzhangxc
csuzhangxc reviewed May 28, 2020
View reviewed changes
pkg/conn/basedb.go Outdated
@@ -56,10 +56,16 @@ func (d *DefaultDBProviderImpl) Apply(config config.DBConfig) (*BaseDB, error) {
}
maxIdleConns = rawCfg.MaxIdleConns
}

for key, val := range config.Session {
dsn += fmt.Sprintf("&%s=\"%s\"", key, val)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to use different formats for types of val? see https://github.com/go-sql-driver/mysql#system-variables

Copy link
Collaborator Author

@GMHDBJD GMHDBJD May 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Because we don't restrict user config, it's hard to judge the type of val.
I use ' for all types and url.QueryEscape to escape string in 36e8103. MySQL extracts the relevant type from a string.

Copy link
Contributor

Choose a reason for hiding this comment

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

can it work if add "" for number value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes，see

dm/tests/all_mode/conf/dm-task.yaml

Line 20 in 9ec51cb

tidb_retry_limit: "10"

Copy link
Member

Choose a reason for hiding this comment

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

MySQL extracts the relevant type from a string.

Are there any docs/links explain about this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems not official. go-sql-driver/mysql#405 (comment)

Copy link
Member

@csuzhangxc csuzhangxc May 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

👍 you can add this as a comment above the code (the link in the about comment has out of date because it's not a permanent link).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

add commet in 5d57a5b

tests/all_mode/conf/dm-task.yaml Show resolved Hide resolved
csuzhangxc
csuzhangxc reviewed May 28, 2020
View reviewed changes
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/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 May 28, 2020
WangXiangUSTC
WangXiangUSTC approved these changes May 29, 2020
View reviewed changes
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/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels May 29, 2020
@GMHDBJD GMHDBJD merged commit 33ae8d3 into pingcap:master May 29, 2020
@GMHDBJD GMHDBJD deleted the support-session branch May 29, 2020 03:20
@sre-bot sre-bot mentioned this pull request May 29, 2020
Closed
@sre-bot
Copy link

sre-bot commented May 29, 2020

cherry pick to release-1.0 in PR #690

@sre-bot sre-bot 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 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 labels May 29, 2020
This was referenced May 29, 2020
Closed
Merged
Closed
GMHDBJD added a commit that referenced this pull request May 29, 2020
@GMHDBJD
config: support setting session variable (#687) (#692)
c29e266
@csuzhangxc csuzhangxc added already-update-docs The docs related to this PR already updated. Add this label once the docs are updated and removed needs-update-docs Should update docs after this PR is merged. Remove this label once the docs are updated labels Jun 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@csuzhangxc csuzhangxc csuzhangxc left review comments

@WangXiangUSTC WangXiangUSTC WangXiangUSTC approved these changes

@lichunzhu lichunzhu Awaiting requested review from lichunzhu

Assignees
No one assigned
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 priority/normal Minor change, requires approval from ≥1 primary reviewer status/LGT2 Two reviewers already commented LGTM, ready for merge type/feature New feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@GMHDBJD @sre-bot @csuzhangxc @WangXiangUSTC