You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org> on 2017/01/25 07:41:43 UTC
[Impala-ASF-CR] IMPALA-4822: Dynamic log level changes to Catalog and Frontend JVMs
Bharath Vissapragada has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5792
Change subject: IMPALA-4822: Dynamic log level changes to Catalog and Frontend JVMs
......................................................................
IMPALA-4822: Dynamic log level changes to Catalog and Frontend JVMs
Very often we have to change the log4j logging levels
of Impalads and Catalog server for debugging purposes.
Currently, there is no way to do that without a restart
of the daemons, which is not a viable option in production
deployments.
This patch addresses this supportability gap by exposing
the ability to set dynamic log4j logging levels using
a simple web endpoint on Impalad and Catalog web pages.
Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
---
M be/src/catalog/catalog-server.cc
M be/src/service/impala-http-handler.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M be/src/util/logging-support.h
M common/thrift/Logging.thrift
M fe/src/main/java/org/apache/impala/util/GlogAppender.java
A www/log_level.tmpl
9 files changed, 197 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/5792/1
--
To view, visit http://gerrit.cloudera.org:8080/5792
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#9).
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
IMPALA-4822: Implement dynamic log level changes
Very often we have to change the logging levels
of Impalads and Catalog server for debugging purposes.
Currently, there is no way to do that without a restart
of the daemons, which is not a viable option in production
deployments.
This patch addresses this supportability gap by exposing
the ability to set dynamic logging levels using a simple
web endpoint on Impalad and Catalog web pages.
This includes setting VLOG levels (equivalent to --v flag)
as well as setting log4j levels on the Frontend and the
Catalog JVMs.
Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
---
M be/src/catalog/catalog-server.cc
M be/src/service/impala-http-handler.cc
M be/src/statestore/statestore.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M be/src/util/logging-support.h
M common/thrift/Logging.thrift
M fe/src/main/java/org/apache/impala/util/GlogAppender.java
M tests/webserver/test_web_pages.py
A www/log_level.tmpl
11 files changed, 480 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/5792/9
--
To view, visit http://gerrit.cloudera.org:8080/5792
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 14: Code-Review+2
Discussed offline. I believe the out-of-the-box behaviour hasn't changed.
--
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: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 13:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5792/13/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:
Line 40
> Does that mean that severities that get mapped to VLOG will always get prin
Yes, Interestingly that has always been the behavior (even without this patch because --v=1 is the default). We have the following mapping that we use to translate. I'm not totally sure what the rationale is behind this mapping.
private TLogLevel levelToSeverity(Level level) {
Preconditions.checkState(!level.equals(Level.OFF));
// TODO: Level does not work well in a HashMap or switch statement due to some
// strangeness with equality testing.
if (level.equals(Level.TRACE)) return TLogLevel.VLOG_3;
if (level.equals(Level.ALL)) return TLogLevel.VLOG_3;
if (level.equals(Level.DEBUG)) return TLogLevel.VLOG;
if (level.equals(Level.ERROR)) return TLogLevel.ERROR;
if (level.equals(Level.FATAL)) return TLogLevel.FATAL;
if (level.equals(Level.INFO)) return TLogLevel.INFO;
if (level.equals(Level.WARN)) return TLogLevel.WARN;
throw new IllegalStateException("Unknown log level: " + level.toString());
}
--
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: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#10).
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
IMPALA-4822: Implement dynamic log level changes
Very often we have to change the logging levels
of Impalads and Catalog server for debugging purposes.
Currently, there is no way to do that without a restart
of the daemons, which is not a viable option in production
deployments.
This patch addresses this supportability gap by exposing
the ability to set dynamic logging levels using a simple
web endpoint on Impalad and Catalog web pages.
This includes setting VLOG levels (equivalent to --v flag)
as well as setting log4j levels on the Frontend and the
Catalog JVMs.
Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
---
M be/src/catalog/catalog-server.cc
M be/src/service/impala-http-handler.cc
M be/src/statestore/statestore.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M be/src/util/logging-support.h
M common/thrift/Logging.thrift
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/util/GlogAppender.java
M tests/webserver/test_web_pages.py
A www/log_level.tmpl
12 files changed, 483 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/5792/10
--
To view, visit http://gerrit.cloudera.org:8080/5792
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 5:
(9 comments)
Since we're accepting user input it would be great to have unit tests. Checking that the correct logging level is actually in effect might be a little tricky. I'm more thinking along the lines of checking garbage user input, checking that settings are reflected and can be reset, etc. Basic validation. I think you should be able to do something similar to webserver-test.cc.
http://gerrit.cloudera.org:8080/#/c/5792/5/be/src/util/backend-gflag-util.h
File be/src/util/backend-gflag-util.h:
Line 27: /// debugging, so we save the original value here incase we need to restore
in case (two words)
http://gerrit.cloudera.org:8080/#/c/5792/5/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:
Line 77: jclass log4j_logger_class_;
no "_" suffix since not private class members; make these static
Line 196: if (args.find("get_java_log") != args.end()) {
make these strings constants like:
static const char* GET_JAVA_LOG = "get_java_log";
etc.
Line 212: logging_level == args.end() || logging_level->second == NULL) return;
use {} for multi-line ifs
http://gerrit.cloudera.org:8080/#/c/5792/5/be/src/util/logging-support.h
File be/src/util/logging-support.h:
Line 25: namespace rapidjson {
Ok to use the include if you prefer that solution. Either wfm.
Line 43: /// the Java log4j log messages to be forwarded to Glog.
update comment to reflect that it loads JNI classes/methods to support dynamic log level changes
http://gerrit.cloudera.org:8080/#/c/5792/5/common/thrift/Logging.thrift
File common/thrift/Logging.thrift:
Line 34: // Helper structs for GetLogLevel(), SetLogLevel() and ResetLogLevel()
update
http://gerrit.cloudera.org:8080/#/c/5792/5/www/log_level.tmpl
File www/log_level.tmpl:
Line 27: Log: <input type="text" name="getclass" placeholder="e.g. org.apache.foo">
Pretty sure org.apache.foo won't be clear to users. I suggest using a full, valid class name like org.apache.impala.analysis.Analyzer
Line 41: Log: <input type="text" name="setclass" placeholder="e.g. org.apache.foo">
use a real class name
--
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#6).
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
IMPALA-4822: Implement dynamic log level changes
Very often we have to change the logging levels
of Impalads and Catalog server for debugging purposes.
Currently, there is no way to do that without a restart
of the daemons, which is not a viable option in production
deployments.
This patch addresses this supportability gap by exposing
the ability to set dynamic logging levels using a simple
web endpoint on Impalad and Catalog web pages.
This includes setting VLOG levels (equivalent to --v flag)
as well as setting log4j levels on the Frontend and the
Catalog JVMs.
Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
---
M be/src/catalog/catalog-server.cc
M be/src/service/impala-http-handler.cc
M be/src/util/backend-gflag-util.cc
M be/src/util/backend-gflag-util.h
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M be/src/util/logging-support.h
M common/thrift/Logging.thrift
M fe/src/main/java/org/apache/impala/util/GlogAppender.java
M tests/webserver/test_web_pages.py
A www/log_level.tmpl
12 files changed, 415 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/5792/6
--
To view, visit http://gerrit.cloudera.org:8080/5792
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 15:
Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/319/
--
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: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#5).
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
IMPALA-4822: Implement dynamic log level changes
Very often we have to change the logging levels
of Impalads and Catalog server for debugging purposes.
Currently, there is no way to do that without a restart
of the daemons, which is not a viable option in production
deployments.
This patch addresses this supportability gap by exposing
the ability to set dynamic logging levels using a simple
web endpoint on Impalad and Catalog web pages.
This includes setting VLOG levels (equivalent to --v flag)
as well as setting log4j levels on the Frontend and the
Catalog JVMs.
Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
---
M be/src/catalog/catalog-server.cc
M be/src/service/impala-http-handler.cc
M be/src/util/backend-gflag-util.cc
M be/src/util/backend-gflag-util.h
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M be/src/util/logging-support.h
M common/thrift/Logging.thrift
M fe/src/main/java/org/apache/impala/util/GlogAppender.java
A www/log_level.tmpl
11 files changed, 343 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/5792/5
--
To view, visit http://gerrit.cloudera.org:8080/5792
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 15:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/310/
--
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: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 13:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5792/13/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:
Line 40
> My understanding is that it should be controlled by log4j on the JVM side r
This mechanism exists so that we can configure Impala's logging in one place, and have the behaviour be consistent across FE and BE. I don't think we should introduce the burden of having them configured separately - is that what this change now implies? Is trace logging off by default?
We should expect that 95% of deployments won't use this feature for fine-grained control, so the out-of-the-box behaviour has to be good enough for them.
--
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: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 16:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/347/
--
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: 16
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 13:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5792/13/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:
Line 40
> Right, but the idea is that if we want to turn off trace debugging, for exa
My understanding is that it should be controlled by log4j on the JVM side rather than using glog. In this case, if we want to turn off trace debugging, we can just "reset Java log levels" and that disables trace logging on the Java side itself and the messages won't be passed from JVM to here.
--
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: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5792/7/common/thrift/Logging.thrift
File common/thrift/Logging.thrift:
PS7, Line 45: TResetJavaLogLevelParams
> Yes, I'll do it then. Just wanted to confirm that is the ask here.
I would also settle for comments and / or a better method name if this is the 'right' place to have the parameters.
--
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 15:
Sounds good - I commented on the bugfix, hopefully we can get these both in quickly.
--
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: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 13:
Henry, do you have any further comments on this? Thanks.
--
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: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 13:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5792/13/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:
Line 40
> These come in the way of dynamic log4j dynamic log level changes because th
Does that mean that severities that get mapped to VLOG will always get printed? i.e. if we have LOG.debug("something") in the frontend, that will get printed at INFO level (per line 55)?
--
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: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 15:
@Henry: The GVO of this patch exposed an unrelated bug that I fixed in another CR [1]. I'll GVO this again once that is merged.
[1] https://gerrit.cloudera.org/#/c/6264
--
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: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4822: Dynamic log level changes to Catalog and Frontend JVMs
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 15:
Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/316/
--
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: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4822: Dynamic log level changes to Catalog and Frontend JVMs
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#2).
Change subject: IMPALA-4822: Dynamic log level changes to Catalog and Frontend JVMs
......................................................................
IMPALA-4822: Dynamic log level changes to Catalog and Frontend JVMs
Very often we have to change the log4j logging levels
of Impalads and Catalog server for debugging purposes.
Currently, there is no way to do that without a restart
of the daemons, which is not a viable option in production
deployments.
This patch addresses this supportability gap by exposing
the ability to set dynamic log4j logging levels using
a simple web endpoint on Impalad and Catalog web pages.
Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
---
M be/src/catalog/catalog-server.cc
M be/src/service/impala-http-handler.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M be/src/util/logging-support.h
M common/thrift/Logging.thrift
M fe/src/main/java/org/apache/impala/util/GlogAppender.java
A www/log_level.tmpl
9 files changed, 228 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/5792/2
--
To view, visit http://gerrit.cloudera.org:8080/5792
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 1:
(2 comments)
PS4 implements dynamic logging changes to the backend (equivalent to setting --v on the fly) and also the "reset" functionality for both log4j and the backend. That resets the logging state as per the user defined values at the startup.
Also, I'm looking for some suggestions on cosmetic changes to the log_level web end point. As of now it works, but looks pretty basic (like it is designed by someone who just learnt HTML :D). So that needs to be improved as well.
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"
> names.h basically has a bunch of common "using" directives, so is more appr
Thanks for explaining.
http://gerrit.cloudera.org:8080/#/c/5792/1/common/thrift/Logging.thrift
File common/thrift/Logging.thrift:
Line 35: struct TGetLogLevelParams {
> Agree that there is additional bookeeping. I'm generally of the opinion tha
I agree with your point that having a visual feedback is much more easy to the end user. For now I've implemented the "reset" functionality. Still need to look into log4j if there is a way to get all the overrides.
--
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4822: Dynamic log level changes to Catalog and Frontend JVMs
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#3).
Change subject: IMPALA-4822: Dynamic log level changes to Catalog and Frontend JVMs
......................................................................
IMPALA-4822: Dynamic log level changes to Catalog and Frontend JVMs
Very often we have to change the log4j logging levels
of Impalads and Catalog server for debugging purposes.
Currently, there is no way to do that without a restart
of the daemons, which is not a viable option in production
deployments.
This patch addresses this supportability gap by exposing
the ability to set dynamic log4j logging levels using
a simple web endpoint on Impalad and Catalog web pages.
Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
---
M be/src/catalog/catalog-server.cc
M be/src/service/impala-http-handler.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M be/src/util/logging-support.h
M common/thrift/Logging.thrift
M fe/src/main/java/org/apache/impala/util/GlogAppender.java
A www/log_level.tmpl
9 files changed, 226 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/5792/3
--
To view, visit http://gerrit.cloudera.org:8080/5792
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
[Impala-ASF-CR] IMPALA-4822: Dynamic log level changes to Catalog and Frontend JVMs
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4822: Dynamic log level changes to Catalog and Frontend JVMs
......................................................................
Patch Set 1:
Could you also add support for changing log levels in C++ code? I think that's possible since gflags lets you change flags through an API.
--
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 10:
PS10 gets rid of TResetJavaLogLevelParams.
--
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: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 13: Code-Review+2
(4 comments)
Looks pretty good to me.
http://gerrit.cloudera.org:8080/#/c/5792/13/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:
Line 40
What's the effect of removing these? Where is the VLOG masking done now?
http://gerrit.cloudera.org:8080/#/c/5792/13/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:
PS13, Line 127: set_glog_url =\
: (self.SET_GLOG_LOGLEVEL_URL + "?glog=foo")
nit: one line, no parentheses?
http://gerrit.cloudera.org:8080/#/c/5792/13/www/log_level.tmpl
File www/log_level.tmpl:
PS13, Line 21: error
It might make more sense to print the form if there was an error, so that the mistake can be rectified.
PS13, Line 25: width: 600px;
Why set the width? Does the page look bad if you let it figure out the width from the parent div?
--
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: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 13:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5792/13/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:
Line 40
> Yes, Interestingly that has always been the behavior (even without this pat
Right, but the idea is that if we want to turn off trace debugging, for example, we can use the usual controls to do so (by setting GLOG_v < 3).
What happens with trace log messages now? Are they always printed?
--
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: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 7:
(22 comments)
Took a look, had some 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 come from now? It's better to include-what-you-use.
PS7, Line 92: bind<void>(LogLevelCallBack, _1, _2))
Everywhere else uses lambdas, please do the same here.
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
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 namespace.
PS7, Line 187: rapidjson
remove
PS7, Line 197: get_
don't need to prefix the parameters with 'get'
PS7, Line 207: return
why not SetErrorMsg() ?
PS7, Line 207: NULL
prefer nullptr now
PS7, Line 224: if (result == NULL) return;
how can result be nullptr if it's a stack-allocated string? This doesn't compile for me:
std::string foo;
if (foo == nullptr) return 1;
PS7, Line 225: result.insert(0, "Effective log level: ");
This kind of presentation logic probably belongs in the template, not here.
Line 227: } else if (args.find("reset_java_log") != args.end()) {
It might be easier to have several different callbacks:
/reset_java_log
/set_java_log?class=foo&level=2
to help break this method up. They can all still use the same template.
Line 247: Status s("Bad glog level input. Valid inputs are integers in [0-3] range.");
> in the [0-3] range
Even better: in the range [0-3]
PS7, Line 256: FLAGS_v
gflags has a SetCommandLineOption() API. Consider using that here instead?
PS7, Line 257: result = "Glog logging level reset to: " + std::to_string(FLAGS_v);
use Substitute()
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 result and display_member through the method.
Then you can get rid of the 'else's everywhere.
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?
Line 62: void LogLevelCallBack(const Webserver::ArgumentMap& args, rapidjson::Document* document);
How about "RegisterLogLevelCallback(string url, Webserver*)" ?
Then you don't need to worry about getting the rapidjson types declared.
Line 66: void SetErrorMessage(const Status& status, const char* member,
No need for this to be declared in the header.
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 parameters - are those the defaults?
http://gerrit.cloudera.org:8080/#/c/5792/7/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:
PS7, Line 56: +
it might be easier to read to wrap the string in parentheses:
get_loglevel_url = ("http://localhost:25000/log_level?"
"getclass=org.apache.impala.catalog.HdfsTable&get_java_log=...")
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>
PS7, Line 76: <table>
Is there any way not to use tables for layout in this file? They are hard to maintain.
--
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 12:
(16 comments)
Getting closer now.
http://gerrit.cloudera.org:8080/#/c/5792/12/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:
Line 209:
remove blank line
http://gerrit.cloudera.org:8080/#/c/5792/12/be/src/util/jni-util.h
File be/src/util/jni-util.h:
PS12, Line 230: the above
explicitly say LoadJniMethod. Otherwise someone might put a method between these two without realising there's a problem.
PS12, Line 234: It seems these
: /// must be defined in the header to compile properly.
can you remove this line (I probably wrote it...)? templates need to be in headers, that's pretty standard.
http://gerrit.cloudera.org:8080/#/c/5792/12/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:
PS12, Line 129: Status SetJavaLogLevel(const TSetJavaLogLevelParams& params, string* result) {
: RETURN_IF_ERROR(
: JniUtil::CallJniMethod(log4j_logger_class_, set_log_level_method, params, result));
: return Status::OK();
: }
Is this called in more than one place? Otherwise, let's just inline it into the callsite on line 189.
PS12, Line 143: const Status& status
What I mean was: rather than constructing a status every time you want to call this with a string, just pass in status.GetDetail().
PS12, Line 149: SetDisplayResult
how about 'AddDocumentMember()" ?
PS12, Line 158: log_getclass->second == nullptr
Please remove the comparisons to nullptr for strings, and replace with .empty() instead. In general, reviews won't always call out every location where there's a common pattern to fix, so it's good to take a look to see if you can spot any other cases.
PS12, Line 215: return
Maybe make this case an error as well so that user knows what's missing.
PS12, Line 223: std::
remove
PS12, Line 229: std::
remove
Line 307: void RegisterLogLevelCallbacks(const string& url, Webserver* webserver, bool register_log4j_handlers) {
long line (consider using git-clang-format)
PS12, Line 310: set_glog_log
sorry to nitpick - but set_glog_log seems repetitive. Why not set_glog_level etc.?
http://gerrit.cloudera.org:8080/#/c/5792/12/be/src/util/logging-support.h
File be/src/util/logging-support.h:
PS12, Line 39: init_log4j
update
PS12, Line 40: const std::string& url
is this needed, or is it the same for all callers?
http://gerrit.cloudera.org:8080/#/c/5792/12/fe/src/main/java/org/apache/impala/util/GlogAppender.java
File fe/src/main/java/org/apache/impala/util/GlogAppender.java:
Line 46:
remove blank line
http://gerrit.cloudera.org:8080/#/c/5792/12/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:
PS12, Line 86: \
remove where you have used parentheses
--
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: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#11).
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
IMPALA-4822: Implement dynamic log level changes
Very often we have to change the logging levels
of Impalads and Catalog server for debugging purposes.
Currently, there is no way to do that without a restart
of the daemons, which is not a viable option in production
deployments.
This patch addresses this supportability gap by exposing
the ability to set dynamic logging levels using a simple
web endpoint on Impalad and Catalog web pages.
This includes setting VLOG levels (equivalent to --v flag)
as well as setting log4j levels on the Frontend and the
Catalog JVMs.
Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
---
M be/src/catalog/catalog-server.cc
M be/src/service/impala-http-handler.cc
M be/src/statestore/statestore.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M be/src/util/logging-support.h
M common/thrift/Logging.thrift
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/util/GlogAppender.java
M tests/webserver/test_web_pages.py
A www/log_level.tmpl
12 files changed, 483 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/5792/11
--
To view, visit http://gerrit.cloudera.org:8080/5792
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
[Impala-ASF-CR] IMPALA-4822: Dynamic log level changes to Catalog and Frontend JVMs
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4822: Dynamic log level changes to Catalog and Frontend JVMs
......................................................................
Patch Set 1:
I created a small screencast here [1] to demo this enhancement. In that video I try to change the log level of class org.apache.impala.catalog.HdfsTable at runtime from DEBUG to TRACE and show that it produces additional logging. Feel free to play with it and appreciate any feedback.
[1] https://vimeo.com/200946011
--
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 <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5792/7/common/thrift/Logging.thrift
File common/thrift/Logging.thrift:
PS7, Line 45: TResetJavaLogLevelParams
> They correspond to the user provided startup params --v and ---non_impala_j
We could pass them along with all the other BE gflags in GetThriftBackendGflags().
--
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#7).
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
IMPALA-4822: Implement dynamic log level changes
Very often we have to change the logging levels
of Impalads and Catalog server for debugging purposes.
Currently, there is no way to do that without a restart
of the daemons, which is not a viable option in production
deployments.
This patch addresses this supportability gap by exposing
the ability to set dynamic logging levels using a simple
web endpoint on Impalad and Catalog web pages.
This includes setting VLOG levels (equivalent to --v flag)
as well as setting log4j levels on the Frontend and the
Catalog JVMs.
Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
---
M be/src/catalog/catalog-server.cc
M be/src/service/impala-http-handler.cc
M be/src/util/backend-gflag-util.cc
M be/src/util/backend-gflag-util.h
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M be/src/util/logging-support.h
M common/thrift/Logging.thrift
M fe/src/main/java/org/apache/impala/util/GlogAppender.java
M tests/webserver/test_web_pages.py
A www/log_level.tmpl
12 files changed, 443 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/5792/7
--
To view, visit http://gerrit.cloudera.org:8080/5792
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Hello Henry Robinson,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5792
to look at the new patch set (#15).
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
IMPALA-4822: Implement dynamic log level changes
Very often we have to change the logging levels
of Impalads and Catalog server for debugging purposes.
Currently, there is no way to do that without a restart
of the daemons, which is not a viable option in production
deployments.
This patch addresses this supportability gap by exposing
the ability to set dynamic logging levels using a simple
web endpoint on Impalad/Catalog/Statestore web pages.
This includes setting VLOG levels (equivalent to --v flag)
as well as setting log4j levels on the Frontend and the
Catalog JVMs.
Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
---
M be/src/catalog/catalog-server.cc
M be/src/service/impala-http-handler.cc
M be/src/statestore/statestore.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M be/src/util/logging-support.h
M common/thrift/Logging.thrift
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/util/GlogAppender.java
M tests/webserver/test_web_pages.py
A www/log_level.tmpl
12 files changed, 469 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/5792/15
--
To view, visit http://gerrit.cloudera.org:8080/5792
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#8).
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
IMPALA-4822: Implement dynamic log level changes
Very often we have to change the logging levels
of Impalads and Catalog server for debugging purposes.
Currently, there is no way to do that without a restart
of the daemons, which is not a viable option in production
deployments.
This patch addresses this supportability gap by exposing
the ability to set dynamic logging levels using a simple
web endpoint on Impalad and Catalog web pages.
This includes setting VLOG levels (equivalent to --v flag)
as well as setting log4j levels on the Frontend and the
Catalog JVMs.
Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
---
M be/src/catalog/catalog-server.cc
M be/src/service/impala-http-handler.cc
M be/src/statestore/statestore.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M be/src/util/logging-support.h
M common/thrift/Logging.thrift
M fe/src/main/java/org/apache/impala/util/GlogAppender.java
M tests/webserver/test_web_pages.py
A www/log_level.tmpl
11 files changed, 485 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/5792/8
--
To view, visit http://gerrit.cloudera.org:8080/5792
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 6:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/5792/6/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:
Line 83: static const string log_inputs[] = {"get_java_log", "set_java_log", "reset_java_log",
> Sorry, I didn't realize the log_inputs and the display_members were differe
Changes undone. Renamed a little as discussed offline.
http://gerrit.cloudera.org:8080/#/c/5792/6/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:
Line 43: """ Helper method that polls a given url and asserts the return code is ok and
> nit: remove space before "Helper"
Done
Line 48: def test_log_level_callback(self):
> test_log_level
Done
Line 50: malformed inputs"""
> Does not test that the log level modifications are actually in effect.
Done
Line 59: # Set the log level of a class to TRACE anc confirm the setting is in place
> typo: and
Done
Line 67: self.get_and_check_status(get_loglevel_url, "TRACE")
> also check a different class and make sure it's still at DEBUG
Done
Line 75: self.get_and_check_status(get_loglevel_url, "DEBUG")
> also add tests for malformed inputs, e.g., a class that does not exist, emp
Added a few more tests. Also, per my understanding, in log4j, there is no such thing as a class that does not exists. It accepts any class that we input (and sets a log level), since we can actually load them at runtime, if it is not already loaded.
--
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4822: Dynamic log level changes to Catalog and Frontend JVMs
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4822: Dynamic log level changes to Catalog and Frontend JVMs
......................................................................
Patch Set 3:
@reviewers: Please don't review PS2/3. My next PS includes a bunch of new stuff. I'll send it out for review soon.
--
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 13:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5792/13/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:
Line 40
> This mechanism exists so that we can configure Impala's logging in one plac
> I don't think we should introduce the burden of having them configured separately - is that what this change now implies?
No. the users can still configure it from a single place
> Is trace logging off by default?
TRACE logging is ON/OFF depending on how users configure GLOG (ex: JniCatalog/JniFrontend still call GlogAppender.Install() based on --v).
> We should expect that 95% of deployments won't use this feature for fine-grained control, so the out-of-the-box behaviour has to be good enough for them.
Yes. Like I mentioned above, this patch doesn't break any existing way of configuring logging. We still configure it using the central GLOG config.
I think there is some disconnect in my understanding here. This patch doesn't break any existing ways per my understanding. May be we can chat offline to clear it out. Apologies for the back and forth.
--
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: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 6:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/5792/6/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:
Line 83: static const string log_inputs[] = {"get_java_log", "set_java_log", "reset_java_log",
Sorry, I didn't realize the log_inputs and the display_members were different strings, it was easier to read the way it was before.
http://gerrit.cloudera.org:8080/#/c/5792/6/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:
Line 43: """ Helper method that polls a given url and asserts the return code is ok and
nit: remove space before "Helper"
Line 48: def test_log_level_callback(self):
test_log_level
Line 50: malformed inputs"""
Does not test that the log level modifications are actually in effect.
Line 59: # Set the log level of a class to TRACE anc confirm the setting is in place
typo: and
Line 67: self.get_and_check_status(get_loglevel_url, "TRACE")
also check a different class and make sure it's still at DEBUG
Line 75: self.get_and_check_status(get_loglevel_url, "DEBUG")
also add tests for malformed inputs, e.g., a class that does not exist, empty strings, garbage log levels, etc.
same for the glog tests
there are some tests below, but I think some cases are missing
--
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 11:
(16 comments)
http://gerrit.cloudera.org:8080/#/c/5792/7/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:
PS7, Line 225: alue output(result.c_str(), document->GetAl
> I thought about this, but from what I understand, mustache templates are lo
Mustache supports basic conditionals - you're using one already in the tempalte ({{^error}} - its contents are only evaluated if 'error' is not set). So instead of building the string to display here, set 'set_java_log_result' and in the template, do something like:
{{?set_java_log_result}}
Effective log level: {{set_java_log_result}}
{{/set_java_log_result}}
http://gerrit.cloudera.org:8080/#/c/5792/11/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:
PS11, Line 81: FLAGS_v_default
I don't think this is the default, but the originally set value (because the default can be overridden from the command line).
Maybe FLAGS_v_original_value ?
PS11, Line 96: InitDynamicLoggingSupport
anon namespace
PS11, Line 217: const Status& status
What information does Status::GetDetail() give you that just passing in the error string does not?
PS11, Line 241: nullptr
Although I see that this construction compiles in Impala, I can't make it compile in other projects. My guess is that some compiler flag allows nullptr to be coerced to string via string(char*).
A std::string is an object, so can't be directly compared to nullptr, I think what you mean here and elsewhere is to check of result is empty, so please do that directly: if (result.empty()) { ...
PS11, Line 251: log_setclass
just 'classname' or similar. 'log_setclass' is hard to understand.
PS11, Line 252: logging_level
just 'level'? Prefer conciseness where you can reasonably do so without compromising readability.
Line 255: return;
No SetErrorMessage()?
PS11, Line 267: null
empty
PS11, Line 301: ResetGlogLevelCallback
if these are not used outside of this module, please put them in the anonymous namespace so they don't pollute the impala namespace.
http://gerrit.cloudera.org:8080/#/c/5792/11/be/src/util/logging-support.h
File be/src/util/logging-support.h:
PS11, Line 22: #include "util/webserver.h"
not needed: just forward declare the class.
Line 27: /// Registers the required native logging functions with JNI. This allows
While you're here - re-wrap to 90 chars.
PS11, Line 39: init_log4j
register_log4j_handlers? 'init_log4j' makes it sound like the log4j subsystem will be initialized.
Line 39: void RegisterLogLevelCallbacks(const string& url, Webserver* webserver, bool init_log4j);
std::
http://gerrit.cloudera.org:8080/#/c/5792/11/common/thrift/Logging.thrift
File common/thrift/Logging.thrift:
PS11, Line 34: // Helper structs for GetJavaLogLevel(), SetJavaLogLevel() and
: // ResetJavaLogLevel() methods
This doesn't really help me - comment should say what they're used for.
http://gerrit.cloudera.org:8080/#/c/5792/7/www/log_level.tmpl
File www/log_level.tmpl:
PS7, Line 76: <table>
> I agree it is hard to maintain. I'm not an expert on this. Do you have any
Look at the other templates to see how they do layout?
If you google for 'bootstrap form layout', you should find the bootstrap documentation etc. This tutorial seems ok: http://www.tutorialrepublic.com/twitter-bootstrap-tutorial/bootstrap-forms.php
--
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: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 15:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/311/
--
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: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 16: Verified+1
--
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: 16
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
IMPALA-4822: Implement dynamic log level changes
Very often we have to change the logging levels
of Impalads and Catalog server for debugging purposes.
Currently, there is no way to do that without a restart
of the daemons, which is not a viable option in production
deployments.
This patch addresses this supportability gap by exposing
the ability to set dynamic logging levels using a simple
web endpoint on Impalad/Catalog/Statestore web pages.
This includes setting VLOG levels (equivalent to --v flag)
as well as setting log4j levels on the Frontend and the
Catalog JVMs.
Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Reviewed-on: http://gerrit.cloudera.org:8080/5792
Reviewed-by: Bharath Vissapragada <bh...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/catalog/catalog-server.cc
M be/src/service/impala-http-handler.cc
M be/src/statestore/statestore.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M be/src/util/logging-support.h
M common/thrift/Logging.thrift
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/util/GlogAppender.java
M tests/webserver/test_web_pages.py
A www/log_level.tmpl
12 files changed, 469 insertions(+), 11 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Bharath Vissapragada: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/5792
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 17
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#12).
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
IMPALA-4822: Implement dynamic log level changes
Very often we have to change the logging levels
of Impalads and Catalog server for debugging purposes.
Currently, there is no way to do that without a restart
of the daemons, which is not a viable option in production
deployments.
This patch addresses this supportability gap by exposing
the ability to set dynamic logging levels using a simple
web endpoint on Impalad and Catalog web pages.
This includes setting VLOG levels (equivalent to --v flag)
as well as setting log4j levels on the Frontend and the
Catalog JVMs.
Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
---
M be/src/catalog/catalog-server.cc
M be/src/service/impala-http-handler.cc
M be/src/statestore/statestore.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M be/src/util/logging-support.h
M common/thrift/Logging.thrift
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/util/GlogAppender.java
M tests/webserver/test_web_pages.py
A www/log_level.tmpl
12 files changed, 475 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/5792/12
--
To view, visit http://gerrit.cloudera.org:8080/5792
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 12:
> I'm not sure the extra complexity of the UI is worth it.
Without the UI, we'd have to remember how to construct the URLs and what the arguments are. The only complexity the UI adds is log_level.tmpl which handles presentation of the returned data. Everything else would stay roughly the same if we had a command-line-only interface.
--
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: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 11:
Marcel, you can make changes like you described just with a URL, see test_web_pages.py which does just that. The UI is nice because then you also have a way of inspecting the state of the system. Allowing mutation without having a way to inspect the state seems bad for usability (and esp. if we expose it to users).
--
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: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5792/7/common/thrift/Logging.thrift
File common/thrift/Logging.thrift:
PS7, Line 45: TResetJavaLogLevelParams
> We already do that [1], just that we don't save them in the frontend anywhe
Seems easy enough to just keep them in the BackendConfig then
--
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 7:
(7 comments)
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 and set it in a new function InitDynamicLogLevels(). That function would remember the default glog level and also load the Java classes.
InitDynamicLogLevels() would be called inside InitCommonRuntime()
http://gerrit.cloudera.org:8080/#/c/5792/7/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:
Line 243: StringParser::StringToInt<int>(glog_level->second.c_str(),
use data() instead of c_str()
Line 247: Status s("Bad glog level input. Valid inputs are integers in [0-3] range.");
in the [0-3] range
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 test above
Line 83: set_glog_url =\
extra space
Line 103: set_glog_url =\
extra space
Line 116: set_glog_url =\
extra space
--
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#13).
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
IMPALA-4822: Implement dynamic log level changes
Very often we have to change the logging levels
of Impalads and Catalog server for debugging purposes.
Currently, there is no way to do that without a restart
of the daemons, which is not a viable option in production
deployments.
This patch addresses this supportability gap by exposing
the ability to set dynamic logging levels using a simple
web endpoint on Impalad/Catalog/Statestore web pages.
This includes setting VLOG levels (equivalent to --v flag)
as well as setting log4j levels on the Frontend and the
Catalog JVMs.
Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
---
M be/src/catalog/catalog-server.cc
M be/src/service/impala-http-handler.cc
M be/src/statestore/statestore.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M be/src/util/logging-support.h
M common/thrift/Logging.thrift
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/util/GlogAppender.java
M tests/webserver/test_web_pages.py
A www/log_level.tmpl
12 files changed, 473 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/5792/13
--
To view, visit http://gerrit.cloudera.org:8080/5792
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 13:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/5792/13/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:
Line 40
> What's the effect of removing these? Where is the VLOG masking done now?
These come in the way of dynamic log4j dynamic log level changes because the per class severity could be different from the global log level and hence passing the if() check. (severity comes from the logging event)
http://gerrit.cloudera.org:8080/#/c/5792/13/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:
PS13, Line 127: set_glog_url =\
: (self.SET_GLOG_LOGLEVEL_URL + "?glog=foo")
> nit: one line, no parentheses?
Done
http://gerrit.cloudera.org:8080/#/c/5792/13/www/log_level.tmpl
File www/log_level.tmpl:
PS13, Line 21: error
> It might make more sense to print the form if there was an error, so that t
Good point. Done.
PS13, Line 25: width: 600px;
> Why set the width? Does the page look bad if you let it figure out the widt
Yes, it basically takes the full page width and looks awkward. Anyway made it 30% rather than a specific number. Let me know if you disagree.
--
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: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 12:
@Henry/Alex/Marcel: Just checking to see if you have any further comments on this. Thanks for the reviews so far.
--
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: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4822: Dynamic log level changes to Catalog and Frontend JVMs
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5792/7/common/thrift/Logging.thrift
File common/thrift/Logging.thrift:
PS7, Line 45: TResetJavaLogLevelParams
> Seems easy enough to just keep them in the BackendConfig then
Yes, I'll do it then. Just wanted to confirm that is the ask here.
--
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 12:
> Marcel, you can make changes like you described just with a URL,
> see test_web_pages.py which does just that. The UI is nice because
> then you also have a way of inspecting the state of the system.
> Allowing mutation without having a way to inspect the state seems
> bad for usability (and esp. if we expose it to users).
I didn't suggest not allowing inspection of the existing log levels, which can also be done via a url.
I'm not sure the extra complexity of the UI is worth it.
--
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: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 15: Code-Review+2
Thanks Henry. Rebased and increased the page width to 50% for it look good on smaller resolutions. Carrying +2.
--
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: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 15:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/319/
--
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: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 11:
Alex beat me to it. Yes this patch does it exactly like the URLs you mentioned. The UI is more for visual feedback.
--
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: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 15:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/316/
--
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: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has uploaded a new patch set (#4).
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
IMPALA-4822: Implement dynamic log level changes
Very often we have to change the logging levels
of Impalads and Catalog server for debugging purposes.
Currently, there is no way to do that without a restart
of the daemons, which is not a viable option in production
deployments.
This patch addresses this supportability gap by exposing
the ability to set dynamic logging levels using a simple
web endpoint on Impalad and Catalog web pages.
This includes setting VLOG levels (equivalent to --v flag)
as well as setting log4j levels on the Frontend and the
Catalog JVMs.
Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
---
M be/src/catalog/catalog-server.cc
M be/src/service/impala-http-handler.cc
M be/src/util/backend-gflag-util.cc
M be/src/util/backend-gflag-util.h
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M be/src/util/logging-support.h
M common/thrift/Logging.thrift
M fe/src/main/java/org/apache/impala/util/GlogAppender.java
A www/log_level.tmpl
11 files changed, 345 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/5792/4
--
To view, visit http://gerrit.cloudera.org:8080/5792
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 7:
(16 comments)
http://gerrit.cloudera.org:8080/#/c/5792/7/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:
PS7, Line 225: result.insert(0, "Effective log level: ");
> Mustache supports basic conditionals - you're using one already in the temp
Oh makes sense. Thanks for explaining this.
http://gerrit.cloudera.org:8080/#/c/5792/11/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:
PS11, Line 81: ic jmethodID se
> I don't think this is the default, but the originally set value (because th
Done
PS11, Line 96: MethodDescriptor get_log_
> anon namespace
Ouch I thought I already put that. Got confused with the bracketing. Thanks for pointing that out.
PS11, Line 217: s_name(log_setclass-
> What information does Status::GetDetail() give you that just passing in the
Any errors in the codepaths of calling jni methods (JniUtil::CallJniMethod()).
PS11, Line 241: ::Parse
> Although I see that this construction compiles in Impala, I can't make it c
Done
PS11, Line 251:
> just 'classname' or similar. 'log_setclass' is hard to understand.
Done
PS11, Line 252: td::to_string
> just 'level'? Prefer conciseness where you can reasonably do so without com
Done
Line 255: } else if (args.find("reset_glog_log") != args.end()) {
> No SetErrorMessage()?
Done
PS11, Line 267:
> empty
Done
PS11, Line 301:
> if these are not used outside of this module, please put them in the anonym
Done
http://gerrit.cloudera.org:8080/#/c/5792/11/be/src/util/logging-support.h
File be/src/util/logging-support.h:
PS11, Line 22: #include "util/webserver.h"
> not needed: just forward declare the class.
Done
Line 27: struct UTF8;
> While you're here - re-wrap to 90 chars.
Done
Line 39:
> std::
Done
PS11, Line 39:
> register_log4j_handlers? 'init_log4j' makes it sound like the log4j subsyst
Done
http://gerrit.cloudera.org:8080/#/c/5792/11/common/thrift/Logging.thrift
File common/thrift/Logging.thrift:
PS11, Line 34: // Helper structs for GetJavaLogLevel(), SetJavaLogLevel() and
: // ResetJavaLogLevel() methods
> This doesn't really help me - comment should say what they're used for.
Done
http://gerrit.cloudera.org:8080/#/c/5792/7/www/log_level.tmpl
File www/log_level.tmpl:
PS7, Line 76: <table>
> Look at the other templates to see how they do layout?
Thanks for the pointer. Pretty helpful. Moved the code to bootstrap. Looks much better.
--
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 5:
(9 comments)
webserver-test.cc seems to be more unit-testing the WebServer class and I don't think we can instantiate the JNI and test the actual code paths. So, I added new e-e tests to the patch. Please let me know if you think we should add more variants.
http://gerrit.cloudera.org:8080/#/c/5792/5/be/src/util/backend-gflag-util.h
File be/src/util/backend-gflag-util.h:
Line 27: /// debugging, so we save the original value here incase we need to restore
> in case (two words)
Done
http://gerrit.cloudera.org:8080/#/c/5792/5/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:
Line 77: jclass log4j_logger_class_;
> no "_" suffix since not private class members; make these static
Done
Line 196: if (args.find("get_java_log") != args.end()) {
> make these strings constants like:
Done. Refactored it a little.
Line 212: logging_level == args.end() || logging_level->second == NULL) return;
> use {} for multi-line ifs
Done
http://gerrit.cloudera.org:8080/#/c/5792/5/be/src/util/logging-support.h
File be/src/util/logging-support.h:
Line 25: namespace rapidjson {
> Ok to use the include if you prefer that solution. Either wfm.
I'm fine with this too.
Line 43: /// the Java log4j log messages to be forwarded to Glog.
> update comment to reflect that it loads JNI classes/methods to support dyna
Done
http://gerrit.cloudera.org:8080/#/c/5792/5/common/thrift/Logging.thrift
File common/thrift/Logging.thrift:
Line 34: // Helper structs for GetLogLevel(), SetLogLevel() and ResetLogLevel()
> update
Done
http://gerrit.cloudera.org:8080/#/c/5792/5/www/log_level.tmpl
File www/log_level.tmpl:
Line 27: Log: <input type="text" name="getclass" placeholder="e.g. org.apache.foo">
> Pretty sure org.apache.foo won't be clear to users. I suggest using a full,
Done
Line 41: Log: <input type="text" name="setclass" placeholder="e.g. org.apache.foo">
> use a real class name
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 16: Code-Review+2
Rebased on top of https://gerrit.cloudera.org/#/c/6264/. Carrying Henry's +2.
--
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: 16
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5792/7/common/thrift/Logging.thrift
File common/thrift/Logging.thrift:
PS7, Line 45: TResetJavaLogLevelParams
> I would also settle for comments and / or a better method name if this is t
Henry, I agree with your original comment that passing in these flags for every invocation is kind of confusing. To me it seems clearer to keep these defaults in the BackendConfig
--
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 12:
(16 comments)
Fxed a minor bug in the web UI where the setting for java log level changes were showing up on the statestore web UI which doesn't have a JVM.
http://gerrit.cloudera.org:8080/#/c/5792/12/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:
Line 209:
> remove blank line
Done
http://gerrit.cloudera.org:8080/#/c/5792/12/be/src/util/jni-util.h
File be/src/util/jni-util.h:
PS12, Line 230: the above
> explicitly say LoadJniMethod. Otherwise someone might put a method between
haha, good point. Changed it.
PS12, Line 234: It seems these
: /// must be defined in the header to compile properly.
> can you remove this line (I probably wrote it...)? templates need to be in
Done
http://gerrit.cloudera.org:8080/#/c/5792/12/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:
PS12, Line 129: Status SetJavaLogLevel(const TSetJavaLogLevelParams& params, string* result) {
: RETURN_IF_ERROR(
: JniUtil::CallJniMethod(log4j_logger_class_, set_log_level_method, params, result));
: return Status::OK();
: }
> Is this called in more than one place? Otherwise, let's just inline it into
Yea that is the only place. I've written this way think it would be more readable but I agree it looks redundant.
PS12, Line 143: const Status& status
> What I mean was: rather than constructing a status every time you want to c
Ah sorry misunderstood. Made the change.
PS12, Line 149: SetDisplayResult
> how about 'AddDocumentMember()" ?
That sounds better. Done.
PS12, Line 158: log_getclass->second == nullptr
> Please remove the comparisons to nullptr for strings, and replace with .emp
Done
PS12, Line 215: return
> Maybe make this case an error as well so that user knows what's missing.
Done
PS12, Line 223: std::
> remove
Done
PS12, Line 229: std::
> remove
Done
Line 307: void RegisterLogLevelCallbacks(const string& url, Webserver* webserver, bool register_log4j_handlers) {
> long line (consider using git-clang-format)
Done.
PS12, Line 310: set_glog_log
> sorry to nitpick - but set_glog_log seems repetitive. Why not set_glog_leve
Valid point. Changed.
http://gerrit.cloudera.org:8080/#/c/5792/12/be/src/util/logging-support.h
File be/src/util/logging-support.h:
PS12, Line 39: init_log4j
> update
Done
PS12, Line 40: const std::string& url
> is this needed, or is it the same for all callers?
Removed, its the same for all callers.
http://gerrit.cloudera.org:8080/#/c/5792/12/fe/src/main/java/org/apache/impala/util/GlogAppender.java
File fe/src/main/java/org/apache/impala/util/GlogAppender.java:
Line 46:
> remove blank line
Done
http://gerrit.cloudera.org:8080/#/c/5792/12/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:
PS12, Line 86: \
> remove where you have used parentheses
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: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 15: Verified-1
Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/310/
--
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: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4822: Dynamic log level changes to Catalog and Frontend JVMs
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Hello Henry Robinson,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5792
to look at the new patch set (#14).
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
IMPALA-4822: Implement dynamic log level changes
Very often we have to change the logging levels
of Impalads and Catalog server for debugging purposes.
Currently, there is no way to do that without a restart
of the daemons, which is not a viable option in production
deployments.
This patch addresses this supportability gap by exposing
the ability to set dynamic logging levels using a simple
web endpoint on Impalad/Catalog/Statestore web pages.
This includes setting VLOG levels (equivalent to --v flag)
as well as setting log4j levels on the Frontend and the
Catalog JVMs.
Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
---
M be/src/catalog/catalog-server.cc
M be/src/service/impala-http-handler.cc
M be/src/statestore/statestore.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M be/src/util/logging-support.h
M common/thrift/Logging.thrift
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/util/GlogAppender.java
M tests/webserver/test_web_pages.py
A www/log_level.tmpl
12 files changed, 469 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/5792/14
--
To view, visit http://gerrit.cloudera.org:8080/5792
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 1:
Rather than using a web page for making those changes, why not simply do it through a URL? (That's how it was handled at Google.)
something like http://.../logv?level=<level> or .../logv_module?<module>=<level>
(I don't care about the actual syntax, that's just an example.)
--
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-4822: Implement dynamic log level changes
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5792/7/common/thrift/Logging.thrift
File common/thrift/Logging.thrift:
PS7, Line 45: TResetJavaLogLevelParams
> We could pass them along with all the other BE gflags in GetThriftBackendGf
We already do that [1], just that we don't save them in the frontend anywhere to be used later.
[1] https://github.com/cloudera/Impala/blob/cdh5-trunk/be/src/util/backend-gflag-util.cc#L67-L68
--
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 <bh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: Yes