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 26: (6 comments) Thanks for review. 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: final byte[] endPrimaryKey, > 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. Ok, I deleted it. Btw, maybe syncLocateTable should also needs to be modified. :) 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 timeout; > Nit: define a constant for the special -1 value. Done 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> keyRanges = new ArrayList<>(); > Should we still use the old style when splitSizeBytes = -1? Yep, a scan token is created for each tablet to be scanned by default. 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. Sorry, I didn't consider the KuduTable API. I think it is better not to let the user directly KeyRange, so I deleted this interface. 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. I added this constant in KuduScanToken. Maybe add some doc for splitSizeBytes <= 0 in KeyRange and SplitKeyRangeRequest. 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. There is a problem here, KeyRange's constructor needs a LocatedTablet. But getting this value here is more difficult, I didn't think of a good way to do it. -- 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: 26 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: Fri, 17 May 2019 04:34:37 +0000 Gerrit-HasComments: Yes
