impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bharath Vissapragada (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Date Wed, 08 Feb 2017 08:08:32 GMT
Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................


Patch Set 7:

(29 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 c
It is now included in logging-support.h


PS7, Line 92:  bind<void>(LogLevelCallBack, _1, _2))
> Everywhere else uses lambdas, please do the same here.
Moved the logic to RegisterLogLevelCallback() as per your suggestion.


http://gerrit.cloudera.org:8080/#/c/5792/7/be/src/util/backend-gflag-util.h
File be/src/util/backend-gflag-util.h:

Line 29: extern int FLAGS_v_default;
> Placement seems awkward. Maybe we can move thins into logging-support.h/cc 
Done. Moved to logging-support.cc


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
Done


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 name
Done


PS7, Line 187: rapidjson
> remove
Done


PS7, Line 197: get_
> don't need to prefix the parameters with 'get'
This was just to make it more readable. Refactored this code.


PS7, Line 207: return
> why not SetErrorMsg() ?
Done


PS7, Line 207: NULL
> prefer nullptr now
Done


PS7, Line 224: if (result == NULL) return;
> how can result be nullptr if it's a stack-allocated string? This doesn't co
Good point. Doesn't it even compile or doesn't it return 1? Anyway, NULL is returned by the
underlying java calls GetLogLevel() or SetLogLevel() and is set in JniUtil::CallJniMethod()


PS7, Line 225:  result.insert(0, "Effective log level: ");
> This kind of presentation logic probably belongs in the template, not here.
I thought about this, but from what I understand, mustache templates are logic-less. Am I
missing something? Basically this should appear only if we set a particular command output.


Line 227:   } else if (args.find("reset_java_log") != args.end()) {
> It might be easier to have several different callbacks:
Yes I was thinking of doing this as the big method seems to be difficult to maintain and understand.
Made the changes.


Line 243:         StringParser::StringToInt<int>(glog_level->second.c_str(),
> use data() instead of c_str()
Removed this part of code and instead added a validator function.


Line 247:       Status s("Bad glog level input. Valid inputs are integers in [0-3] range.");
> Even better: in the range [0-3]
Done


Line 247:       Status s("Bad glog level input. Valid inputs are integers in [0-3] range.");
> in the [0-3] range
Done


PS7, Line 256: FLAGS_v
> gflags has a SetCommandLineOption() API. Consider using that here instead?
I looked into it and basically does the same thing, like parsing from a string and setting
the flag. Made the change now, just to be on the safer side like avoid races etc. Also looks
like gflag lets us register a validator function using RegisterFlagValidator() that is called
everytime we reset the flag to a newer value. I added one so that we don't accidentally set
it to a bad value. LKM if you'd like any changes in that.


PS7, Line 257: result = "Glog logging level reset to: " + std::to_string(FLAGS_v);
> use Substitute()
Done


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
Done


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?
Sorry I forgot to clean these up along with ResetJavaLogLevel. Removed.


Line 62: void LogLevelCallBack(const Webserver::ArgumentMap& args, rapidjson::Document*
document);
> How about "RegisterLogLevelCallback(string url, Webserver*)" ?
Good idea. That cleans up the rapidjson stuff here.


Line 66: void SetErrorMessage(const Status& status, const char* member,
> No need for this to be declared in the header.
Done


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 param
They correspond to the user provided startup params --v and ---non_impala_java_vlog. We don't
save them in the frontend code, so we need to pass them as params from the backend everytime
we call reset. Do you prefer to save them as static variables somewhere in the frontend?


http://gerrit.cloudera.org:8080/#/c/5792/7/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:

Line 54:     self.get_and_check_status("http://localhost:25000/log_level")
> let's also test these for the statestored and catalogd like in the memz tes
Done


PS7, Line 56: +
> it might be easier to read to wrap the string in parentheses:
Done


Line 83:     set_glog_url  =\
> extra space
Done


Line 103:     set_glog_url  =\
> extra space
Done


Line 116:     set_glog_url  =\
> extra space
Done


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>
Done


PS7, Line 76: <table>
> Is there any way not to use tables for layout in this file? They are hard t
I agree it is hard to maintain. I'm not an expert on this. Do you have any suggestions that
I can dig into further?


-- 
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 <bharathv@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message