Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Add a Log Throttler ...................................................................... Patch Set 4: (6 comments) http://gerrit.cloudera.org:8080/#/c/16400/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16400/4//COMMIT_MSG@9 PS4, Line 9: LogThrottler uses an instance of Logger This is part of implementation detail and unnecessary. Better to mention that this change implements a simple log throttler that throttles messages by logging at most one log message per specified time duration. http://gerrit.cloudera.org:8080/#/c/16400/4//COMMIT_MSG@11 PS4, Line 11: message. message parameters, right? http://gerrit.cloudera.org:8080/#/c/16400/4/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/4/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@118 PS4, Line 118: final We want a single instance per class, so this should be static. If it's static final then it should be named in CAPS. http://gerrit.cloudera.org:8080/#/c/16400/4/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@554 PS4, Line 554: 10 I think 60 secs would be better. http://gerrit.cloudera.org:8080/#/c/16400/4/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java File java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java: http://gerrit.cloudera.org:8080/#/c/16400/4/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java@42 PS4, Line 42: private long lastLoggedTime = 0; Nit: If time were to start from epoch then the first log message will be throttled :) One option would be to initialize with -1 and use that special value in shouldLog(). http://gerrit.cloudera.org:8080/#/c/16400/4/java/kudu-client/src/main/java/org/apache/kudu/util/LogThrottler.java@49 PS4, Line 49: * Calls shouldLog method, logs message at trace level if method true, does nothing if false Java doc comments are meant for the users and there is no need to mention private function names. Same for methods below. Something like... "Throttles the log trace message 'msg' if the last message was logged less than 'seconds' ago." -- 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: 4 Gerrit-Owner: Mahesh Reddy <mre...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Bankim Bhavsar <ban...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <mre...@cloudera.com> Gerrit-Comment-Date: Thu, 03 Sep 2020 22:48:13 +0000 Gerrit-HasComments: Yes
