You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dinesh Bhat (Code Review)" <ge...@cloudera.org> on 2016/09/19 17:52:00 UTC
[kudu-CR] [tools]: Keep the verbosity of CLI at WARNING and above
Hello David Ribeiro Alves, Adar Dembo, Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/4447
to review the following change.
Change subject: [tools]: Keep the verbosity of CLI at WARNING and above
......................................................................
[tools]: Keep the verbosity of CLI at WARNING and above
This keeps the verbosity of the 'kudu' commands at WARNING
and above if the user had not specified 'minloglevel' or
'--v' on the command line options. Both stdout and stderr
will suppress INFO level output without having to explicitly
append '2>/dev/null' to cli commands.
Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
---
M src/kudu/tools/tool_main.cc
1 file changed, 9 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/4447/1
--
To view, visit http://gerrit.cloudera.org:8080/4447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
[kudu-CR] [tools]: Keep the verbosity of CLI at FATAL and above
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [tools]: Keep the verbosity of CLI at FATAL and above
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4447/1/src/kudu/tools/tool_main.cc
File src/kudu/tools/tool_main.cc:
PS1, Line 215: // Set the verbosity of the commands to WARNING(2) and above.
: // If the user had explicitly specified verbosity, then user's
: // verbosity level is honored. Since '--v' depends on minloglevel
: // specifying either of them on CLI will override this setting.
: if (google::GetCommandLineFlagInfoOrDie("minloglevel").is_default &&
: google::GetCommandLineFlagInfoOrDie("v").is_default) {
: google::SetCommandLineOption("minloglevel",
: SimpleItoa(google::GLOG_WARNING).c_str());
: }
> hrm, I disagree with the idea to not log WARNING/ERROR... isn't the whole p
It's difficult to corral INFO (and even WARNING) messages; they can come from all over the codebase, and ones that aren't useful in a CLI context may very well be useful when you're logging to a file.
My argument against is:
1. It's unusual to see this kind of logging (with timestamps, pid, line numbers, etc.) in a UNIX command-line tool, at least not by default
2. The returned Status should describe what went wrong at a high enough level that a user can understand what to do next (i.e. retry, give up, remediate this other thing first, etc.).
3. Most of the commands are read-only or idempotent, so it's easy to retry with --v or --minloglevel 0 to yield more descriptive errors. I'd liken this to my workflow with other UNIX tools like git: if I encounter an error and I want more information, I rerun the same command with debug logging enabled.
--
To view, visit http://gerrit.cloudera.org:8080/4447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [tools]: Keep the verbosity of CLI at FATAL and above
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [tools]: Keep the verbosity of CLI at FATAL and above
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4447/1/src/kudu/tools/tool_main.cc
File src/kudu/tools/tool_main.cc:
PS1, Line 215: // Set the verbosity of the commands to WARNING(2) and above.
: // If the user had explicitly specified verbosity, then user's
: // verbosity level is honored. Since '--v' depends on minloglevel
: // specifying either of them on CLI will override this setting.
: if (google::GetCommandLineFlagInfoOrDie("minloglevel").is_default &&
: google::GetCommandLineFlagInfoOrDie("v").is_default) {
: google::SetCommandLineOption("minloglevel",
: SimpleItoa(google::GLOG_WARNING).c_str());
: }
> That's correct. --v controls VLOG(LOG at lowest level, which is INFO), and
Yes, I think I can make peace with WARNING or higher. Maybe even INFO. Todd is right that most uses of the tool are debug-related, and so more information is generally better.
--
To view, visit http://gerrit.cloudera.org:8080/4447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [tools]: Keep the verbosity of CLI at FATAL and above
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.
Change subject: [tools]: Keep the verbosity of CLI at FATAL and above
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4447/1/src/kudu/tools/tool_main.cc
File src/kudu/tools/tool_main.cc:
PS1, Line 215: FLAGS_minloglevel = google::GLOG_FATAL;
: }
: return show_help;
: }
:
: int main(int argc, char** argv) {
: bool show_help = ParseCommandLineFlags(&argc, &argv);
: FLAGS_logtostderr = true;
: k
> I agree it's a little unusual looking, but given that many of the usages of
One of the motivations of being less verbose here was to keep the output more machine-readable(I should update that in commit-message). Also binaries retrying on those RPC errors are difficult to apprehend for folks other than developers. Given that this tool may be used by support folks as well, we were inclining to go with default=quiet as opposed to default=verbose mode.
--
To view, visit http://gerrit.cloudera.org:8080/4447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [tools]: Keep the verbosity of CLI at WARNING and above
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Hello David Ribeiro Alves, Adar Dembo, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4447
to look at the new patch set (#2).
Change subject: [tools]: Keep the verbosity of CLI at WARNING and above
......................................................................
[tools]: Keep the verbosity of CLI at WARNING and above
This keeps the verbosity of the 'kudu' commands at WARNING
and above if the user had not specified 'minloglevel' or
'--v' on the command line options. Both stdout and stderr
will suppress INFO level output without having to explicitly
append '2>/dev/null' to cli commands.
Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
---
M src/kudu/tools/tool_main.cc
1 file changed, 9 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/4447/2
--
To view, visit http://gerrit.cloudera.org:8080/4447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [tools]: Keep the verbosity of CLI at WARNING and above
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [tools]: Keep the verbosity of CLI at WARNING and above
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4447/1/src/kudu/tools/tool_main.cc
File src/kudu/tools/tool_main.cc:
PS1, Line 215: // Set the verbosity of the commands to WARNING(2) and above.
: // If the user had explicitly specified verbosity, then user's
: // verbosity level is honored. Since '--v' depends on minloglevel
: // specifying either of them on CLI will override this setting.
: if (google::GetCommandLineFlagInfoOrDie("minloglevel").is_default &&
: google::GetCommandLineFlagInfoOrDie("v").is_default) {
: google::SetCommandLineOption("minloglevel",
: SimpleItoa(google::GLOG_WARNING).c_str());
: }
Couple of comments:
1. Can you move this into ParseCommandLineFlags, since it's doing some parsing of its own?
2. I still think that the default logging behavior should be to log absolutely nothing. Maybe FATAL or higher so that _crashes_ are logged, but that's about it. This is because during regular operations (i.e. outside of crashes), every action returns a detailed Status which gets logged already. That Status should be enough to diagnose simple errors. If not, the user can rerun the action with --v or --minloglevel set to something else.
3. Ideally we'd show --v and --minloglevel on every action's help, but adding it to every action manually is a pain in the butt. This is where "inheriting" parameters from modes in the mode chain would be a nice feature to have. You don't need to implement that, though; I'm just thinking out loud.
--
To view, visit http://gerrit.cloudera.org:8080/4447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [tools]: Keep the verbosity of CLI at FATAL and above
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has abandoned this change.
Change subject: [tools]: Keep the verbosity of CLI at FATAL and above
......................................................................
Abandoned
Since we decided to keep the INFO level logs with these tools, this patch is not relevant anymore. Idea is to be as much informative in the output as possible when using these tools.
--
To view, visit http://gerrit.cloudera.org:8080/4447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [tools]: Keep the verbosity of CLI at FATAL and above
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [tools]: Keep the verbosity of CLI at FATAL and above
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4447/1/src/kudu/tools/tool_main.cc
File src/kudu/tools/tool_main.cc:
PS1, Line 215: // Set the verbosity of the commands to WARNING(2) and above.
: // If the user had explicitly specified verbosity, then user's
: // verbosity level is honored. Since '--v' depends on minloglevel
: // specifying either of them on CLI will override this setting.
: if (google::GetCommandLineFlagInfoOrDie("minloglevel").is_default &&
: google::GetCommandLineFlagInfoOrDie("v").is_default) {
: google::SetCommandLineOption("minloglevel",
: SimpleItoa(google::GLOG_WARNING).c_str());
: }
> @dinesh: thank you for the feedback. Yes, essentially this is what we can
Also, --vmodule affects only the verbose logging stuff, like VLOG(), right (which will be logged only for INFO and lower level of main log level)? It does not affect the main logging level like used in the LOG() directive.
--
To view, visit http://gerrit.cloudera.org:8080/4447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [tools]: Keep the verbosity of CLI at FATAL and above
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [tools]: Keep the verbosity of CLI at FATAL and above
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4447/1/src/kudu/tools/tool_main.cc
File src/kudu/tools/tool_main.cc:
PS1, Line 215: // Set the verbosity of the commands to WARNING(2) and above.
: // If the user had explicitly specified verbosity, then user's
: // verbosity level is honored. Since '--v' depends on minloglevel
: // specifying either of them on CLI will override this setting.
: if (google::GetCommandLineFlagInfoOrDie("minloglevel").is_default &&
: google::GetCommandLineFlagInfoOrDie("v").is_default) {
: google::SetCommandLineOption("minloglevel",
: SimpleItoa(google::GLOG_WARNING).c_str());
: }
> No, we wouldn't have to show _all_ gflags help. Imagine that the root mode
hrm, I disagree with the idea to not log WARNING/ERROR... isn't the whole point of those log levels that they are things that should be seen by the user? I can understand not logging INFO... but even then, why not just make sure that we aren't unnecessarily verbose in INFO messages?
--
To view, visit http://gerrit.cloudera.org:8080/4447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [tools]: Keep the verbosity of CLI at FATAL and above
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Hello David Ribeiro Alves, Adar Dembo, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4447
to look at the new patch set (#4).
Change subject: [tools]: Keep the verbosity of CLI at FATAL and above
......................................................................
[tools]: Keep the verbosity of CLI at FATAL and above
This keeps the verbosity of the 'kudu' commands at FATAL
if the user had not specified '--minloglevel' or '--v' on the
command line options. Both stdout and stderr will suppress
INFO level output without having to explicitly append '2>/dev/null'
to cli commands.
Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
---
M src/kudu/tools/tool_main.cc
1 file changed, 8 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/4447/4
--
To view, visit http://gerrit.cloudera.org:8080/4447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [tools]: Keep the verbosity of CLI at FATAL and above
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.
Change subject: [tools]: Keep the verbosity of CLI at FATAL and above
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4447/1/src/kudu/tools/tool_main.cc
File src/kudu/tools/tool_main.cc:
PS1, Line 215: FLAGS_minloglevel = google::GLOG_FATAL;
: }
: return show_help;
: }
:
: int main(int argc, char** argv) {
: bool show_help = ParseCommandLineFlags(&argc, &argv);
: FLAGS_logtostderr = true;
: k
> Also, --vmodule affects only the verbose logging stuff, like VLOG(), right
That's correct. --v controls VLOG(LOG at lowest level, which is INFO), and minloglevel controls LOG levels and typically you would want use one of these. My understanding is that, consumers of these additional gflags are either ops/support or developers. So for these audience, keeping the knobs at more granularity(file level) might help in the long run than keeping the loglevel knobs at library. My initial intention of this change was to get rid of INFO level messages on CLI like "Opened FS with uuid: xxxx", and I am glad it led to some interesting discussions on this topic.
Folks, are we concluding that we should keep the loglevels for CLI at WARNINGS and above then ? If the user explicitly specifies a loglevel via --minloglevel or --v, then we can honor that. Also, all this impacts to 'kudu' toolset alone. We also seem to be converging that we don't want to be under a contract about machine-readability of the 'kudu' output to keep the CLI more flexible.
--
To view, visit http://gerrit.cloudera.org:8080/4447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [tools]: Keep the verbosity of CLI at WARNING and above
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.
Change subject: [tools]: Keep the verbosity of CLI at WARNING and above
......................................................................
Patch Set 1:
(1 comment)
TFTR Adar, please see below.
http://gerrit.cloudera.org:8080/#/c/4447/1/src/kudu/tools/tool_main.cc
File src/kudu/tools/tool_main.cc:
PS1, Line 215: // Set the verbosity of the commands to WARNING(2) and above.
: // If the user had explicitly specified verbosity, then user's
: // verbosity level is honored. Since '--v' depends on minloglevel
: // specifying either of them on CLI will override this setting.
: if (google::GetCommandLineFlagInfoOrDie("minloglevel").is_default &&
: google::GetCommandLineFlagInfoOrDie("v").is_default) {
: google::SetCommandLineOption("minloglevel",
: SimpleItoa(google::GLOG_WARNING).c_str());
: }
> Couple of comments:
1. Done.
2. I was wondering if we want to keep the errors visible - for eg, if we run cluster ksck on a cluster which is partitioned, some RPC failures indicate(although not user-friendly output) that those hosts are not reachable. But this makes the output slightly less machine-readable. So keeping the level to FATAL for now, and let's decipher the status from the output itself rather than from error/warning logs.
3. When you say 'parameters', I am thinking about optional mode params(which could be gflags too in our case). But inheriting them means exposing all the ugly output of gflags help again, isn't it ?
--
To view, visit http://gerrit.cloudera.org:8080/4447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [tools]: Keep the verbosity of CLI at FATAL and above
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Hello David Ribeiro Alves, Adar Dembo, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4447
to look at the new patch set (#3).
Change subject: [tools]: Keep the verbosity of CLI at FATAL and above
......................................................................
[tools]: Keep the verbosity of CLI at FATAL and above
This keeps the verbosity of the 'kudu' commands at FATAL
if the user had not specified '--minloglevel' or '--v' on the
command line options. Both stdout and stderr will suppress
INFO level output without having to explicitly append '2>/dev/null'
to cli commands.
Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
---
M src/kudu/tools/tool_main.cc
1 file changed, 9 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/4447/3
--
To view, visit http://gerrit.cloudera.org:8080/4447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [tools]: Keep the verbosity of CLI at FATAL and above
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.
Change subject: [tools]: Keep the verbosity of CLI at FATAL and above
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4447/1/src/kudu/tools/tool_main.cc
File src/kudu/tools/tool_main.cc:
PS1, Line 215: FLAGS_minloglevel = google::GLOG_FATAL;
: }
: return show_help;
: }
:
: int main(int argc, char** argv) {
: bool show_help = ParseCommandLineFlags(&argc, &argv);
: FLAGS_logtostderr = true;
: k
> What it we could set up different level of logging for different components
@alexey: currently one of the gflags '--vmodule' lets us achieve what you are describing above. http://rpg.ifi.uzh.ch/docs/glog.html#verbose
--
To view, visit http://gerrit.cloudera.org:8080/4447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [tools]: Keep the verbosity of CLI at FATAL and above
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [tools]: Keep the verbosity of CLI at FATAL and above
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4447/1/src/kudu/tools/tool_main.cc
File src/kudu/tools/tool_main.cc:
PS1, Line 215: // Set the verbosity of the commands to WARNING(2) and above.
: // If the user had explicitly specified verbosity, then user's
: // verbosity level is honored. Since '--v' depends on minloglevel
: // specifying either of them on CLI will override this setting.
: if (google::GetCommandLineFlagInfoOrDie("minloglevel").is_default &&
: google::GetCommandLineFlagInfoOrDie("v").is_default) {
: google::SetCommandLineOption("minloglevel",
: SimpleItoa(google::GLOG_WARNING).c_str());
: }
> One of the motivations of being less verbose here was to keep the output mo
I'm arguing for more verbosity in anything that is "unexpected" or "abnormal" which is definitely the case for anything WARN or ERROR. For 'INFO' level, I'm also saying that if anything is 100% expected, we probably shouldn't bother with it.
As for machine-parseable output, I have a couple thoughts:
1) any machine consuming the output should be parsing stdout, not stderr (where the logs go)
2) we should be very careful about signing up for any compatibility guarantee on the textual output format of these tools. I'd say in fact we should explicitly document that the output of these tools is _not_ parseable. Otherwise we'll be unable to make improvements over time to make things more human-readable. If machine-parseable output is a goal, we should provide --json options as appropriate.
--
To view, visit http://gerrit.cloudera.org:8080/4447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [tools]: Keep the verbosity of CLI at FATAL and above
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [tools]: Keep the verbosity of CLI at FATAL and above
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4447/1/src/kudu/tools/tool_main.cc
File src/kudu/tools/tool_main.cc:
PS1, Line 215: // Set the verbosity of the commands to WARNING(2) and above.
: // If the user had explicitly specified verbosity, then user's
: // verbosity level is honored. Since '--v' depends on minloglevel
: // specifying either of them on CLI will override this setting.
: if (google::GetCommandLineFlagInfoOrDie("minloglevel").is_default &&
: google::GetCommandLineFlagInfoOrDie("v").is_default) {
: google::SetCommandLineOption("minloglevel",
: SimpleItoa(google::GLOG_WARNING).c_str());
: }
> "I'm arguing for more verbosity in anything that is "unexpected" or "abnorm
What it we could set up different level of logging for different components? I.e., imagine we could set INFO for CLI tools, WARNING for client library, and ERROR for util and RPC?
Would it help?
In previous project I worked on that was possible (actually, that was me who brought that functionality). It was also possible to change it dynamically via REST interface (for server-side components). I think we can have something similar for Kudu. That proved to be very useful while tracing some issues (e.g., set it to DEBUG level just for one sub-component and leave ERROR for the rest).
What do you think?
--
To view, visit http://gerrit.cloudera.org:8080/4447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [tools]: Keep the verbosity of CLI at FATAL and above
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.
Change subject: [tools]: Keep the verbosity of CLI at FATAL and above
......................................................................
Patch Set 3:
(1 comment)
Thanks, updated.
http://gerrit.cloudera.org:8080/#/c/4447/3/src/kudu/tools/tool_main.cc
File src/kudu/tools/tool_main.cc:
PS3, Line 215: google::SetCommandLineOption("minloglevel",
: SimpleItoa(google::GLOG_FATAL).c_str());
> Can this just be:
Done, declaration is already available under logging.h
--
To view, visit http://gerrit.cloudera.org:8080/4447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [tools]: Keep the verbosity of CLI at FATAL and above
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [tools]: Keep the verbosity of CLI at FATAL and above
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4447/1/src/kudu/tools/tool_main.cc
File src/kudu/tools/tool_main.cc:
PS1, Line 215: // Set the verbosity of the commands to WARNING(2) and above.
: // If the user had explicitly specified verbosity, then user's
: // verbosity level is honored. Since '--v' depends on minloglevel
: // specifying either of them on CLI will override this setting.
: if (google::GetCommandLineFlagInfoOrDie("minloglevel").is_default &&
: google::GetCommandLineFlagInfoOrDie("v").is_default) {
: google::SetCommandLineOption("minloglevel",
: SimpleItoa(google::GLOG_WARNING).c_str());
: }
> It's difficult to corral INFO (and even WARNING) messages; they can come fr
I agree it's a little unusual looking, but given that many of the usages of the tool are for debugging, etc, I think starting with verbosity (and perhaps offering a standard -quiet) is better than the other way around.
As an example, if you try to run 'list_tables' against a master which is down right now, your commend just hangs as it generates retries. I would actually prefer it to be _more_ verbose here, saying that it's trying to contact a server and getting Connection Refused. I'd rather have the verbosity there by default and if people want to silence it they can pipe or --quiet
--
To view, visit http://gerrit.cloudera.org:8080/4447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [tools]: Keep the verbosity of CLI at FATAL and above
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [tools]: Keep the verbosity of CLI at FATAL and above
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4447/1/src/kudu/tools/tool_main.cc
File src/kudu/tools/tool_main.cc:
PS1, Line 215: // Set the verbosity of the commands to WARNING(2) and above.
: // If the user had explicitly specified verbosity, then user's
: // verbosity level is honored. Since '--v' depends on minloglevel
: // specifying either of them on CLI will override this setting.
: if (google::GetCommandLineFlagInfoOrDie("minloglevel").is_default &&
: google::GetCommandLineFlagInfoOrDie("v").is_default) {
: google::SetCommandLineOption("minloglevel",
: SimpleItoa(google::GLOG_WARNING).c_str());
: }
> I'm arguing for more verbosity in anything that is "unexpected" or "abnorma
"I'm arguing for more verbosity in anything that is "unexpected" or "abnormal" which is definitely the case for anything WARN or ERROR. For 'INFO' level, I'm also saying that if anything is 100% expected, we probably shouldn't bother with it."
Thanks for clarifying, but is this your take on the CLI's verbosity? Or on general glog verbosity in Kudu?
I'm asking because we don't uphold your INFO-level standard in Kudu in general, and I'm not sure if we should: "expected" stuff in the logs still serve as useful breadcrumbs, and the timestamps are useful for diagnosing unexpected slowdowns or races. For example, opening an FsManager logs the contents of the instance PB file at the INFO level. It's completely expected, but the logged UUID is useful for matching against peer UUIDs. I'm using this as an example because many CLI commands open FsManagers and log this same thing which isn't particularly useful in the CLI context.
So going back to my earlier point, I don't see how we can include INFO-level logs in the CLI without either logging too much, or removing too much stuff that's otherwise useful in server glogs. I'll concede that WARNING-level may be reasonable, though there's something to be said for the purity of never logging to stderr in normal operation.
Regarding machine-readability, I agree with your points that it's orthogonal to stderr logging and that we shouldn't guarantee anything. I'm willing to erode the second point for some simple actions (i.e. I don't mind signing on a dotted line that "kudu fs dump uuid" prints nothing but a uuid to stdout), but I don't feel strongly.
--
To view, visit http://gerrit.cloudera.org:8080/4447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [tools]: Keep the verbosity of CLI at FATAL and above
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [tools]: Keep the verbosity of CLI at FATAL and above
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4447/1/src/kudu/tools/tool_main.cc
File src/kudu/tools/tool_main.cc:
PS1, Line 215: // Set the verbosity of the commands to WARNING(2) and above.
: // If the user had explicitly specified verbosity, then user's
: // verbosity level is honored. Since '--v' depends on minloglevel
: // specifying either of them on CLI will override this setting.
: if (google::GetCommandLineFlagInfoOrDie("minloglevel").is_default &&
: google::GetCommandLineFlagInfoOrDie("v").is_default) {
: google::SetCommandLineOption("minloglevel",
: SimpleItoa(google::GLOG_WARNING).c_str());
: }
> I agree it's a little unusual looking, but given that many of the usages of
What you're suggesting is useful, but I don't know how we implement it given the constraints at hand: the CLI shares nearly all of its code with the rest of Kudu, and log calls have statically defined levels. Suppose we wanted more verbosity in the tool and less in glog files. Would we run the CLI with --v=1 and audit the VLOG(1) calls to make sure they're useful?
BTW, are you advocating for more CLI verbosity or less? In your first comment you suggested we audit INFO messages and remove the excessively verbose ones, while in the second comment you said you'd prefer more verbosity in the CLI.
--
To view, visit http://gerrit.cloudera.org:8080/4447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [tools]: Keep the verbosity of CLI at FATAL and above
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [tools]: Keep the verbosity of CLI at FATAL and above
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/4447/1/src/kudu/tools/tool_main.cc
File src/kudu/tools/tool_main.cc:
PS1, Line 215: google::SetCommandLineOption("minloglevel",
: SimpleItoa(google::GLOG_FATAL).c_str());
: }
: return show_help;
: }
:
: int main(int argc, char** argv) {
: bool show_help = ParseCommandLineFlags(&argc, &argv);
: F
> 1. Done.
No, we wouldn't have to show _all_ gflags help. Imagine that the root mode had 'v' and 'minloglevel' defined as parameters. Then, every mode and action below the root would expose 'v' and 'minloglevel' in its help (probably with a little message explaining that it's inherited from a mode and where it came from). That's about it.
http://gerrit.cloudera.org:8080/#/c/4447/3/src/kudu/tools/tool_main.cc
File src/kudu/tools/tool_main.cc:
PS3, Line 215: google::SetCommandLineOption("minloglevel",
: SimpleItoa(google::GLOG_FATAL).c_str());
Can this just be:
FLAGS_minloglevel = google::GLOG_FATAL;
Provided you DECLARE_int32(minloglevel) at the top of the file?
--
To view, visit http://gerrit.cloudera.org:8080/4447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [tools]: Keep the verbosity of CLI at FATAL and above
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [tools]: Keep the verbosity of CLI at FATAL and above
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4447/1/src/kudu/tools/tool_main.cc
File src/kudu/tools/tool_main.cc:
PS1, Line 215: // Set the verbosity of the commands to WARNING(2) and above.
: // If the user had explicitly specified verbosity, then user's
: // verbosity level is honored. Since '--v' depends on minloglevel
: // specifying either of them on CLI will override this setting.
: if (google::GetCommandLineFlagInfoOrDie("minloglevel").is_default &&
: google::GetCommandLineFlagInfoOrDie("v").is_default) {
: google::SetCommandLineOption("minloglevel",
: SimpleItoa(google::GLOG_WARNING).c_str());
: }
> @alexey: currently one of the gflags '--vmodule' lets us achieve what you a
@dinesh: thank you for the feedback. Yes, essentially this is what we can use, but the module for that --vmodule flag is different from what I described: those are patterns for file names, which is cumbersome for a big codebase like Kudu's. And it does not allow to dynamically control logging level for an already running server.
--
To view, visit http://gerrit.cloudera.org:8080/4447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I882a340d4c1d205e4e998c888f487b7185000e3c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes