Alex Behm has posted comments on this change. Change subject: IMPALA-4822: Dynamic log level changes to Catalog and Frontend JVMs ......................................................................
Patch Set 1: (6 comments) This is awesome! Code looks good to me. If we really want to make this feature visible to everyone, then I think we need a few usability improvements (listed below). An alternative would be to make this a 'hidden feature' in which case we can ignore some of the usability concerns. I'd prefer to make it public, just listing some choices here. Usability improvements for the Web UI: 1. The Web UI should be clearer in terms of what input is expected in these text boxes, e.g., a full Java class name. Each box should provide an example of a valid input. 2. List all valid LOG levels on the webpage. 3. 'log4j' is too technical for the WebUI, how about "Java log level (log4j)" or something less technical 4. Show the current 'global' default log level 5. One button to restore the log levels to their original configuration. 6. List of custom LOG level changes that are currently in effect We don't have to do all those in one patch, but I think 1-3 are must-haves for this patch. 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 common/names.h is somewhat of a special 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. 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() Line 35: struct TGetLogLevelParams { Instead of this GetLogLevel() I think we should instead list all custom log levels that are in effect in addition to the default log level. That seems more user friendly and then this GetLogLevel() seems unnecessary. Line 41: remove blank line 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 -- 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-HasComments: Yes
