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 <bhara...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Henry Robinson <he...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-HasComments: Yes
