Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Log Throttler ...................................................................... Patch Set 1: (9 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. 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 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: 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. Java style checker would point them out. Same for bunch of lines below. 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 which means the same map will be used by all instances of throttled logger. Also should we consider limiting the size of the map to prevent it from growing too large? Currently once added the log entries are never removed. 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. 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. I'll let the style checker point of such issues. 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 have to do computation for minutes, milliseconds etc.? 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()? 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. -- 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: 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 01:01:09 +0000 Gerrit-HasComments: Yes
