Alex Behm has posted comments on this change. Change subject: IMPALA-4822: Dynamic log level changes to Catalog and Frontend JVMs ......................................................................
Patch Set 1: (2 comments) Responding to comments. Will wait for next PS to continue the review. 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" > Moved. For my understanding, whats special about names.h include? names.h basically has a bunch of common "using" directives, so is more appropriate to put into the "using" section and not the "include" section if that makes sense http://gerrit.cloudera.org:8080/#/c/5792/1/common/thrift/Logging.thrift File common/thrift/Logging.thrift: Line 35: struct TGetLogLevelParams { > Per my understanding log4j doesn't provide any easy way to get that list. S Agree that there is additional bookeeping. I'm generally of the opinion that if the user has a way to change a configuration setting, then there should also be a way to inspect the current state of the configuration. This serves two purposes: 1. When changing a setting, the user gets immediate visual feedback that the setting is indeed in effect. 2. The user can easily see if he forgot to undo some changes he made in the past. It can avoid making redundant changes (forgot which classes he already increased the log level for). For now, I'm ok with implementing a button that reset the configuration back to the original state. Happy to discuss further. -- 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
