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 Wed, 01 Feb 2017 22:23:19 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:

(2 comments)

Responding to comments. Will wait for next PS to continue the review.

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"
> Moved. For my understanding, whats special about names.h include?
names.h basically has a bunch of common "using" directives, so is more appropriate to put
into the "using" section and not the "include" section if that makes sense


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

Line 35: struct TGetLogLevelParams {
> Per my understanding log4j doesn't provide any easy way to get that list. S
Agree that there is additional bookeeping. I'm generally of the opinion that if the user has
a way to change a configuration setting, then there should also be a way to inspect the current
state of the configuration. This serves two purposes: 1. When changing a setting, the user
gets immediate visual feedback that the setting is indeed in effect. 2. The user can easily
see if he forgot to undo some changes he made in the past. It can avoid making redundant changes
(forgot which classes he already increased the log level for).

For now, I'm ok with implementing a button that reset the configuration back to the original
state.

Happy to discuss 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: 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