Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16941 )
Change subject: KUDU-3205: Fix building scan tokens when tablet not found errors occur ...................................................................... Patch Set 1: Code-Review+1 (5 comments) http://gerrit.cloudera.org:8080/#/c/16941/1/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/16941/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java@685 PS1, Line 685: if (serverIndex != null) { nit: for readability, consider if (serverIndex == null) { continue; } // business as usual http://gerrit.cloudera.org:8080/#/c/16941/1/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/16941/1/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java@263 PS1, Line 263: * Get replicas of this tablet. The returned list may not be mutated. > nit: do you think it's worth adding a note here expressing that the replica +1 http://gerrit.cloudera.org:8080/#/c/16941/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java: http://gerrit.cloudera.org:8080/#/c/16941/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java@732 PS1, Line 732: KuduScanToken.KuduScanTokenBuilder tokenBuilder nit: maybe, add includeTabletMetadata(true) and includeTableMetadata(true) to be explicit what settings are important here, or maybe just add an ASSERT to make sure the builder or the generated token has those properties set. http://gerrit.cloudera.org:8080/#/c/16941/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java@733 PS1, Line 733: tokens Probably, that's not exactly related to this change, bust just in case: what happens in case of ReplicaSelection.CLOSEST_REPLICA mode and when the information on one of the replicas is missing in the token? Should client try to re-resolve the location of replicas in that case because the new replica might be placed on the same node where the client runs? http://gerrit.cloudera.org:8080/#/c/16941/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java@739 PS1, Line 739: tokens.get(0).intoScanner(newClient); Does it matter what's the role of the replica removed? Does it work as expected when a leader replica's metadata has been removed from the token, and the ReplicaSelection is set to ReplicaSelection.LEADER_ONLY? -- To view, visit http://gerrit.cloudera.org:8080/16941 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I68679dee1ad7ebca405dd6e086770f3e034e310c Gerrit-Change-Number: 16941 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 12 Jan 2021 01:31:50 +0000 Gerrit-HasComments: Yes
