Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Log Throttler ...................................................................... Patch Set 1: (15 comments) http://gerrit.cloudera.org:8080/#/c/16400/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16400/1//COMMIT_MSG@7 PS1, Line 7: Log Throttler > Nit: Add a Java Log Throttler. Done http://gerrit.cloudera.org:8080/#/c/16400/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/16400/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@118 PS1, Line 118: logThrottler > It's a static final object, so should be named in caps like THROTTLE_LOG Done http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java File java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java: PS1: > Could probably use some scrubbing to match the GSG https://google.github.io Done http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@28 PS1, Line 28: // Issue 1: store template message w/ or w/o parameters, if just template message then assuming failure is a : // parameter if something fails once then a second time but this time b/c of a different reason, second log message : // will be suppressed and we won't know that there's a second cause of this failure : // : // Issue 2: Use log.info(msg, arguments) for everything or check for length so Object[] for arguments wont be : // created before method is invoked. From the Logger Interface regarding this issue: : // However, this variant incurs the hidden (and relatively small) cost of creating an <code>Object[]</code> before : // invoking the method, even if this logger is disabled for INFO. : // : // Note for Issue 2: Only using INFO level as an example, applies to other Levels too > These lines look long. Line length needs to be < 100. Done http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@50 PS1, Line 50: static > As per the current implementation, I don't think this should be static whic if map isnt static then size would be less of an issue, one map per class essentially http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@50 PS1, Line 50: timestamps > nit: this isn't just the timestamps, so its call-sites read somewhat awkwar Done http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@50 PS1, Line 50: final > Values are added to this map, so this can't be final. Done http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@52 PS1, Line 52: > Nit: Unnecessary space. Done http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@62 PS1, Line 62: long seconds > How about using Duration as argument for all variants so that callers don't if user supplies a unit less than seconds then when it's converted to seconds in this class there will be a rounding error, of course we can use the smallest time unit available (nanoseconds), if anyone else has thoughts please chime in. http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@63 PS1, Line 63: processing(seconds, msg) > Depending on how pervasive this becomes, rather than mapping the message to If memory is a concern I can map the hashCode of the String instead http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@119 PS1, Line 119: @param duration number of seconds between each desired log message > nit: How about calling this something like throttlingIntervalSecs or somesu Done http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@123 PS1, Line 123: message > Like other functions, msg should be sufficient. Done http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@123 PS1, Line 123: processing > Choose a better name as "processing" doesn't convey much, like shouldLog()? Done http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@124 PS1, Line 124: current > nit: maybe "nowSecs"? Done http://gerrit.cloudera.org:8080/#/c/16400/1/java/kudu-client/src/main/java/org/apache/kudu/util/ThrottlerLogUtil.java@125 PS1, Line 125: // probably can just use duration instead of gap > What is is this conversion doing? Isn't 'time' already TimeUnit.SECONDS? Done -- To view, visit http://gerrit.cloudera.org:8080/16400 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia2089b6fc905a5b54d664b7200060cabb965f40f Gerrit-Change-Number: 16400 Gerrit-PatchSet: 1 Gerrit-Owner: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Comment-Date: Wed, 02 Sep 2020 18:45:34 +0000 Gerrit-HasComments: Yes
