Alex Behm has posted comments on this change. Change subject: IMPALA-4822: Implement dynamic log level changes ......................................................................
Patch Set 5: (9 comments) Since we're accepting user input it would be great to have unit tests. Checking that the correct logging level is actually in effect might be a little tricky. I'm more thinking along the lines of checking garbage user input, checking that settings are reflected and can be reset, etc. Basic validation. I think you should be able to do something similar to webserver-test.cc. http://gerrit.cloudera.org:8080/#/c/5792/5/be/src/util/backend-gflag-util.h File be/src/util/backend-gflag-util.h: Line 27: /// debugging, so we save the original value here incase we need to restore in case (two words) http://gerrit.cloudera.org:8080/#/c/5792/5/be/src/util/logging-support.cc File be/src/util/logging-support.cc: Line 77: jclass log4j_logger_class_; no "_" suffix since not private class members; make these static Line 196: if (args.find("get_java_log") != args.end()) { make these strings constants like: static const char* GET_JAVA_LOG = "get_java_log"; etc. Line 212: logging_level == args.end() || logging_level->second == NULL) return; use {} for multi-line ifs http://gerrit.cloudera.org:8080/#/c/5792/5/be/src/util/logging-support.h File be/src/util/logging-support.h: Line 25: namespace rapidjson { Ok to use the include if you prefer that solution. Either wfm. Line 43: /// the Java log4j log messages to be forwarded to Glog. update comment to reflect that it loads JNI classes/methods to support dynamic log level changes http://gerrit.cloudera.org:8080/#/c/5792/5/common/thrift/Logging.thrift File common/thrift/Logging.thrift: Line 34: // Helper structs for GetLogLevel(), SetLogLevel() and ResetLogLevel() update http://gerrit.cloudera.org:8080/#/c/5792/5/www/log_level.tmpl File www/log_level.tmpl: Line 27: Log: <input type="text" name="getclass" placeholder="e.g. org.apache.foo"> Pretty sure org.apache.foo won't be clear to users. I suggest using a full, valid class name like org.apache.impala.analysis.Analyzer Line 41: Log: <input type="text" name="setclass" placeholder="e.g. org.apache.foo"> use a real class name -- 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: 5 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-HasComments: Yes
