impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Date Thu, 09 Feb 2017 19:52:30 GMT
Henry Robinson has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes

Patch Set 11:

File be/src/util/

PS7, Line 225: alue output(result.c_str(), document->GetAl
> I thought about this, but from what I understand, mustache templates are lo
Mustache supports basic conditionals - you're using one already in the tempalte ({{^error}}
- its contents are only evaluated if 'error' is not set). So instead of building the string
to display here, set 'set_java_log_result' and in the template, do something like:

    Effective log level: {{set_java_log_result}}
File be/src/util/

PS11, Line 81: FLAGS_v_default
I don't think this is the default, but the originally set value (because the default can be
overridden from the command line).

Maybe FLAGS_v_original_value ?

PS11, Line 96: InitDynamicLoggingSupport
anon namespace

PS11, Line 217: const Status& status
What information does Status::GetDetail() give you that just passing in the error string does

PS11, Line 241: nullptr
Although I see that this construction compiles in Impala, I can't make it compile in other
projects. My guess is that some compiler flag allows nullptr to be coerced to string via string(char*).

A std::string is an object, so can't be directly compared to nullptr, I think what you mean
here and elsewhere is to check of result is empty, so please do that directly: if (result.empty())
{ ...

PS11, Line 251: log_setclass
just 'classname' or similar. 'log_setclass' is hard to understand.

PS11, Line 252: logging_level
just 'level'? Prefer conciseness where you can reasonably do so without compromising readability.

Line 255:     return;
No SetErrorMessage()?

PS11, Line 267: null

PS11, Line 301: ResetGlogLevelCallback
if these are not used outside of this module, please put them in the anonymous namespace so
they don't pollute the impala namespace.
File be/src/util/logging-support.h:

PS11, Line 22: #include "util/webserver.h"
not needed: just forward declare the class.

Line 27: /// Registers the required native logging functions with JNI. This allows
While you're here - re-wrap to 90 chars.

PS11, Line 39: init_log4j
register_log4j_handlers? 'init_log4j' makes it sound like the log4j subsystem will be initialized.

Line 39: void RegisterLogLevelCallbacks(const string& url, Webserver* webserver, bool
File common/thrift/Logging.thrift:

PS11, Line 34: // Helper structs for GetJavaLogLevel(), SetJavaLogLevel() and
             : // ResetJavaLogLevel() methods
This doesn't really help me - comment should say what they're used for.
File www/log_level.tmpl:

PS7, Line 76: <table>
> I agree it is hard to maintain. I'm not an expert on this. Do you have any 
Look at the other templates to see how they do layout?

If you google for 'bootstrap form layout', you should find the bootstrap documentation etc.
This tutorial seems ok:

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Bharath Vissapragada <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-HasComments: Yes

View raw message