Henry Robinson has posted comments on this change. Change subject: IMPALA-4822: Implement dynamic log level changes ......................................................................
Patch Set 7: (22 comments) Took a look, had some comments. http://gerrit.cloudera.org:8080/#/c/5792/7/be/src/service/impala-http-handler.cc File be/src/service/impala-http-handler.cc: Line 37 where has this gone? where does the definition for Webserver::UrlCallback come from now? It's better to include-what-you-use. PS7, Line 92: bind<void>(LogLevelCallBack, _1, _2)) Everywhere else uses lambdas, please do the same here. http://gerrit.cloudera.org:8080/#/c/5792/7/be/src/util/logging-support.cc File be/src/util/logging-support.cc: PS7, Line 79: / only // for .cc files PS7, Line 78: static jclass log4j_logger_class_; : /// Jni method descriptors corresponding to getLogLevel() and setLogLevel() operations. : static jmethodID get_log_level_method; // GlogAppender.getLogLevel() : static jmethodID set_log_level_method; // GlogAppender.setLogLevel() : static jmethodID reset_log_levels_method; // GlogAppender.resetLogLevels() Do these need to be in the impala namespace? If not, move to anonymous namespace. PS7, Line 187: rapidjson remove PS7, Line 197: get_ don't need to prefix the parameters with 'get' PS7, Line 207: return why not SetErrorMsg() ? PS7, Line 207: NULL prefer nullptr now PS7, Line 224: if (result == NULL) return; how can result be nullptr if it's a stack-allocated string? This doesn't compile for me: std::string foo; if (foo == nullptr) return 1; PS7, Line 225: result.insert(0, "Effective log level: "); This kind of presentation logic probably belongs in the template, not here. Line 227: } else if (args.find("reset_java_log") != args.end()) { It might be easier to have several different callbacks: /reset_java_log /set_java_log?class=foo&level=2 to help break this method up. They can all still use the same template. Line 247: Status s("Bad glog level input. Valid inputs are integers in [0-3] range."); > in the [0-3] range Even better: in the range [0-3] PS7, Line 256: FLAGS_v gflags has a SetCommandLineOption() API. Consider using that here instead? PS7, Line 257: result = "Glog logging level reset to: " + std::to_string(FLAGS_v); use Substitute() PS7, Line 260: Value output(result.c_str(), document->GetAllocator()); : document->AddMember(display_member, output, document->GetAllocator()); I think it would be clearer to do this inline, and return rather than carry result and display_member through the method. Then you can get rid of the 'else's everywhere. http://gerrit.cloudera.org:8080/#/c/5792/7/be/src/util/logging-support.h File be/src/util/logging-support.h: PS7, Line 51: /// Helper method to set the log level of given Java class. It is a JNI wrapper around : /// GlogAppender.setLogLevel(). : Status SetJavaLogLevel(const TSetJavaLogLevelParams& params, string* response); : : /// Helper method to get the log level of given Java class. It is a JNI wrapper around : /// GlogAppender.getLogLevel(). : Status GetJavaLogLevel(const TGetJavaLogLevelParams& params, string* response); Why are these exported? There are no other consumers, right? Line 62: void LogLevelCallBack(const Webserver::ArgumentMap& args, rapidjson::Document* document); How about "RegisterLogLevelCallback(string url, Webserver*)" ? Then you don't need to worry about getting the rapidjson types declared. Line 66: void SetErrorMessage(const Status& status, const char* member, No need for this to be declared in the header. http://gerrit.cloudera.org:8080/#/c/5792/7/common/thrift/Logging.thrift File common/thrift/Logging.thrift: PS7, Line 45: TResetJavaLogLevelParams Reset implies 'return to defaults'. So it's confusing that this takes parameters - are those the defaults? http://gerrit.cloudera.org:8080/#/c/5792/7/tests/webserver/test_web_pages.py File tests/webserver/test_web_pages.py: PS7, Line 56: + it might be easier to read to wrap the string in parentheses: get_loglevel_url = ("http://localhost:25000/log_level?"; "getclass=org.apache.impala.catalog.HdfsTable&get_java_log=...") http://gerrit.cloudera.org:8080/#/c/5792/7/www/log_level.tmpl File www/log_level.tmpl: PS7, Line 32: b use <strong> instead of <b> PS7, Line 76: <table> Is there any way not to use tables for layout in this file? They are hard to maintain. -- 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-HasComments: Yes
