Yao Xu has posted comments on this change. ( http://gerrit.cloudera.org:8080/12323 )
Change subject: KUDU-2670: Part 1: Build ScanToken by KeyRange ...................................................................... Patch Set 10: (22 comments) Hi, guys. I refactored part of the code for this patch (unittest and Client API) and supplement this part of the code document. http://gerrit.cloudera.org:8080/#/c/12323/6/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/12323/6/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1204 PS6, Line 1204: if (tablet == null) { > Nit: Line too long. Done http://gerrit.cloudera.org:8080/#/c/12323/6/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1209 PS6, Line 1209: final ServerInfo info = tablet.getReplicaSelectedServerInfo(keepAliveRequest.getReplicaSelection(), > The wording here suggests that this value is advisory (i.e. the server migh Done http://gerrit.cloudera.org:8080/#/c/12323/6/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1211 PS6, Line 1211: if (info == null) { > Nit: "a {@code Deferred} object which yields the list of key ranges." Done http://gerrit.cloudera.org:8080/#/c/12323/6/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1213 PS6, Line 1213: } > Is there any use case for applications to call this API directly? If so, sh Done http://gerrit.cloudera.org:8080/#/c/12323/6/java/kudu-client/src/main/java/org/apache/kudu/client/KeyRange.java File java/kudu-client/src/main/java/org/apache/kudu/client/KeyRange.java: http://gerrit.cloudera.org:8080/#/c/12323/6/java/kudu-client/src/main/java/org/apache/kudu/client/KeyRange.java@23 PS6, Line 23: * Class used to represent primary key range in tablet. > Should document that startPrimaryKey and endPrimaryKey represent encoded pr Done http://gerrit.cloudera.org:8080/#/c/12323/6/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java: http://gerrit.cloudera.org:8080/#/c/12323/6/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java@304 PS6, Line 304: return this; > Again, careful when referencing "split tablet"; it makes it sound as if the Done http://gerrit.cloudera.org:8080/#/c/12323/6/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java@305 PS6, Line 305: } > If unset or <= 0 is more accurate I suppose. Done http://gerrit.cloudera.org:8080/#/c/12323/6/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java@408 PS6, Line 408: if (keyRanges.isEmpty()) { > Nit: wrap Done http://gerrit.cloudera.org:8080/#/c/12323/6/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java@428 PS6, Line 428: } > private Done http://gerrit.cloudera.org:8080/#/c/12323/6/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java@437 PS6, Line 437: } catch (Exception e) { > I think we need to use a DedlineTracker to be sure the timeout represents t Done http://gerrit.cloudera.org:8080/#/c/12323/6/java/kudu-client/src/main/java/org/apache/kudu/client/SplitKeyRangeRequest.java File java/kudu-client/src/main/java/org/apache/kudu/client/SplitKeyRangeRequest.java: http://gerrit.cloudera.org:8080/#/c/12323/6/java/kudu-client/src/main/java/org/apache/kudu/client/SplitKeyRangeRequest.java@33 PS6, Line 33: * RPC to split a tablet's primary key range into smaller ranges suitable for concurrent scanning. > One of the confusing aspects of this feature is that tablets aren't actuall Done http://gerrit.cloudera.org:8080/#/c/12323/6/java/kudu-client/src/main/java/org/apache/kudu/client/SplitKeyRangeRequest.java@36 PS6, Line 36: class SplitKeyRangeRequest extends KuduRpc<SplitKeyRangeResponse> { > I don't think this needs to be final since it's internal preventing extensi Done http://gerrit.cloudera.org:8080/#/c/12323/6/java/kudu-client/src/main/java/org/apache/kudu/client/SplitKeyRangeRequest.java@38 PS6, Line 38: private final byte[] startPrimaryKey; : private final byte[] endPrimaryKey; : private final byte[] partitionKey; : private final long splitSizeBytes; > Document the meaning of these fields. Done http://gerrit.cloudera.org:8080/#/c/12323/6/java/kudu-client/src/main/java/org/apache/kudu/client/SplitKeyRangeRequest.java@43 PS6, Line 43: /** > Nit: unnecessary empty line here. Done http://gerrit.cloudera.org:8080/#/c/12323/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestSplitKeyRangeRequest.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestSplitKeyRangeRequest.java: http://gerrit.cloudera.org:8080/#/c/12323/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestSplitKeyRangeRequest.java@19 PS6, Line 19: > I don't mind a test of the request/response. More coverage is a great thing Done http://gerrit.cloudera.org:8080/#/c/12323/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestSplitKeyRangeRequest.java@34 PS6, Line 34: > Why do these flags need to be set? Done http://gerrit.cloudera.org:8080/#/c/12323/6/java/kudu-client/src/test/java/org/apache/kudu/util/SplitTestUtil.java File java/kudu-client/src/test/java/org/apache/kudu/util/SplitTestUtil.java: PS6: > License header. Done http://gerrit.cloudera.org:8080/#/c/12323/6/java/kudu-client/src/test/java/org/apache/kudu/util/SplitTestUtil.java@16 PS6, Line 16: > Could this be moved into the ClientTestUtil class for things that are gener Done http://gerrit.cloudera.org:8080/#/c/12323/6/java/kudu-client/src/test/java/org/apache/kudu/util/SplitTestUtil.java@104 PS6, Line 104: > for (int i = 1; i < numbers.size() - 1; i += 2) { Done http://gerrit.cloudera.org:8080/#/c/12323/6/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala: PS6: > Could you write a spark test that uses this new feature? Done http://gerrit.cloudera.org:8080/#/c/12323/6/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala: http://gerrit.cloudera.org:8080/#/c/12323/6/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala@37 PS6, Line 37: * @param splitSizeBytes Sets the target number of bytes per spark task. If set, tablet's > Agreed with Grant, though we need to be careful when we talk about "splitti Done http://gerrit.cloudera.org:8080/#/c/12323/6/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala@37 PS6, Line 37: * @param splitSizeBytes Sets the target number of bytes per spark task. If set, tablet's > Perhaps make these docs a bit more spark centric. Something along the lines Done -- To view, visit http://gerrit.cloudera.org:8080/12323 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0502f5d64569e8b1d45e88de3cb36aa2e01234d0 Gerrit-Change-Number: 12323 Gerrit-PatchSet: 10 Gerrit-Owner: yangz <zhe...@gmail.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yao Xu <oclarms....@gmail.com> Gerrit-Reviewer: yangz <zhe...@gmail.com> Gerrit-Comment-Date: Sat, 23 Mar 2019 10:37:47 +0000 Gerrit-HasComments: Yes
