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 20: (12 comments) Apologize for updating the patch late. I have checked the error case in the unit test, it should not be caused by this patch. 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..." Done 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: > Nit: replace with 'to' Done http://gerrit.cloudera.org:8080/#/c/12323/10/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1942 PS10, Line 1942: * @param startPrimaryKey start splitting at the beginning of the tablet > Nit: replace with "start splitting at the beginning of the tablet" Done http://gerrit.cloudera.org:8080/#/c/12323/10/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1944 PS10, Line 1944: * @param partitionKey the partition key of the tablet to find > Nit: replace with "stop splitting at the end of the tablet" Done http://gerrit.cloudera.org:8080/#/c/12323/10/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1947 PS10, Line 1947: > Nit: "a key range" Done http://gerrit.cloudera.org:8080/#/c/12323/10/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1949 PS10, Line 1949: ess > Odd to see this here. Are you sure you didn't mean "RPC that prompted the s Done http://gerrit.cloudera.org:8080/#/c/12323/10/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1974 PS10, Line 1974: * are a lot of tablets, and query each tablet to split the tablet's primary key range into : * smaller ranges. This doesn't change the layout of the tablet. : * This method blocks until it gets all the key ranges. > Is this comment correct for this method? Seems like it was copied > from getTableKeyRanges where it is appropriate. Yes, this method is the synchronized method for getTableKeyRanges, so the comment look the same. http://gerrit.cloudera.org:8080/#/c/12323/10/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2049 PS10, Line 2049: } > Shouldn't this use endPrimaryKey not endPartitionKey? Are we > missing test coverage that would surfaced this error? I added some unit tests for this case. http://gerrit.cloudera.org:8080/#/c/12323/10/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2159 PS10, Line 2159: } > These new methods duplicate a lot from locateTable and friends. Can > we share code between them? Sorry, I didn't find a way to share this code. But I've made improvements in code readability. 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: > You can omit this sentence for simple getters and just use the @return. Done http://gerrit.cloudera.org:8080/#/c/12323/10/java/kudu-client/src/main/java/org/apache/kudu/client/KeyRange.java@62 PS10, Line 62: /** : * @return the located tablet : */ : public LocatedTablet getTablet() { : return tablet; : } : > Can we omit this and provide the tablet in the constructor? Done 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.apache.kudu.test.KuduTestHarness; > If you statically import the Assert functions you care about, you can make 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: 20 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: Mon, 13 May 2019 05:52:16 +0000 Gerrit-HasComments: Yes
