impala-reviews mailing list archives

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

(16 comments)

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.

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
Done


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 
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 
Done


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
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
Done


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


PS12, Line 223: std::
> remove
Done


PS12, Line 229: std::
> remove
Done


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


PS12, Line 310: set_glog_log
> sorry to nitpick - but set_glog_log seems repetitive. Why not set_glog_leve
Valid point. Changed.


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
Done


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.


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
Done


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
Done


-- 
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