You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tamas Mate (Code Review)" <ge...@cloudera.org> on 2020/06/08 11:47:39 UTC

[Impala-ASF-CR] IMPALA-8860: Improve /log level usability on WebUI

Tamas Mate has posted comments on this change. ( http://gerrit.cloudera.org:8080/15903 )

Change subject: IMPALA-8860: Improve /log_level usability on WebUI
......................................................................


Patch Set 7:

(2 comments)

Apologies for the delay.

Based on my understanding the 'getLogLevel()' becomes deprecated and will not be called anymore. This method and its thrift object could be removed, other than this LGTM!

http://gerrit.cloudera.org:8080/#/c/15903/7/common/thrift/Logging.thrift
File common/thrift/Logging.thrift:

http://gerrit.cloudera.org:8080/#/c/15903/7/common/thrift/Logging.thrift@38
PS7, Line 38: struct TGetJavaLogLevelParams {
            :   1: required string class_name
            : }
I think this becomes unused as 'getLogLevel()' will not be called anymore.


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

http://gerrit.cloudera.org:8080/#/c/15903/7/fe/src/main/java/org/apache/impala/util/GlogAppender.java@145
PS7, Line 145:   /**
             :    * Get the log4j log level corresponding to a serialized TGetJavaLogLevelParams.
             :    */
             :   public static String getLogLevel(byte[] serializedParams) throws ImpalaException {
             :     TGetJavaLogLevelParams thriftParams = new TGetJavaLogLevelParams();
             :     JniUtil.deserializeThrift(protocolFactory_, thriftParams, serializedParams);
             :     String className = thriftParams.getClass_name();
             :     if (Strings.isNullOrEmpty(className)) return null;
             :     return Logger.getLogger(className).getEffectiveLevel().toString();
             :   }
I think this can be removed, as far as I can see it was only called by the BE earlier, which is now calling the 'getLogLevels()' method.



-- 
To view, visit http://gerrit.cloudera.org:8080/15903
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2fbf2ef21f4af297913a4e9b16a391768624da33
Gerrit-Change-Number: 15903
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Garaguly <zg...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Zoltan Garaguly <zg...@cloudera.com>
Gerrit-Comment-Date: Mon, 08 Jun 2020 11:47:39 +0000
Gerrit-HasComments: Yes