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 23: (18 comments) http://gerrit.cloudera.org:8080/#/c/12323/20/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/20/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1942 PS20, Line 1942: * @param startPrimaryKey the primary key to begin splitting at (inclusive), pass null to : * start splitting at the beginning of t > Same. Done http://gerrit.cloudera.org:8080/#/c/12323/20/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1978 PS20, Line 1978: * This method blocks until it gets all the key ranges. : * @param table the table to get key ranges from > See my other comments about this in KuduTable. Done http://gerrit.cloudera.org:8080/#/c/12323/20/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1990 PS20, Line 1990: * larger or smaller than this value. If unset or <= 0, the key range > Not your fault, but MasterErrorException hasn't existed since commit 0a7923 Done http://gerrit.cloudera.org:8080/#/c/12323/20/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2015 PS20, Line 2015: * Get all or some key range for a given table. This may query the master multiple times if there > Nit: unnecessary empty line here. Done http://gerrit.cloudera.org:8080/#/c/12323/20/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2016 PS20, Line 2016: * 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 > Same. Done http://gerrit.cloudera.org:8080/#/c/12323/20/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2080 PS20, Line 2080: } > The join here means that this isn't an async call, and will likely muck wit This is a good idea, I have not used Deferred.group before. :) http://gerrit.cloudera.org:8080/#/c/12323/20/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/20/java/kudu-client/src/main/java/org/apache/kudu/client/KeyRange.java@35 PS20, Line 35: * @param primaryKeyStart the encoded primary key where to start in the key range (inclusive) > primary Done http://gerrit.cloudera.org:8080/#/c/12323/20/java/kudu-client/src/main/java/org/apache/kudu/client/KeyRange.java@38 PS20, Line 38: */ > Doc 'tablet'. Based on toString() it looks like it could be null; when does It cannot be null, we add doc for this http://gerrit.cloudera.org:8080/#/c/12323/20/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/20/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java@312 PS20, Line 312: It is used to split tablet' > It is used to split the tablet's Done http://gerrit.cloudera.org:8080/#/c/12323/20/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java@416 PS20, Line 416: newKeyRan > Shouldn't this be newKeyRanges? > > Seems like a pretty serious bug (L419 could throw an > IndexOutOfRangeException); why didn't any unit tests catch this? Yes, it should be use newKeyRanges. But the current implementation, newKeyRange will not be empty (getTabletsKeyRanges returns at least one). http://gerrit.cloudera.org:8080/#/c/12323/20/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java@435 PS20, Line 435: > Could use primaryKeyStart here (local declared on L432). Done http://gerrit.cloudera.org:8080/#/c/12323/20/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/20/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java@226 PS20, Line 226: range > ranges Done http://gerrit.cloudera.org:8080/#/c/12323/20/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java@230 PS20, Line 230: * @param primaryKeyStart the primary key to begin splitting at (inclusive), pass null to : * start splitting at the beginning of t > This documentation is confusing: is this saying that if these are null, we I'm so sorry, I misunderstood your previous comments. http://gerrit.cloudera.org:8080/#/c/12323/20/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/20/java/kudu-client/src/main/java/org/apache/kudu/client/SplitKeyRangeRequest.java@55 PS20, Line 55: * @param timeoutMillis the timeout of the request in milliseconds > Doc this one? Done http://gerrit.cloudera.org:8080/#/c/12323/20/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/20/java/kudu-client/src/test/java/org/apache/kudu/client/TestSplitKeyRange.java@20 PS20, Line 20: import static org.apache.kudu.test.ClientTestUtil.createTableWithOneThousandRows; > Please expand this. Done http://gerrit.cloudera.org:8080/#/c/12323/20/java/kudu-client/src/test/java/org/apache/kudu/client/TestSplitKeyRange.java@121 PS20, Line 121: // TODO: Response should be return empty. But this does not affect scan result. > If we flushed the MRS by sleeping on L59, how is this querying the > MRS? The test case is to check if the DRS's data is not in [primaryKeyStart, primaryKeyEnd), the rpc must return a key range. Because the MRS's data may be in the [primaryKeyStart, primaryKeyEnd). I think it would be better to add some doc. http://gerrit.cloudera.org:8080/#/c/12323/20/java/kudu-client/src/test/java/org/apache/kudu/client/TestSplitKeyRange.java@123 PS20, Line 123: assertEquals(tablet1.toString(), keyRanges.get(0).getTablet().toString()); > Nit: indentation. Done http://gerrit.cloudera.org:8080/#/c/12323/20/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala File java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala: http://gerrit.cloudera.org:8080/#/c/12323/20/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala@1043 PS20, Line 1043: val t = "scanWithKeyRangeTest" : sqlContext.read.options(kuduOptions).format("kudu").load.createOrReplaceTempView(t) : val results = sqlContext.sql(s"SELECT * FROM $t").collectAsList() : assert(results.size() == rowCount * 100) : } : assert(actualNumTasks >= 2) : } : } > This has turned out to be a flaky test pattern. See https://gerrit.cloudera 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: 23 Gerrit-Owner: yangz <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yao Xu <[email protected]> Gerrit-Reviewer: yangz <[email protected]> Gerrit-Comment-Date: Tue, 14 May 2019 15:21:58 +0000 Gerrit-HasComments: Yes
