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

mon: clean up in ceph_mon.cc #14102

Merged
merged 1 commit into from Jun 28, 2017

Conversation

Projects
None yet
4 participants
@renhwztetecs
Member

renhwztetecs commented Mar 23, 2017

The output prints to the ceph-mon.log,
so need replace cout with dout

Signed-off-by: huanwen ren ren.huanwen@zte.com.cn

@joscollin

Could you please remove the commented line 402, along with this fix ?

@renhwztetecs

This comment has been minimized.

Member

renhwztetecs commented Mar 23, 2017

@joscollin
good idea! I updated it
thank you

@joscollin

Looks good. Approved.

@renhwztetecs

This comment has been minimized.

Member

renhwztetecs commented Mar 23, 2017

jenkins test this please

@jecluis

given you are changing cout with dout, it would also make sense to change cerr to derr.

And both commits should be squashed.

@renhwztetecs

This comment has been minimized.

Member

renhwztetecs commented Mar 24, 2017

@jecluis
updated it
thank you

@joscollin

Please correct the mistakes mentioned. Thanks.

cerr << " extract the monmap from the local monitor store and exit\n";
cerr << " --mon-data <directory>\n";
cerr << " where the mon store and keyring are located\n";
cout << "usage: ceph-mon -i monid [flags]" << std::endl;

This comment has been minimized.

@joscollin

joscollin Mar 24, 2017

Member

@renhwztetecs @jecluis As I understand from jecluis' comment, this should be derr.

This comment has been minimized.

@renhwztetecs

renhwztetecs Mar 25, 2017

Member

For the ceph-mon command parameter hint, using "cout" is better than using the "derr", I think. because it benefits the debug
@jecluis @joscollin what do you think

This comment has been minimized.

@joscollin

joscollin Mar 25, 2017

Member

@renhwztetecs
I will wait for @jecluis input on this, as this change is originally suggested by him.

This comment has been minimized.

@jecluis

jecluis Mar 25, 2017

Member

I'm sorry if I wasn't clear. Given we are changing from cout to dout in most of this file, I thought it was warranted that we'd mimic that for cerr to derr. But that only makes sense on the portion of code that is potentially useful to have written to the log.

For instance, while it makes sense to output the error condition of check_mon_data_exists() (below, c. line 301), it does not make sense, in my opinion, to have argument validation errors outputted to the log. The same goes for usage. An additional argument why we should not use derr (or dout) in those cases, would be that we do not want additional noise on the CLI when the user runs ceph-mon on the CLI - dout and derr will output a lot of information that is only useful in the context of a log, and will be nothing but cumbersome (and possibly unexpected) when the user runs ceph-mon --help, for instance.

Does this make sense to you both?

This comment has been minimized.

@jecluis

jecluis Mar 25, 2017

Member

To some extent, one could argue that we should not output to dout or derr before we prefork. However, taking such a broad criteria would potentially ignore some useful messages to have in the log.

E.g., having the messages during mkfs being outputted to cerr and cout would be nice, as they'd be cleaner when presented to the user if called via the CLI; on the other hand, we don't get them written to the log if ceph-mon, and if the monitor is not invoked with -d, then we would really like to have them written somewhere. Granted, the monitor will only fork later on, but the caller may be ignoring any output from the daemon.

The more I think about it, the more divided I am on what's the best course of action. Any thoughts?

This comment has been minimized.

@renhwztetecs

renhwztetecs Mar 27, 2017

Member

strongly agree with

usage();
}
if (g_conf->name.get_id().empty()) {
cerr << "must specify id (--id <id> or --name mon.<id>)" << std::endl;
derr << "must specify id (--id <id> or --name mon.<id>)" << dendl;

This comment has been minimized.

@jecluis

jecluis Mar 25, 2017

Member

... and this.

This comment has been minimized.

@renhwztetecs

renhwztetecs Mar 27, 2017

Member

update it

exit(1);
}
if (g_conf->mon_data.empty()) {
cerr << "must specify '--mon-data=foo' data path" << std::endl;
derr << "must specify '--mon-data=foo' data path" << dendl;

This comment has been minimized.

@jecluis

jecluis Mar 25, 2017

Member

... and this.

This comment has been minimized.

@renhwztetecs

renhwztetecs Mar 27, 2017

Member

update it

usage();
}
if (force_sync && !yes_really) {
cerr << "are you SURE you want to force a sync? this will erase local data and may\n"
<< "break your mon cluster. pass --yes-i-really-mean-it if you do." << std::endl;
derr << "are you SURE you want to force a sync? this will erase local data and may\n"

This comment has been minimized.

@jecluis

jecluis Mar 25, 2017

Member

... and so should this.

This comment has been minimized.

@renhwztetecs

renhwztetecs Mar 27, 2017

Member

update it

@@ -272,23 +272,23 @@ int main(int argc, const char **argv)
}
}
if (!args.empty()) {
cerr << "too many arguments: " << args << std::endl;
derr << "too many arguments: " << args << dendl;

This comment has been minimized.

@jecluis

jecluis Mar 25, 2017

Member

E.g., this should be kept as cerr.

This comment has been minimized.

@renhwztetecs

renhwztetecs Mar 27, 2017

Member

update it

cerr << " extract the monmap from the local monitor store and exit\n";
cerr << " --mon-data <directory>\n";
cerr << " where the mon store and keyring are located\n";
cout << "usage: ceph-mon -i monid [flags]" << std::endl;

This comment has been minimized.

@jecluis

jecluis Mar 25, 2017

Member

To some extent, one could argue that we should not output to dout or derr before we prefork. However, taking such a broad criteria would potentially ignore some useful messages to have in the log.

E.g., having the messages during mkfs being outputted to cerr and cout would be nice, as they'd be cleaner when presented to the user if called via the CLI; on the other hand, we don't get them written to the log if ceph-mon, and if the monitor is not invoked with -d, then we would really like to have them written somewhere. Granted, the monitor will only fork later on, but the caller may be ignoring any output from the daemon.

The more I think about it, the more divided I am on what's the best course of action. Any thoughts?

@@ -298,25 +298,25 @@ int main(int argc, const char **argv)
int err = check_mon_data_exists();
if (err == -ENOENT) {
if (::mkdir(g_conf->mon_data.c_str(), 0755)) {
cerr << "mkdir(" << g_conf->mon_data << ") : "
<< cpp_strerror(errno) << std::endl;
derr << "mkdir(" << g_conf->mon_data << ") : "

This comment has been minimized.

@jecluis

jecluis Mar 25, 2017

Member

This would be nice to have, but I wonder whether cerr is actually more appropriate.

This comment has been minimized.

@renhwztetecs

renhwztetecs Mar 27, 2017

Member

Reserved here, other are changed

@renhwztetecs

This comment has been minimized.

Member

renhwztetecs commented Mar 27, 2017

@jecluis @joscollin
updated it
thanks

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Apr 10, 2017

@renhwztetecs needs rebase.
@jecluis ping?

@renhwztetecs

This comment has been minimized.

Member

renhwztetecs commented Apr 11, 2017

@tchaikov @jecluis
rebased it
thanks

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented May 16, 2017

@jecluis ping?

@tchaikov tchaikov requested a review from jecluis May 16, 2017

@renhwztetecs

This comment has been minimized.

Member

renhwztetecs commented May 18, 2017

@jecluis please reviews again
thank you

@renhwztetecs

This comment has been minimized.

Member

renhwztetecs commented Jun 17, 2017

jenkins test this please

@jecluis

Testing this manually.

@jecluis

This comment has been minimized.

Member

jecluis commented Jun 27, 2017

@renhwztetecs needs rebase

@renhwztetecs

This comment has been minimized.

Member

renhwztetecs commented Jun 27, 2017

retest this please

mon: clean up in ceph_mon.cc
The output prints to the ceph-mon.log,
so need replace cout with dout,in addition, keep cout in the usage().

Signed-off-by: huanwen ren <ren.huanwen@zte.com.cn>
@renhwztetecs

This comment has been minimized.

Member

renhwztetecs commented Jun 27, 2017

@jecluis #15723 merge part code
I drop the repetition, please review again

@jecluis

This comment has been minimized.

Member

jecluis commented Jun 28, 2017

lgtm

@jecluis jecluis merged commit 40f023c into ceph:master Jun 28, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
default Build finished.
Details
make check make check succeeded
Details

@renhwztetecs renhwztetecs deleted the renhwztetecs:renhw-wip-moninit-cleanup branch Jun 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment