Use logger in config service client as default logger for kv client #187
Conversation
client/etcd/client.go
Outdated
| @@ -311,7 +309,7 @@ func validateTopLevelNamespace(namespace string) error { | |||
| } | |||
|
|
|||
| func (c *csclient) sanitizeOptions(opts kv.Options) (kv.Options, error) { | |||
| if opts.Logger() == nil { | |||
| if opts.Logger() == nil || reflect.DeepEqual(opts.Logger(), log.NullLogger) { | |||
There was a problem hiding this comment.
We can probably just do opts.Logger() == log.NullLogger here?
There was a problem hiding this comment.
We can't, the NullLogger is not comparable, I guess because of the slice of fields in it.
There was a problem hiding this comment.
Sure, but log.NullLogger is a singleton and of type Logger, which is an interface, we should be able to compare interface pointers no?
There was a problem hiding this comment.
Are you saying we should compare the address of the struct under the log.Logger interface? but I don't see how can we we get that without using reflect or unsafe pointer? Please feel free to commandeer this pr if there is an easy way to do it.
Or we can define NullLogger as var NullLogger Logger = &nullLogger{} to cheat around it.
There was a problem hiding this comment.
What I meant was NullLogger is a singleton and of Logger type, which is an interface. Therefore under the hood, NullLogger contains a pointer to the slice fields in the null logger on the heap. As such, checking opts.Logger() == log.NullLogger should just work. FWIW I just tried it and it builds locally.
There was a problem hiding this comment.
Ah I see, didn't realize the NullLogger wasn't a pointer type by itself, which would cause a panic during runtime. Put up a m3x PR here to make NullLogger a pointer type: m3db/m3x#108 and updated this PR to use direct logger comparison without reflection.
| SetNamespace(kvPrefix). | ||
| SetEnvironment(c.opts.Env()) | ||
| c.txn, c.txnErr = c.createTxnStore(kvOpts) | ||
| c.txn, c.txnErr = c.TxnStore(kv.NewOptions()) |
There was a problem hiding this comment.
We don't need to set the namespace and environment here anymore?
There was a problem hiding this comment.
That's taken care of in sanitizeOptions.
There was a problem hiding this comment.
Oh, I see, Github had collapsed those lines in my view lol.
| SetNamespace(kvPrefix). | ||
| SetEnvironment(c.opts.Env()) | ||
| c.txn, c.txnErr = c.createTxnStore(kvOpts) | ||
| c.txn, c.txnErr = c.TxnStore(kv.NewOptions()) |
There was a problem hiding this comment.
Oh, I see, Github had collapsed those lines in my view lol.
Seems current logic means the kv client in m3db will be using nulllogger as well, we probably need to upgrade all repos with this change, seems the only place with a real logger with the previous version is the one in m3aggregator where we explicitly set a logger with Warn level.
@robskillington @xichen2020 @prateek @jeromefroe