Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12275 )
Change subject: KUDU-2674: [java] Add a Java KuduPartitioner API ...................................................................... Patch Set 1: (10 comments) http://gerrit.cloudera.org:8080/#/c/12275/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java: http://gerrit.cloudera.org:8080/#/c/12275/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2167 PS1, Line 2167: long deadline) { Would the plumbing be a little cleaner if this were a DeadLineTracker rather than a raw deadline value? http://gerrit.cloudera.org:8080/#/c/12275/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduPartitioner.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduPartitioner.java: PS1: License header. http://gerrit.cloudera.org:8080/#/c/12275/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduPartitioner.java@17 PS1, Line 17: it will not reflect any metadata changes to the table that have occurred duplicated http://gerrit.cloudera.org:8080/#/c/12275/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduPartitioner.java@32 PS1, Line 32: they Nit: "the user" or "the client" http://gerrit.cloudera.org:8080/#/c/12275/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduPartitioner.java@37 PS1, Line 37: while (true) { I don't think this should be done here; we don't typically do blocking operations in constructors. http://gerrit.cloudera.org:8080/#/c/12275/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduPartitioner.java@47 PS1, Line 47: catch (KuduException ex) { : throw new IllegalStateException(ex); : } Let's do this in a different method where we could throw the original KuduException. http://gerrit.cloudera.org:8080/#/c/12275/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduPartitioner.java@76 PS1, Line 76: * @return The resulting partition index, or -1 if the row falls into a : * non-covered range. The result will be less than numPartitions(). Wouldn't it be more Java-rific to throw an exception if the row falls into a non-covered range? http://gerrit.cloudera.org:8080/#/c/12275/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduPartitioner.java@87 PS1, Line 87: if (entry == null) { : throw new IllegalArgumentException(); : } Given the presence of the sentinel, how is this case even possible? If it's not, I'd just combine the whole thing: return partitionByStartKey.floorEntry(partitionKey).getValue(); http://gerrit.cloudera.org:8080/#/c/12275/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduPartitioner.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduPartitioner.java: http://gerrit.cloudera.org:8080/#/c/12275/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduPartitioner.java@51 PS1, Line 51: public void testPartitioner() throws Exception { I think it'd also be good to test the partitioner in a situation where it's not possible to perform the partitioning (i.e. masters down or something). http://gerrit.cloudera.org:8080/#/c/12275/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduPartitioner.java@95 PS1, Line 95: : // We don't expect a completely even division of rows into partitions, but : // we should be within 10% of that. Couldn't we just calculate the exact number of rows to fall into each partition? There's no randomness here so the assertion fuzziness seems unwarranted. -- To view, visit http://gerrit.cloudera.org:8080/12275 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7a2d47aab5318c0b6d29a8cb2b073c05bc1b6478 Gerrit-Change-Number: 12275 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Sun, 27 Jan 2019 19:22:30 +0000 Gerrit-HasComments: Yes
