Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16400 )
Change subject: KUDU-3012: Log Throttler ...................................................................... Patch Set 1: (1 comment) 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@63 PS1, Line 63: processing(seconds, msg) > Performance is also an issue. Hashing a string is way more expensive than h Among the throttle log implementations I've used in the past, don't recall seeing a separate unique key being supplied. It's like offloading an implementation detail to the callers and one more thing to think about. As a caller, I just want the message to be throttled. So that's the argument in favor of not adding another parameter for the callers. If performance of throttle log becomes an issue then adding separate key would start making sense. Till then I don't see a need for it. Re: Storing hash code v/s storing string as key. Java Hashmap implementation will compute the hash code of the string key anyways, so computationally using string v/s hash code as key would be the same. Though storing hashcode would help from memory perspective. -- 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 21:35:18 +0000 Gerrit-HasComments: Yes
