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

address comments #1

Closed
wants to merge 1 commit into from
Closed

address comments #1

wants to merge 1 commit into from

Conversation

winkyao
Copy link

@winkyao winkyao commented Apr 2, 2019

Address comments

@kolbe
Copy link
Owner

kolbe commented Apr 2, 2019

@winkyao it seems like you're mis-using the configCheck (--config-check) flag. That flag is meant to cause the server to exit if config check is successful. Morgan and others agreed that another flag, --config-strict, be added, and if it is true the server should exit if the config check fails. So, --config-strict/configStrict should be checked in loadConfig().

Another issue is that importing "github.com/prometheus/common/log" as you've done here and emitting a log message to it bypasses the log facility that is used by the rest of tidb-server (which has not yet been set up at the time the config file is read). That means (I think) that this message will always be written to stderr, and never to the log file:

$ ./bin/tidb-server --config=../config.toml --config-strict=true --log-file=tidb.log
WARN[0000] config file ../config.toml contained unknown configuration options:
  status-host
  nonsense   source="config.go:393"

This doesn't seem very ideal, since it means tidb-server is emitting output to stderr when it probably shouldn't be, and it's not going to put this message in the error log, which is where an administrator will expect to look for important warnings.

The only solution I can figure is to have Config.Load return an error whether or not strict checking is enabled, and then to decide in tidb-server/main.go whether the error returned should cause the server to exit based on the value of the configStrict flag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants