Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20715 )
Change subject: KUDU-3526 [java] Scanner should bind with a tserver in java client. ...................................................................... Patch Set 19: (13 comments) It seems not many applications use Java Kudu client for fetching/reading data: Impala uses Kudu C++ client for that, so don't be surprised to hit more bugs on this path. Thank you for addressing the issue! http://gerrit.cloudera.org:8080/#/c/20715/19//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20715/19//COMMIT_MSG@10 PS19, Line 10: It is because that scanner does not bound with the : tserver with which it first communicates. This is applicable only to scanners with LEADER_ONLY replica selection policy, right? Or scanners with other policy selection are affected as well? http://gerrit.cloudera.org:8080/#/c/20715/19/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java: http://gerrit.cloudera.org:8080/#/c/20715/19/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@290 PS19, Line 290: private String tsUUID = null; It would be great to document this new field. http://gerrit.cloudera.org:8080/#/c/20715/19/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@632 PS19, Line 632: scanFinished(); : return Deferred.fromResult(resp.data); // there Should tsUUID be updated in this case as well? What if the application tries to close the scanner explicitly after this, when there was only one scan batch returned: does it work as expected with this code? http://gerrit.cloudera.org:8080/#/c/20715/19/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java File java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java: http://gerrit.cloudera.org:8080/#/c/20715/19/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java@306 PS19, Line 306: Get the replica server info by uuid Get information on tablet server by its UUID. http://gerrit.cloudera.org:8080/#/c/20715/19/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java@307 PS19, Line 307: replica server What's "replica server"? http://gerrit.cloudera.org:8080/#/c/20715/19/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java@308 PS19, Line 308: replica server ? http://gerrit.cloudera.org:8080/#/c/20715/19/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java: http://gerrit.cloudera.org:8080/#/c/20715/19/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@636 PS19, Line 636: leaderHp nit: rename into 'referenceServerHostPort'? http://gerrit.cloudera.org:8080/#/c/20715/19/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@638 PS19, Line 638: tabletID nit: rename into 'referenceTabletId' http://gerrit.cloudera.org:8080/#/c/20715/19/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@650 PS19, Line 650: quiesceTserver nit: this name for a variable is confusing -- should it be something like leaderStepDown or similar? http://gerrit.cloudera.org:8080/#/c/20715/19/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@654 PS19, Line 654: while (true) { I guess it's better to do this via assertEventuallyTrue() with some reasonable timeout (10 seconds?). You could find how it's done in other Java Kudu tests. http://gerrit.cloudera.org:8080/#/c/20715/19/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@670 PS19, Line 670: // Simulate that another request(like Batch) has sent to the wrong leader tablet server and : // the change of leadership has been acknowledged. The response will demote the leader. Doesn't demoteLeader() simply nullifies a field of the RemoteTablet instance? If so, why to add this comment then? http://gerrit.cloudera.org:8080/#/c/20715/19/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@676 PS19, Line 676: // Send the "next" type of scan requests. nit: it's be clear what's going below on even without this comment; this comment doesn't seem to bring any extra clarity http://gerrit.cloudera.org:8080/#/c/20715/19/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@678 PS19, Line 678: while (kuduScanner.hasMoreRows()) { : kuduScanner.nextRows(); : } Should we check that at least one batch of rows has been returned below? Otherwise, this tests is a dud if kuduScanner.hasMoreRows() returns 'false'. -- To view, visit http://gerrit.cloudera.org:8080/20715 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9cf65f4215e99198dd41b43d14e50c8c23b8a9b2 Gerrit-Change-Number: 20715 Gerrit-PatchSet: 19 Gerrit-Owner: Song Jiacheng <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Song Jiacheng <[email protected]> Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Comment-Date: Wed, 03 Jan 2024 23:54:48 +0000 Gerrit-HasComments: Yes
