impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4822: Dynamic log level changes to Catalog and Frontend JVMs
Date Sat, 28 Jan 2017 01:20:34 GMT
Alex Behm has posted comments on this change.

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


Patch Set 1:

(6 comments)

This is awesome! Code looks good to me.

If we really want to make this feature visible to everyone, then I think we need a few usability
improvements (listed below). An alternative would be to make this a 'hidden feature' in which
case we can ignore some of the usability concerns. I'd prefer to make it public, just listing
some choices here.

Usability improvements for the Web UI:
1. The Web UI should be clearer in terms of what input is expected in these text boxes, e.g.,
a full Java class name. Each box should provide an example of a valid input.
2. List all valid LOG levels on the webpage.
3. 'log4j' is too technical for the WebUI, how about "Java log level (log4j)" or something
less technical
4. Show the current 'global' default log level
5. One button to restore the log levels to their original configuration.
6. List of custom LOG level changes that are currently in effect

We don't have to do all those in one patch, but I think 1-3 are must-haves for this patch.

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

common/names.h is somewhat of a special 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.


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()


Line 35: struct TGetLogLevelParams {
Instead of this GetLogLevel() I think we should instead list all custom log levels that are
in effect in addition to the default log level. That seems more user friendly and then this
GetLogLevel() seems unnecessary.


Line 41: 
remove blank line


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


-- 
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-HasComments: Yes

Mime
View raw message