Adar Dembo 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: (15 comments) http://gerrit.cloudera.org:8080/#/c/12323/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12323/10//COMMIT_MSG@18 PS10, Line 18: by size. And generate Nit: "by size, and generate..." http://gerrit.cloudera.org:8080/#/c/12323/10/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/10/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1937 PS10, Line 1937: for Nit: replace with 'to' http://gerrit.cloudera.org:8080/#/c/12323/10/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1942 PS10, Line 1942: * start at the beginning Nit: replace with "start splitting at the beginning of the tablet" http://gerrit.cloudera.org:8080/#/c/12323/10/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1944 PS10, Line 1944: * get all the key ranges Nit: replace with "stop splitting at the end of the tablet" http://gerrit.cloudera.org:8080/#/c/12323/10/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1947 PS10, Line 1947: the size of key range Nit: "a key range" http://gerrit.cloudera.org:8080/#/c/12323/10/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1949 PS10, Line 1949: a master lookup Odd to see this here. Are you sure you didn't mean "RPC that prompted the split key range request"? http://gerrit.cloudera.org:8080/#/c/12323/10/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1974 PS10, Line 1974: * Get all or some key range for a given table. This may query the master multiple times if there : * are a lot of tablets. And query the tablet server where each tablet is located, split the : * [startPrimaryKey, endPrimaryKey) in the tablet into smaller ranges. Is this comment correct for this method? Seems like it was copied from getTableKeyRanges where it is appropriate. http://gerrit.cloudera.org:8080/#/c/12323/10/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1991 PS10, Line 1991: If unset or <= 0, the key range : * includes all the data of the Tablet. What's the use case for this? It means we're not calling split key range at all; couldn't the caller trivially construct the list of key ranges without our help? http://gerrit.cloudera.org:8080/#/c/12323/10/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2015 PS10, Line 2015: private Deferred<List<KeyRange>> loopGetTableKeyRanges(final KuduTable table, Doc this too. http://gerrit.cloudera.org:8080/#/c/12323/10/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2033 PS10, Line 2033: String tableId = table.getTableId(); Nit: just use table.getTableId() directly on L2041. http://gerrit.cloudera.org:8080/#/c/12323/10/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2049 PS10, Line 2049: KeyRange keyRange = new KeyRange(startPrimaryKey, endPartitionKey, -1); Shouldn't this use endPrimaryKey not endPartitionKey? Are we missing test coverage that would surfaced this error? http://gerrit.cloudera.org:8080/#/c/12323/10/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2159 PS10, Line 2159: Deferred<List<KeyRange>> getTableKeyRanges(final KuduTable table, These new methods duplicate a lot from locateTable and friends. Can we share code between them? http://gerrit.cloudera.org:8080/#/c/12323/10/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/10/java/kudu-client/src/main/java/org/apache/kudu/client/KeyRange.java@47 PS10, Line 47: * Gets the start primary key. You can omit this sentence for simple getters and just use the @return. http://gerrit.cloudera.org:8080/#/c/12323/10/java/kudu-client/src/main/java/org/apache/kudu/client/KeyRange.java@62 PS10, Line 62: /** : * Sets the tablet which the key range is in : * @param tablet : */ : public void setTablet(LocatedTablet tablet) { : this.tablet = tablet; : } Can we omit this and provide the tablet in the constructor? Seems like the reason this can't be in the constructor is the double duty that the class plays: 1. User-visible return value from the new public split key range methods. 2. Intermediate deserialized output stored in SplitKeyRangeResponse, where the tablet isn't yet known. Perhaps we can find a different way to handle #2 and use KeyRange only for #1? http://gerrit.cloudera.org:8080/#/c/12323/10/java/kudu-client/src/test/java/org/apache/kudu/client/TestSplitKeyRange.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestSplitKeyRange.java: http://gerrit.cloudera.org:8080/#/c/12323/10/java/kudu-client/src/test/java/org/apache/kudu/client/TestSplitKeyRange.java@26 PS10, Line 26: import org.junit.Assert; If you statically import the Assert functions you care about, you can make the test more terse below. -- 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: Sun, 24 Mar 2019 00:10:41 +0000 Gerrit-HasComments: Yes
