Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-4822: Implement dynamic log level changes ......................................................................
Patch Set 5: (9 comments) webserver-test.cc seems to be more unit-testing the WebServer class and I don't think we can instantiate the JNI and test the actual code paths. So, I added new e-e tests to the patch. Please let me know if you think we should add more variants. 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) Done 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 Done Line 196: if (args.find("get_java_log") != args.end()) { > make these strings constants like: Done. Refactored it a little. Line 212: logging_level == args.end() || logging_level->second == NULL) return; > use {} for multi-line ifs Done 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. I'm fine with this too. Line 43: /// the Java log4j log messages to be forwarded to Glog. > update comment to reflect that it loads JNI classes/methods to support dyna Done 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 Done 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, Done Line 41: Log: <input type="text" name="setclass" placeholder="e.g. org.apache.foo"> > use a real class name 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: 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
