From 3495db7fe6ebe9de3efa872ae7a1f90f736b996d Mon Sep 17 00:00:00 2001 From: Ibrahim Jarif Date: Mon, 25 Feb 2019 12:56:15 +0530 Subject: [PATCH 1/3] Set glog flags from configuration This PR fixes https://github.com/dgraph-io/dgraph/issues/2854 Why glog can't use values set in config file? glog reads values from flags. When a user sets the value (of a glog flag) in a config file, glog doesn't recognize it because the flag value hasn't changed. How does this PR fix it? With this PR, we set the value of glog flags from the values in config file. Note: This is a hack. glog doesn't allow setting configuration programatically and thus we have to use this hack. Signed-off-by: Ibrahim Jarif --- dgraph/cmd/root.go | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/dgraph/cmd/root.go b/dgraph/cmd/root.go index 0f8f5a47869..983dfc862c2 100644 --- a/dgraph/cmd/root.go +++ b/dgraph/cmd/root.go @@ -17,7 +17,7 @@ package cmd import ( - goflag "flag" + "flag" "strings" "github.com/dgraph-io/dgraph/dgraph/cmd/alpha" @@ -31,7 +31,6 @@ import ( "github.com/dgraph-io/dgraph/dgraph/cmd/zero" "github.com/dgraph-io/dgraph/x" "github.com/spf13/cobra" - flag "github.com/spf13/pflag" "github.com/spf13/viper" ) @@ -55,9 +54,9 @@ cluster. func Execute() { initCmds() - // Convinces goflags that Parse() has been called to avoid noisy logs. + // Convinces glog that Parse() has been called to avoid noisy logs. // https://github.com/kubernetes/kubernetes/issues/17162#issuecomment-225596212 - x.Check(goflag.CommandLine.Parse([]string{})) + x.Check(flag.CommandLine.Parse([]string{})) // Dumping the usage in case of an error makes the error messages harder to see. RootCmd.SilenceUsage = true @@ -87,10 +86,12 @@ func initCmds() { "Allow trace endpoint to be accessible from remote") rootConf.BindPFlags(RootCmd.PersistentFlags()) - flag.CommandLine.AddGoFlagSet(goflag.CommandLine) + // Add all existing global flag (eg: from glog) to rootCmd's flags + RootCmd.PersistentFlags().AddGoFlagSet(flag.CommandLine) + // Always set stderrthreshold=0. Don't let users set it themselves. - x.Check(flag.Set("stderrthreshold", "0")) - x.Check(flag.CommandLine.MarkDeprecated("stderrthreshold", + x.Check(RootCmd.PersistentFlags().Set("stderrthreshold", "0")) + x.Check(RootCmd.PersistentFlags().MarkDeprecated("stderrthreshold", "Dgraph always sets this flag to 0. It can't be overwritten.")) for _, sc := range subcommands { @@ -112,6 +113,25 @@ func initCmds() { for _, sc := range subcommands { sc.Conf.SetConfigFile(cfg) x.Check(x.Wrapf(sc.Conf.ReadInConfig(), "reading config")) + setGlogFlags(sc.Conf) } }) } + +// setGlogFlags function sets the glog flags based on the configuration. +// We need to manually set the flags from configuration because glog reads +// values from flags, not configuration. +func setGlogFlags(conf *viper.Viper) { + // List of flags taken from + // https://github.com/golang/glog/blob/master/glog.go#L399 + // and https://github.com/golang/glog/blob/master/glog_file.go#L41 + glogFlags := [...]string{ + "log_dir", "logtostderr", "alsologtostderr", "v", "stderrthreshold", "vmodule", "log_backtrace_at", + } + for _, gflag := range glogFlags { + // Set value of flag to the value in config + if stringValue, ok := conf.Get(gflag).(string); ok { + flag.Lookup(gflag).Value.Set(stringValue) + } + } +} From e0f41436db9fd4fae87f0449754b5eb4eacc65f1 Mon Sep 17 00:00:00 2001 From: Ibrahim Jarif Date: Mon, 25 Feb 2019 13:08:27 +0530 Subject: [PATCH 2/3] Fix error check Signed-off-by: Ibrahim Jarif --- dgraph/cmd/root.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dgraph/cmd/root.go b/dgraph/cmd/root.go index 983dfc862c2..1b554d83b17 100644 --- a/dgraph/cmd/root.go +++ b/dgraph/cmd/root.go @@ -131,7 +131,7 @@ func setGlogFlags(conf *viper.Viper) { for _, gflag := range glogFlags { // Set value of flag to the value in config if stringValue, ok := conf.Get(gflag).(string); ok { - flag.Lookup(gflag).Value.Set(stringValue) + x.Check(flag.Lookup(gflag).Value.Set(stringValue)) } } } From 80203622d6ce78e2c2ec9f06f5387ca375415098 Mon Sep 17 00:00:00 2001 From: Ibrahim Jarif Date: Mon, 25 Feb 2019 13:08:57 +0530 Subject: [PATCH 3/3] Fix line length Signed-off-by: Ibrahim Jarif --- dgraph/cmd/root.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dgraph/cmd/root.go b/dgraph/cmd/root.go index 1b554d83b17..453c81cb9c9 100644 --- a/dgraph/cmd/root.go +++ b/dgraph/cmd/root.go @@ -126,7 +126,8 @@ func setGlogFlags(conf *viper.Viper) { // https://github.com/golang/glog/blob/master/glog.go#L399 // and https://github.com/golang/glog/blob/master/glog_file.go#L41 glogFlags := [...]string{ - "log_dir", "logtostderr", "alsologtostderr", "v", "stderrthreshold", "vmodule", "log_backtrace_at", + "log_dir", "logtostderr", "alsologtostderr", "v", + "stderrthreshold", "vmodule", "log_backtrace_at", } for _, gflag := range glogFlags { // Set value of flag to the value in config