impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <ger...@cloudera.org>
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:

(16 comments)

Getting closer now.

http://gerrit.cloudera.org:8080/#/c/5792/12/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:

Line 209: 
remove blank line


http://gerrit.cloudera.org:8080/#/c/5792/12/be/src/util/jni-util.h
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.


http://gerrit.cloudera.org:8080/#/c/5792/12/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:

PS12, Line 129: Status SetJavaLogLevel(const TSetJavaLogLevelParams& params, string* result)
{
              :   RETURN_IF_ERROR(
              :       JniUtil::CallJniMethod(log4j_logger_class_, set_log_level_method, params,
result));
              :   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::
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_level etc.?


http://gerrit.cloudera.org:8080/#/c/5792/12/be/src/util/logging-support.h
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?


http://gerrit.cloudera.org:8080/#/c/5792/12/fe/src/main/java/org/apache/impala/util/GlogAppender.java
File fe/src/main/java/org/apache/impala/util/GlogAppender.java:

Line 46: 
remove blank line


http://gerrit.cloudera.org:8080/#/c/5792/12/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:

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


-- 
To view, visit http://gerrit.cloudera.org:8080/5792
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message