Grant Henke 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: (9 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 rathe I think it makes calling these methods a bit more of a pain plumbing the long deadline from the public methods as far down as possible is easier. 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: 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 Done 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" Done 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 oper I can change over to a builder pattern. 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 KuduE Done 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 Done 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 Done 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 I added a test to check for expected NonCoveredRangeExceptions. Beyond that a partitioner that fails cannot be built. 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 parti I primarily wrote the test this way to match the c++ tests and verify the Java implementation behaves the same way. We could in hindsight with knowledge of the hash function used, but I didn't want this test to do that because "knowing" where the partition lands is basically what the partitioner code does. My test would become a re-implementation of the partitioner in a way. -- 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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 06 Feb 2019 19:40:40 +0000 Gerrit-HasComments: Yes
