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 <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 31 Aug 2020 13:19:50 +0000 Gerrit-HasComments: Yes
