Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12323 )
Change subject: KUDU-2670: Part 1: Build ScanToken by KeyRange ...................................................................... Patch Set 24: (7 comments) Thanks for all the updates. It's looking really good. I have a few more small comments/questions. http://gerrit.cloudera.org:8080/#/c/12323/24/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/24/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1996 PS24, Line 1996: List<KeyRange> syncGetTableKeyRanges(KuduTable table, It's not common to have synchronous methods in the AsyncKuduClient. Given this is just a call to getTableKeyRanges with a following .join() call and it's only used once in KuduTable, maybe you could just call getTableKeyRanges(...).join() in KuduTable. http://gerrit.cloudera.org:8080/#/c/12323/24/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/24/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java@294 PS24, Line 294: private long splitSizeBytes = -1; Nit: define a constant for the special -1 value. http://gerrit.cloudera.org:8080/#/c/12323/24/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java@408 PS24, Line 408: List<KeyRange> newKeyRanges = table.getTabletsKeyRanges( Should we still use the old style when splitSizeBytes = -1? http://gerrit.cloudera.org:8080/#/c/12323/24/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java: http://gerrit.cloudera.org:8080/#/c/12323/24/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java@245 PS24, Line 245: public List<KeyRange> getTabletsKeyRanges(byte[] primaryKeyStart, I am not sure this should live here. Or at least is shouldn't be public given it returns a private KeyRange. You could call client.getTableKeyRanges(...).join() in the ScanToken code. http://gerrit.cloudera.org:8080/#/c/12323/24/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/24/java/kudu-client/src/main/java/org/apache/kudu/client/SplitKeyRangeRequest.java@37 PS24, Line 37: Perhaps add the constant for the -1 value mentioned in KuduScanToken here. http://gerrit.cloudera.org:8080/#/c/12323/24/java/kudu-client/src/main/java/org/apache/kudu/client/SplitKeyRangeRequest.java@109 PS24, Line 109: SplitKeyRangeResponse response = new SplitKeyRangeResponse( See my comment on SplitKeyRangeResponse. http://gerrit.cloudera.org:8080/#/c/12323/24/java/kudu-client/src/main/java/org/apache/kudu/client/SplitKeyRangeResponse.java File java/kudu-client/src/main/java/org/apache/kudu/client/SplitKeyRangeResponse.java: http://gerrit.cloudera.org:8080/#/c/12323/24/java/kudu-client/src/main/java/org/apache/kudu/client/SplitKeyRangeResponse.java@33 PS24, Line 33: SplitKeyRangeResponse(long elapsedMillis, String tsUUID, List<KeyRangePB> keyRanges) { Can we translate KeyRangePB to KeyRange before passing into this constructor? This is usually done via methods added to ProtobufHelper and placed in a deserialize method on the request. See GetTableSchemaRequest.deserialize for an example. -- 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: 24 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: Thu, 16 May 2019 14:06:51 +0000 Gerrit-HasComments: Yes
