Song Jiacheng 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 23: (13 comments) Thanks for the review! 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 scanner does not bind with the tserver : with which it first communicates. > This is applicable only to scanners with LEADER_ONLY replica selection poli This problem is triggered when getReplicaSelectedServerInfo cound not find a tserver because of the leader demotion. For the policy CLOSEST_REPLICA I think this will not happen. 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: /** > It would be great to document this new field. Done http://gerrit.cloudera.org:8080/#/c/20715/19/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@632 PS19, Line 632: resourceMetrics.update(resp.resourceMetricsPb); : } > Should tsUUID be updated in this case as well? What if the application tri Done 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 information on tablet server by > Get information on tablet server by its UUID. Done http://gerrit.cloudera.org:8080/#/c/20715/19/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java@307 PS19, Line 307: tablet server > What's "replica server"? Done http://gerrit.cloudera.org:8080/#/c/20715/19/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java@308 PS19, Line 308: tablet server > ? Sorry, I'm also very confused how it came from. 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: scanner > nit: rename into 'referenceServerHostPort'? Done http://gerrit.cloudera.org:8080/#/c/20715/19/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@638 PS19, Line 638: ostAndPo > nit: rename into 'referenceTabletId' Done http://gerrit.cloudera.org:8080/#/c/20715/19/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@650 PS19, Line 650: uilder process > nit: this name for a variable is confusing -- should it be something like l Done http://gerrit.cloudera.org:8080/#/c/20715/19/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@654 PS19, Line 654: > I guess it's better to do this via assertEventuallyTrue() with some reasona Tried to search for the method but did not find it. Thanks! http://gerrit.cloudera.org:8080/#/c/20715/19/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@670 PS19, Line 670: HostAndPort targetHp = harness.findLeaderTabletServer(targetTablet); : return !targetHp.equals(referenceServerHostPort); > Doesn't demoteLeader() simply nullifies a field of the RemoteTablet instanc These two operations, demoteLeader() and emptyTabletsCacheForTable() , are actually the behaviors of the client when another request has sent to the wrong leader tablet server and the change of leadership has been acknowledged. Not only the demoteLeader(). http://gerrit.cloudera.org:8080/#/c/20715/19/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@676 PS19, Line 676: // Simulate that another request(like Bat > nit: it's be clear what's going below on even without this comment; this co Done http://gerrit.cloudera.org:8080/#/c/20715/19/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@678 PS19, Line 678: kuduScanner.currentTablet().demoteLeader( : kuduScanner.currentTablet().getLeaderServerInfo().getUuid()); : asy > Should we check that at least one batch of rows has been returned below? O Done -- 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: 23 Gerrit-Owner: Song Jiacheng <songjiach...@thinkingdata.cn> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <greber...@gmail.com> Gerrit-Reviewer: Song Jiacheng <songjiach...@thinkingdata.cn> Gerrit-Reviewer: Yifan Zhang <chinazhangyi...@163.com> Gerrit-Comment-Date: Thu, 04 Jan 2024 13:51:29 +0000 Gerrit-HasComments: Yes
