You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mahesh Reddy (Code Review)" <ge...@cloudera.org> on 2020/08/29 01:17:02 UTC
[kudu-CR] KUDU-3012: Throttled log warning
Mahesh Reddy has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16386
Change subject: KUDU-3012: Throttled log warning
......................................................................
KUDU-3012: Throttled log warning
RateLimitedLogger sets limit on amount of messages that can be logged
over a parameterized time period. Suppresses any additional messages.
Implementation comes from https://github.com/Swrve/rate-limited-logger.
Change-Id: Iedaf46276cb2c67f4c2436487200bea3d43d736f
---
M java/kudu-client/build.gradle
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
A java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottlerUtil.java
A java/kudu-client/src/test/java/org/apache/kudu/util/MockLogger.java
A java/kudu-client/src/test/java/org/apache/kudu/util/TestRateLimitedLog.java
5 files changed, 458 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/16386/1
--
To view, visit http://gerrit.cloudera.org:8080/16386
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iedaf46276cb2c67f4c2436487200bea3d43d736f
Gerrit-Change-Number: 16386
Gerrit-PatchSet: 1
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] KUDU-3012: Throttled log warning
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16386 )
Change subject: KUDU-3012: Throttled log warning
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/16386/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16386/1//COMMIT_MSG@11
PS1, Line 11: Implementation comes from https://github.com/Swrve/rate-limited-logger
I am not sure if this small piece of functionality is worth and external dependency.
When boiled down to what we need implementing it ourselves could be small and easy to extend as needed. IIUC the implementation essentially keeps a map of unique log messages and skips logging them if they have been logged recently. A super simple example implementation can be seen here too: https://stackoverflow.com/questions/41465002/can-i-make-log4j-suppress-logging-specific-messages-for-a-time
http://gerrit.cloudera.org:8080/#/c/16386/1/java/kudu-client/build.gradle
File java/kudu-client/build.gradle:
http://gerrit.cloudera.org:8080/#/c/16386/1/java/kudu-client/build.gradle@36
PS1, Line 36: compile group: 'com.swrve', name: 'rate-limited-logger', version: '2.0.0'
Can you move this into dependencies.gradle like the other dependencies?
http://gerrit.cloudera.org:8080/#/c/16386/1/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottlerUtil.java
File java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottlerUtil.java:
http://gerrit.cloudera.org:8080/#/c/16386/1/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottlerUtil.java@27
PS1, Line 27: public static final int MAXRATE = 60;
: public static final int DURATION = 60;
> Nit: Not sure whether System properties are used in kudu-java, to make defa
I don't think we need to make this configurable as the system level. For any given log line it's probably good enough to use our judgement picking the throttle parameters.
http://gerrit.cloudera.org:8080/#/c/16386/1/java/kudu-client/src/test/java/org/apache/kudu/util/TestRateLimitedLog.java
File java/kudu-client/src/test/java/org/apache/kudu/util/TestRateLimitedLog.java:
http://gerrit.cloudera.org:8080/#/c/16386/1/java/kudu-client/src/test/java/org/apache/kudu/util/TestRateLimitedLog.java@28
PS1, Line 28: MockLogger mockLogger = new MockLogger();
Instead of adding MockLogger would it make sense to use the existing CapturingLogAppender?: https://github.com/apache/kudu/blob/master/java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingLogAppender.java
--
To view, visit http://gerrit.cloudera.org:8080/16386
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iedaf46276cb2c67f4c2436487200bea3d43d736f
Gerrit-Change-Number: 16386
Gerrit-PatchSet: 1
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 31 Aug 2020 13:19:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3012: Throttled log warning
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has abandoned this change. ( http://gerrit.cloudera.org:8080/16386 )
Change subject: KUDU-3012: Throttled log warning
......................................................................
Abandoned
new implementation of throttler: https://gerrit.cloudera.org/c/16400/
--
To view, visit http://gerrit.cloudera.org:8080/16386
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: Iedaf46276cb2c67f4c2436487200bea3d43d736f
Gerrit-Change-Number: 16386
Gerrit-PatchSet: 1
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3012: Throttled log warning
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16386 )
Change subject: KUDU-3012: Throttled log warning
......................................................................
Patch Set 1:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/16386/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java:
http://gerrit.cloudera.org:8080/#/c/16386/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@51
PS1, Line 51:
Nit: Remove the extra line.
http://gerrit.cloudera.org:8080/#/c/16386/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@119
PS1, Line 119:
This line can be removed.
http://gerrit.cloudera.org:8080/#/c/16386/1/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottlerUtil.java
File java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottlerUtil.java:
http://gerrit.cloudera.org:8080/#/c/16386/1/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottlerUtil.java@27
PS1, Line 27: MAXRATE
nit: _ between MAX and RATE
http://gerrit.cloudera.org:8080/#/c/16386/1/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottlerUtil.java@27
PS1, Line 27: public static final int MAXRATE = 60;
: public static final int DURATION = 60;
Nit: Not sure whether System properties are used in kudu-java, to make default values configurable, you could use Integer.getInteger() https://docs.oracle.com/javase/8/docs/api/java/lang/Integer.html#getInteger-java.lang.String-int-
I couldn't find usage of gflag equivalent in kudu-java.
http://gerrit.cloudera.org:8080/#/c/16386/1/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottlerUtil.java@36
PS1, Line 36: final RateLimitedLog THROTTLED_LOG = RateLimitedLog
: .withRateLimit(log)
: .maxRate(MAXRATE).every(Duration.ofSeconds(DURATION))
: .build();
: return THROTTLED_LOG;
It'd be better to call the generic getRateLimitedLogger() method from here instead.
http://gerrit.cloudera.org:8080/#/c/16386/1/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottlerUtil.java@50
PS1, Line 50: int period
Nit: could consider taking Duration argument instead.
http://gerrit.cloudera.org:8080/#/c/16386/1/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottlerUtil.java@51
PS1, Line 51: final RateLimitedLog THROTTLED_LOG1 = RateLimitedLog
: .withRateLimit(log)
: .maxRate(max).every(Duration.ofSeconds(period))
: .build();
: return THROTTLED_LOG1;
Nit: local variable seems unnecessary, could simply return with a single line.
http://gerrit.cloudera.org:8080/#/c/16386/1/java/kudu-client/src/test/java/org/apache/kudu/util/TestRateLimitedLog.java
File java/kudu-client/src/test/java/org/apache/kudu/util/TestRateLimitedLog.java:
http://gerrit.cloudera.org:8080/#/c/16386/1/java/kudu-client/src/test/java/org/apache/kudu/util/TestRateLimitedLog.java@25
PS1, Line 25:
Nit: Blank line
http://gerrit.cloudera.org:8080/#/c/16386/1/java/kudu-client/src/test/java/org/apache/kudu/util/TestRateLimitedLog.java@30
PS1, Line 30: 5
For test purposes, this duration can be lower like 2 secs.
http://gerrit.cloudera.org:8080/#/c/16386/1/java/kudu-client/src/test/java/org/apache/kudu/util/TestRateLimitedLog.java@34
PS1, Line 34: assertEquals(mockLogger.infoMessageCount, 25); // first 25 logging messages
Convention is expected value as the first arg.
--
To view, visit http://gerrit.cloudera.org:8080/16386
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iedaf46276cb2c67f4c2436487200bea3d43d736f
Gerrit-Change-Number: 16386
Gerrit-PatchSet: 1
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 29 Aug 2020 03:39:42 +0000
Gerrit-HasComments: Yes