impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bharath Vissapragada (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Date Thu, 16 Feb 2017 00:34:31 GMT
Bharath Vissapragada has posted comments on this change.

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

Patch Set 12:


Fxed a minor bug in the web UI where the setting for java log level changes were showing up
on the statestore web UI which doesn't have a JVM.
File be/src/catalog/

Line 209: 
> remove blank line
File be/src/util/jni-util.h:

PS12, Line 230: the above
> explicitly say LoadJniMethod. Otherwise someone might put a method between 
haha, good point. Changed it.

PS12, Line 234: It seems these
              :   /// must be defined in the header to compile properly.
> can you remove this line (I probably wrote it...)? templates need to be in 
File be/src/util/

PS12, Line 129: Status SetJavaLogLevel(const TSetJavaLogLevelParams& params, string* result)
              :   RETURN_IF_ERROR(
              :       JniUtil::CallJniMethod(log4j_logger_class_, set_log_level_method, params,
              :   return Status::OK();
              : }
> Is this called in more than one place? Otherwise, let's just inline it into
Yea that is the only place. I've written this way think it would be more readable but I agree
it looks redundant.

PS12, Line 143: const Status& status
> What I mean was: rather than constructing a status every time you want to c
Ah sorry misunderstood. Made the change.

PS12, Line 149: SetDisplayResult
> how about 'AddDocumentMember()" ?
That sounds better. Done.

PS12, Line 158: log_getclass->second == nullptr
> Please remove the comparisons to nullptr for strings, and replace with .emp

PS12, Line 215: return
> Maybe make this case an error as well so that user knows what's missing.

PS12, Line 223: std::
> remove

PS12, Line 229: std::
> remove

Line 307: void RegisterLogLevelCallbacks(const string& url, Webserver* webserver, bool
register_log4j_handlers) {
> long line (consider using git-clang-format)

PS12, Line 310: set_glog_log
> sorry to nitpick - but set_glog_log seems repetitive. Why not set_glog_leve
Valid point. Changed.
File be/src/util/logging-support.h:

PS12, Line 39: init_log4j
> update

PS12, Line 40: const std::string& url
> is this needed, or is it the same for all callers?
Removed, its the same for all callers.
File fe/src/main/java/org/apache/impala/util/

Line 46: 
> remove blank line
File tests/webserver/

PS12, Line 86: \
> remove where you have used parentheses

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 12
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