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 <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sat, 29 Aug 2020 03:39:42 +0000 Gerrit-HasComments: Yes
