Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Log Throttler ...................................................................... Patch Set 1: (6 comments) Just passing through. 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/styleguide/javaguide.html Or at least match existing codebase style. 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 awkwardly IMO. It'd be a bit more self-documenting if it were named timestampSecsByMessage or messageToTimestampSecs or lastLoggedTimeSecsPerMessage or something? 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 lastLoggedTimestamp, it might be worth considering mapping some integer key instead, and exposing the keys publicly, e.g. ThrottlerLogUtil { public static final int APPLY_ON_CLOSED_KEY = 0; public static final int OTHER_THROTTLED_KEY = 1; public void info(long seconds, int key, String msg, ...) { if (processing(seconds, key)) { log.info(msg, ...); } } } and at call-sites: logThrottler.info(10, APPLY_ON_CLOSED_KEY, "message"); That would also allow the message to contain mutable messages, if we ever want to use this for that. What do you think? 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 somesuch? That way it's clear from the name what this is and how it's used. 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"? 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? -- 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 01:36:25 +0000 Gerrit-HasComments: Yes
