Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-4822: Dynamic log level changes to Catalog and Frontend JVMs ......................................................................
Patch Set 1: (6 comments) Thanks Alex for the reviews. Very helpful. I agree this needs some discussion on usability aspect. PS2 made some changes. 1. Changed the title to "Java log level (log4j)" to make it look more non-technical. 2. The log level input is now a drop down to make things more clearer to the end user. 3. It now has an example in the text box itself. Looks like this [1] 4. Regarding point (6), I'm not totally sure if we should something about it, I've added a comment in the CR. May be we can discuss it further. 5. Still working on a single button to restore log levels. @Henry: Still looking into the glog dynamic changes. Looks like it is possible and is as simple as FLAGS_v = <new_log_level>, but I'm still digging into it. [1] http://www.dumpt.com/img/viewer.php?file=8undlklgs7wt9q939hq9.png http://gerrit.cloudera.org:8080/#/c/5792/1/be/src/util/logging-support.cc File be/src/util/logging-support.cc: Line 26: #include "rpc/jni-thrift-util.h" > move before common/names.h Moved. For my understanding, whats special about names.h include? http://gerrit.cloudera.org:8080/#/c/5792/1/be/src/util/logging-support.h File be/src/util/logging-support.h: Line 21: #include "rapidjson/rapidjson.h" > I think we should be able to forward declare instead of include. Done. This one has some nested typedefs with some dependencies. Let me know if you think we should still retain it. http://gerrit.cloudera.org:8080/#/c/5792/1/common/thrift/Logging.thrift File common/thrift/Logging.thrift: Line 34: // Helper structs for GetLogLevel and SetLogLevel methods > GetLogLevel() and SetLogLevel() Done Line 35: struct TGetLogLevelParams { > Instead of this GetLogLevel() I think we should instead list all custom log Per my understanding log4j doesn't provide any easy way to get that list. So we need to manually track all the classes that were overriden via web UI. IMO that is some additional book keeping overhead on the Catalog server and some additional lines of code for a feature we don't often use unless we are debugging something. Thoughts? Line 41: > remove blank line Done http://gerrit.cloudera.org:8080/#/c/5792/1/fe/src/main/java/org/apache/impala/util/GlogAppender.java File fe/src/main/java/org/apache/impala/util/GlogAppender.java: Line 155: * Get the log4j log level corresponding to a seriazlied TGetLogLevelParams. > typo: serialized 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: 1 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-HasComments: Yes
