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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-HasComments: Yes
