Henry Robinson has posted comments on this change. Change subject: IMPALA-4822: Implement dynamic log level changes ......................................................................
Patch Set 11: (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: alue output(result.c_str(), document->GetAl > I thought about this, but from what I understand, mustache templates are lo Mustache supports basic conditionals - you're using one already in the tempalte ({{^error}} - its contents are only evaluated if 'error' is not set). So instead of building the string to display here, set 'set_java_log_result' and in the template, do something like: {{?set_java_log_result}} Effective log level: {{set_java_log_result}} {{/set_java_log_result}} http://gerrit.cloudera.org:8080/#/c/5792/11/be/src/util/logging-support.cc File be/src/util/logging-support.cc: PS11, Line 81: FLAGS_v_default I don't think this is the default, but the originally set value (because the default can be overridden from the command line). Maybe FLAGS_v_original_value ? PS11, Line 96: InitDynamicLoggingSupport anon namespace PS11, Line 217: const Status& status What information does Status::GetDetail() give you that just passing in the error string does not? PS11, Line 241: nullptr Although I see that this construction compiles in Impala, I can't make it compile in other projects. My guess is that some compiler flag allows nullptr to be coerced to string via string(char*). A std::string is an object, so can't be directly compared to nullptr, I think what you mean here and elsewhere is to check of result is empty, so please do that directly: if (result.empty()) { ... PS11, Line 251: log_setclass just 'classname' or similar. 'log_setclass' is hard to understand. PS11, Line 252: logging_level just 'level'? Prefer conciseness where you can reasonably do so without compromising readability. Line 255: return; No SetErrorMessage()? PS11, Line 267: null empty PS11, Line 301: ResetGlogLevelCallback if these are not used outside of this module, please put them in the anonymous namespace so they don't pollute the impala namespace. 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. Line 27: /// Registers the required native logging functions with JNI. This allows While you're here - re-wrap to 90 chars. PS11, Line 39: init_log4j register_log4j_handlers? 'init_log4j' makes it sound like the log4j subsystem will be initialized. Line 39: void RegisterLogLevelCallbacks(const string& url, Webserver* webserver, bool init_log4j); std:: 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. http://gerrit.cloudera.org:8080/#/c/5792/7/www/log_level.tmpl File www/log_level.tmpl: PS7, Line 76: <table> > I agree it is hard to maintain. I'm not an expert on this. Do you have any Look at the other templates to see how they do layout? If you google for 'bootstrap form layout', you should find the bootstrap documentation etc. This tutorial seems ok: http://www.tutorialrepublic.com/twitter-bootstrap-tutorial/bootstrap-forms.php -- 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: 11 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-Reviewer: Marcel Kornacker <[email protected]> Gerrit-HasComments: Yes
