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 Tue, 14 Feb 2017 23:05:33 GMT
Henry Robinson has posted comments on this change.

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

Patch Set 12:


Getting closer now.
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 these two without
realising there's a problem.

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 headers, that's
pretty standard.
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 the callsite on
line 189.

PS12, Line 143: const Status& status
What I mean was: rather than constructing a status every time you want to call this with a
string, just pass in status.GetDetail().

PS12, Line 149: SetDisplayResult
how about 'AddDocumentMember()" ?

PS12, Line 158: log_getclass->second == nullptr
Please remove the comparisons to nullptr for strings, and replace with .empty() instead. In
general, reviews won't always call out every location where there's a common pattern to fix,
so it's good to take a look to see if you can spot any other cases.

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

PS12, Line 223: std::

PS12, Line 229: std::

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_level etc.?
File be/src/util/logging-support.h:

PS12, Line 39: init_log4j

PS12, Line 40: const std::string& url
is this needed, or is it 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