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

Avoid some pollution of CLI argument namespace across vtgate and vttablet #8931

Merged
merged 6 commits into from
Oct 19, 2021

Conversation

aquarapid
Copy link
Contributor

@aquarapid aquarapid commented Oct 5, 2021

This does not fix all instances of this problem, but it does improve the situation:

Before:

$ bin/vtgate -h 2>&1 | grep '  -' | wc -l
343
$ bin/vttablet -h 2>&1 | grep '  -' | wc -l
491

After:

$ bin/vtgate -h 2>&1 | grep '  -' | wc -l
196
$ bin/vttablet -h 2>&1 | grep '  -' | wc -l
484

The vttablet situation is still not great, but the remaining vttablet flags that are really vtgate-only are not easy to remove, e.g. the ones from go/mysql, like:

  -mysql_auth_server_static_file string
  -mysql_auth_server_static_string string
  -mysql_auth_static_reload_interval duration
  -mysql_clientcert_auth_method string

which is not easy to break into sub-packages without import cycles; and will need greater surgery.

in all the vttablet CLI flags into vtgate.

Signed-off-by: Jacques Grove <aquarapid@gmail.com>
CLI flags with vtgate vault flags

Signed-off-by: Jacques Grove <aquarapid@gmail.com>
Signed-off-by: Jacques Grove <aquarapid@gmail.com>
Copy link
Contributor

@ajm188 ajm188 left a comment

Choose a reason for hiding this comment

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

I know this is still in draft, but in case it's helpful, I wonder if it would be better to move the flag definitions into subpackages rather than the other way around? For example, what I'm starting to do here: f556f7d

go/vt/vttablet/tabletserver/tabletserver.go Show resolved Hide resolved
Signed-off-by: Jacques Grove <aquarapid@gmail.com>
@aquarapid
Copy link
Contributor Author

I know this is still in draft, but in case it's helpful, I wonder if it would be better to move the flag definitions into subpackages rather than the other way around? For example, what I'm starting to do here: f556f7d

Yeah, that probably fair enough. But more surgery than I'm willing to commit to right now.

Signed-off-by: Jacques Grove <aquarapid@gmail.com>
@aquarapid aquarapid added the Component: General Changes throughout the code base label Oct 5, 2021
Signed-off-by: Jacques Grove <aquarapid@gmail.com>
@aquarapid
Copy link
Contributor Author

For release notes:

It has been possible for several Vitess versions to pass certain vttablet flags to vtgate and certain vtgate flags to vttablet.  This did not have any effect on the operation of vtgate or vttablet, but did confuse users when looking at the commandline help.  This change fixes some instances of this problem.  As a result, users might have vtgate and vttablet startup scripts that use flags that will no longer be accepted, and vtgate or vttablet startup will fail.  Please test your vtgate and vttablet startup with the flags you use before rolling this version out in production."

@deepthi deepthi merged commit 5fee4d0 into vitessio:main Oct 19, 2021
@deepthi deepthi deleted the jg_flags branch October 19, 2021 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants