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

*: new --log-prefix flag / option #850

Merged
merged 1 commit into from
Jan 28, 2017

Conversation

ploxiln
Copy link
Member

@ploxiln ploxiln commented Jan 24, 2017

instantiate Logger in nsqd.New() instead of nsqd.NewOptions(), only if not already present in opts

More options spam, I understand if it's not desired. But if you don't mind this, I'll do this for nsqlookupd and nsqadmin as well.

(why? it's redundant with the service / container name in my setup. not a big deal)

@ploxiln ploxiln changed the title nsqd: new --log-prefix flag / option new --log-prefix flag / option Jan 25, 2017
@mreiferson mreiferson changed the title new --log-prefix flag / option nsqd: new --log-prefix flag / option Jan 28, 2017
@mreiferson
Copy link
Member

OK, fine with me, but we should add it for all 3 binaries?

@ploxiln
Copy link
Member Author

ploxiln commented Jan 28, 2017

That's my suggestion, do it for all three - was just testing the waters with one easier-to-review piece first. I'll add changes for the other two shortly ...

@ploxiln
Copy link
Member Author

ploxiln commented Jan 28, 2017

Done for nsqd, nsqlookupd, nsqadmin.

I just realized, previously one would be able to set Logger to nil in options to disable logging, now that won't work, you'll have to create a no-op Logger. All these apps have logf() like:

func (n *NSQD) logf(f string, args ...interface{}) {
	if n.getOpts().Logger == nil {
		return
	}
	n.getOpts().Logger.Output(2, fmt.Sprintf(f, args...))
}

(On the upside, we can get rid of that conditional now...)

instantiate Logger in $APP.New() instead of $APP.NewOptions(),
only if not already present in opts

Now, setting Logger to nil in Options is not sufficient
to disable logging. A no-op logger must be created and assigned.
@ploxiln
Copy link
Member Author

ploxiln commented Jan 28, 2017

squashed in removal of the now (unfortunately) useless nil checks in logf()

@mreiferson mreiferson changed the title nsqd: new --log-prefix flag / option *: new --log-prefix flag / option Jan 28, 2017
@mreiferson mreiferson merged commit 1eed090 into nsqio:master Jan 28, 2017
@ploxiln ploxiln deleted the config_log_prefix branch April 17, 2017 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants