Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-4822: Implement dynamic log level changes ......................................................................
Patch Set 7: (16 comments) http://gerrit.cloudera.org:8080/#/c/5792/7/be/src/util/logging-support.cc File be/src/util/logging-support.cc: PS7, Line 225: result.insert(0, "Effective log level: "); > Mustache supports basic conditionals - you're using one already in the temp Oh makes sense. Thanks for explaining this. http://gerrit.cloudera.org:8080/#/c/5792/11/be/src/util/logging-support.cc File be/src/util/logging-support.cc: PS11, Line 81: ic jmethodID se > I don't think this is the default, but the originally set value (because th Done PS11, Line 96: MethodDescriptor get_log_ > anon namespace Ouch I thought I already put that. Got confused with the bracketing. Thanks for pointing that out. PS11, Line 217: s_name(log_setclass- > What information does Status::GetDetail() give you that just passing in the Any errors in the codepaths of calling jni methods (JniUtil::CallJniMethod()). PS11, Line 241: ::Parse > Although I see that this construction compiles in Impala, I can't make it c Done PS11, Line 251: > just 'classname' or similar. 'log_setclass' is hard to understand. Done PS11, Line 252: td::to_string > just 'level'? Prefer conciseness where you can reasonably do so without com Done Line 255: } else if (args.find("reset_glog_log") != args.end()) { > No SetErrorMessage()? Done PS11, Line 267: > empty Done PS11, Line 301: > if these are not used outside of this module, please put them in the anonym Done http://gerrit.cloudera.org:8080/#/c/5792/11/be/src/util/logging-support.h File be/src/util/logging-support.h: PS11, Line 22: #include "util/webserver.h" > not needed: just forward declare the class. Done Line 27: struct UTF8; > While you're here - re-wrap to 90 chars. Done Line 39: > std:: Done PS11, Line 39: > register_log4j_handlers? 'init_log4j' makes it sound like the log4j subsyst Done http://gerrit.cloudera.org:8080/#/c/5792/11/common/thrift/Logging.thrift File common/thrift/Logging.thrift: PS11, Line 34: // Helper structs for GetJavaLogLevel(), SetJavaLogLevel() and : // ResetJavaLogLevel() methods > This doesn't really help me - comment should say what they're used for. Done http://gerrit.cloudera.org:8080/#/c/5792/7/www/log_level.tmpl File www/log_level.tmpl: PS7, Line 76: <table> > Look at the other templates to see how they do layout? Thanks for the pointer. Pretty helpful. Moved the code to bootstrap. Looks much better. -- 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: 7 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-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-HasComments: Yes
