You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2018/01/08 19:10:32 UTC
[kudu-CR] KUDU-2236: add debug logging where appropriate
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8961
Change subject: KUDU-2236: add debug logging where appropriate
......................................................................
KUDU-2236: add debug logging where appropriate
The previous fixes for KUDU-2236 adjusted the test to ensure its success
when they should have instead addressed the regression in logging
behavior. I.e. testCloseShortlyAfterOpen() should not catch any scary
log messages because it itself isn't doing anything scary.
This patch addresses the fact that exception messages are spewed even
when we might expect them because a connection has been explicitly
closed. In such cases, DEBUG-level messages will be logged instead of
INFO- or WARN-level messages.
CapturingLogAppenders can now specify what log levels to read from, and
testCloseShortlyAfterOpen will now parse INFO-level messages or higher
(i.e. not DEBUG-level).
Change-Id: I780df2eeb51a14b65dd4283dfe9d4d6daf909661
---
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/util/CapturingLogAppender.java
4 files changed, 30 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/8961/1
--
To view, visit http://gerrit.cloudera.org:8080/8961
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I780df2eeb51a14b65dd4283dfe9d4d6daf909661
Gerrit-Change-Number: 8961
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] KUDU-2236: use debug logging where appropriate
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8961 )
Change subject: KUDU-2236: use debug logging where appropriate
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/8961/2/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java:
http://gerrit.cloudera.org:8080/#/c/8961/2/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java@a347
PS2, Line 347:
does this already get logged at debug level elsewhere?
http://gerrit.cloudera.org:8080/#/c/8961/2/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
File java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java:
http://gerrit.cloudera.org:8080/#/c/8961/2/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java@688
PS2, Line 688: LOG.warn(msg);
does this need to be warned even in the non-explicit disconnect case? would it already have been logged elsewhere? seems like this is a net new log added by this patch?
http://gerrit.cloudera.org:8080/#/c/8961/2/java/kudu-client/src/test/java/org/apache/kudu/util/CapturingLogAppender.java
File java/kudu-client/src/test/java/org/apache/kudu/util/CapturingLogAppender.java:
http://gerrit.cloudera.org:8080/#/c/8961/2/java/kudu-client/src/test/java/org/apache/kudu/util/CapturingLogAppender.java@60
PS2, Line 60: * Sets the Log4j log level to read log messages from.
is this inclusive? perhaps rename to 'setMinimumLevel' or somesuch to indicate that it's this level and above?
http://gerrit.cloudera.org:8080/#/c/8961/2/java/kudu-client/src/test/java/org/apache/kudu/util/CapturingLogAppender.java@64
PS2, Line 64: logger.setLevel(level);
does this have the side effect of actually disabling debug logs while the capturing appender is active? Also it doesn't seem to restore it afterwards?
Perhaps instead of changing the underlying logger level we should just be filtering in the append() call?
--
To view, visit http://gerrit.cloudera.org:8080/8961
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I780df2eeb51a14b65dd4283dfe9d4d6daf909661
Gerrit-Change-Number: 8961
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 11 Jan 2018 02:11:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2236: use debug logging where appropriate
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has abandoned this change. ( http://gerrit.cloudera.org:8080/8961 )
Change subject: KUDU-2236: use debug logging where appropriate
......................................................................
Abandoned
Seems to be fixed in https://github.com/apache/kudu/commit/ead756844ce9ada904fcc3666df25692f63e76b8
--
To view, visit http://gerrit.cloudera.org:8080/8961
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I780df2eeb51a14b65dd4283dfe9d4d6daf909661
Gerrit-Change-Number: 8961
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2236: use debug logging where appropriate
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8961
to look at the new patch set (#2).
Change subject: KUDU-2236: use debug logging where appropriate
......................................................................
KUDU-2236: use debug logging where appropriate
The previous fixes for KUDU-2236 adjusted the test to ensure its success
when they should have instead addressed the regression in logging
behavior. I.e. testCloseShortlyAfterOpen() should not catch any scary
log messages because it itself isn't doing anything scary.
This patch addresses the fact that exception messages are spewed even
when we might expect them because a connection has been explicitly
closed. In such cases, DEBUG-level messages will be logged instead of
INFO- or WARN-level messages.
CapturingLogAppenders can now specify what log levels to read from, and
testCloseShortlyAfterOpen will now parse INFO-level messages or higher
(i.e. not DEBUG-level).
Change-Id: I780df2eeb51a14b65dd4283dfe9d4d6daf909661
---
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/util/CapturingLogAppender.java
4 files changed, 30 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/8961/2
--
To view, visit http://gerrit.cloudera.org:8080/8961
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I780df2eeb51a14b65dd4283dfe9d4d6daf909661
Gerrit-Change-Number: 8961
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins