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: Dynamic log level changes to Catalog and Frontend JVMs
Date Tue, 31 Jan 2017 20:03:15 GMT
Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-4822: Dynamic log level changes to Catalog and Frontend JVMs
......................................................................


Patch Set 1:

(6 comments)

Thanks Alex for the reviews. Very helpful. I agree this needs some discussion on usability
aspect. PS2 made some changes.

1. Changed the title to "Java log level (log4j)" to make it look more non-technical.
2. The log level input is now a drop down to make things more clearer to the end user.
3. It now has an example in the text box itself. Looks like this [1]
4. Regarding point (6), I'm not totally sure if we should something about it, I've added a
comment in the CR. May be we can discuss it further.
5. Still working on a single button to restore log levels.

@Henry: Still looking into the glog dynamic changes. Looks like it is possible and is as simple
as FLAGS_v = <new_log_level>, but I'm still digging into it.

[1] http://www.dumpt.com/img/viewer.php?file=8undlklgs7wt9q939hq9.png

http://gerrit.cloudera.org:8080/#/c/5792/1/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:

Line 26: #include "rpc/jni-thrift-util.h"
> move before common/names.h
Moved. For my understanding, whats special about names.h include?


http://gerrit.cloudera.org:8080/#/c/5792/1/be/src/util/logging-support.h
File be/src/util/logging-support.h:

Line 21: #include "rapidjson/rapidjson.h"
> I think we should be able to forward declare instead of include.
Done. This one has some nested typedefs with some dependencies. Let me know if you think we
should still retain it.


http://gerrit.cloudera.org:8080/#/c/5792/1/common/thrift/Logging.thrift
File common/thrift/Logging.thrift:

Line 34: // Helper structs for GetLogLevel and SetLogLevel methods
> GetLogLevel() and SetLogLevel()
Done


Line 35: struct TGetLogLevelParams {
> Instead of this GetLogLevel() I think we should instead list all custom log
Per my understanding log4j doesn't provide any easy way to get that list. So we need to manually
track all the classes that were overriden via web UI. IMO that is some additional book keeping
overhead on the Catalog server and some additional lines of code for a feature we don't often
use unless we are debugging something. Thoughts?


Line 41: 
> remove blank line
Done


http://gerrit.cloudera.org:8080/#/c/5792/1/fe/src/main/java/org/apache/impala/util/GlogAppender.java
File fe/src/main/java/org/apache/impala/util/GlogAppender.java:

Line 155:    * Get the log4j log level corresponding to a seriazlied TGetLogLevelParams.
> typo: serialized
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: 1
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