Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11333 )
Change subject: [KUDU-2521] Java Implementation for BloomFilter ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/11333/1/java/kudu-client/src/main/java/org/apache/kudu/util/BloomFilter.java File java/kudu-client/src/main/java/org/apache/kudu/util/BloomFilter.java: http://gerrit.cloudera.org:8080/#/c/11333/1/java/kudu-client/src/main/java/org/apache/kudu/util/BloomFilter.java@31 PS1, Line 31: > Shall we start the review at this Change-ID ? Typically you can just rebase and keep pushing to the same change-id / gerrit patchset, but in this case we can just continue the review here. http://gerrit.cloudera.org:8080/#/c/11333/1/java/kudu-client/src/main/java/org/apache/kudu/util/BloomFilter.java@109 PS1, Line 109: public void put(Long data) { should this be 'long'? http://gerrit.cloudera.org:8080/#/c/11333/1/java/kudu-client/src/main/java/org/apache/kudu/util/BloomFilter.java@149 PS1, Line 149: public static interface HashFunction { Exposing this abstraction as an enum might be the better way to go, since it's a 'closed' set of options. I think a user could technically implement this interface for their custom hash function, which would work until the bloom filter was sent to the server, at which point the server would have no idea what to do with it. An enum better models the 'one of a few options, but not open to extension' idea. http://gerrit.cloudera.org:8080/#/c/11333/1/java/kudu-client/src/main/java/org/apache/kudu/util/BloomFilter.java@288 PS1, Line 288: private static class BloomKeyProbe { Is the key probe useful? As I understand it it's used in the server as an optimization when looking up whether a key is in multiple bloom filters, but since we don't need such optimized lookups in this Java implementation it could potentially be removed? Then you could expose mayContains for specific types instead of having to do type dispatch, and the number of methods will be about the same. -- To view, visit http://gerrit.cloudera.org:8080/11333 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32673c008f9958088d281c2c198c543bfea2eb8e Gerrit-Change-Number: 11333 Gerrit-PatchSet: 1 Gerrit-Owner: [email protected] Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Comment-Date: Wed, 29 Aug 2018 22:35:33 +0000 Gerrit-HasComments: Yes
