Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nomycnf: simplify db parameters #4088

Merged
merged 5 commits into from Jul 17, 2018
Merged

nomycnf: simplify db parameters #4088

merged 5 commits into from Jul 17, 2018

Conversation

@sougou
Copy link
Member

@sougou sougou commented Jul 16, 2018

This is the first step towards vttablet not depending on mycnf. This PR makes the following changes:

  • If host or socket file is specified in the command line, they supersede what's in mycnf.
  • DB parameters have separated into two groups: connectivity parameters, and user credentials. Connectivity parameters are combined with user credentials to build the the mysql connection parameters.
  • A new per-user use_ssl flag has been added that lets you choose which connections must use SSL. This allows you to selectively enable SSL for certain connections. The use case for this is the repl connection, because replication can go over open networks.
  • New simpler flag names like db_host, etc. have now been introduced.
  • Legacy flag names still work like before, but they will be deprecated soon.
  • Reasonable defaults have been provided for all flags. One doesn't have to specify any connection parameters under normal circumstances, like the case where mysqlctl initialized the mysql.
  • DBConfigs has been solidified. The app needs to specify whether it wants connectivity with or without the database. This greatly reduces ambiguity about whether we're connected to just the server or to a specific database.
  • Examples have been simplified: all db config parameters have been removed.
  • vtqueryserver has its own db_name flag. It's the only case where you need to specify a db name on the command line.
  • A test has been added in tabletmanager.py to test and demonstrate the new flags.
sougou added 3 commits Jul 15, 2018
* Introduce new db connectivity flags.
* Mark old flags as deprecated.
* If new flags are set, they supersede legacy flags.
* Provide defaults to existing flags so one doesn't
  have to specify them for normal cases.

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
DBConfigs API has been changed:
* Conn param members have been made private and are not changeable.
* New functions like AppWithDB, etc. have been added. They return
  newly allocated conn params.
* New DBName field can now be set by the app, which can then be
  combined with conn params as needed.
* SideCarDBName and DBName are atomic. There's no worry about possible
  race while accessing them.
* 'WithDB' functions return a conn with the db name set. For cases
  where a db is always needed, like App, the API only provides a
  WithDB function. Also, vice-versa.
* A test has been added to tabletmanager to show that the new
  flags work.

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
* Remove legacy parameters from examples.
* Add comments.

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
@sougou sougou requested review from alainjobart and demmer Jul 16, 2018
@sougou
Copy link
Member Author

@sougou sougou commented Jul 16, 2018

It's possible one may want to enable SSL for just the repl
user because replication can happen on open networks.
The per-user use_ssl flag allows you to select which
connections use ssl and which don't.

DBConfigs has been refactored to use a map instead of
hardcoded user configs. This reduces boilerplating.

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Copy link
Contributor

@jvaidya jvaidya left a comment

Looks great on the whole. This will really reduce a lot of confusion, especially once the deprecated parameters are deleted. Thanks for tackling this.
I have a couple comments inline.

SidecarDBName string
// The flags will change the global singleton
// TODO(sougou): deprecate the legacy flags.
func registerUserFlags(dbc *userConfig, name string) {

This comment has been minimized.

@jvaidya

jvaidya Jul 17, 2018
Contributor

  • Rename to registerPerUserFlags?
  • Rename "name" to "username"?

This comment has been minimized.

@sougou

sougou Jul 17, 2018
Author Member

I can call it key or userKey, because the actual username is a field inside the struct.

}
dbConfigs.SidecarDBName.Set("_vt")

This comment has been minimized.

@jvaidya

jvaidya Jul 17, 2018
Contributor

Use the command line parameter rather than hard-coding "_vt".

This comment has been minimized.

@sougou

sougou Jul 17, 2018
Author Member

It's the eventual intent.

The problem is that "_vt" is hard-coded all over the place. Before we can do the command-line, we need to refactor those places to use the sidecar name field.

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Copy link
Contributor

@jvaidya jvaidya left a comment

Thanks for addressing the feedback. LGTM.

@sougou sougou merged commit 0dfff0e into vitessio:master Jul 17, 2018
4 checks passed
4 checks passed
@dco
DCO All commits have a DCO sign-off from the author
Details
codeclimate 4 fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants