Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17971 )
Change subject: KUDU-3295: Fix the number of replicas does not equal the number of servers ...................................................................... Patch Set 2: (5 comments) Thanks for the patch! http://gerrit.cloudera.org:8080/#/c/17971/2/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/17971/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2374 PS2, Line 2374: dnsFailedServers.add(addresses.get(0).getHost()); nit: could you add a comment explaining that resolveTS() only resolves the first address, which is why we're not adding the rest of the addresses? http://gerrit.cloudera.org:8080/#/c/17971/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2417 PS2, Line 2417: if (!lookupExceptions.isEmpty() && : lookupExceptions.size() == tabletPb.getInternedReplicasCount()) { : Status statusIOE = Status.IOError("Couldn't find any valid locations, exceptions: " + : lookupExceptions); : throw new NonRecoverableException(statusIOE); : } : Seems like we should be handling the case where all replicas have failed to be looked up. http://gerrit.cloudera.org:8080/#/c/17971/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java: http://gerrit.cloudera.org:8080/#/c/17971/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java@200 PS2, Line 200: call discoverTablets nit: maybe remove this comment, given it doesn't add much context http://gerrit.cloudera.org:8080/#/c/17971/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java@204 PS2, Line 204: Builder nit: Build Also, we're only creating one bad location, aren't we? The other locations are fine, right? http://gerrit.cloudera.org:8080/#/c/17971/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java@205 PS2, Line 205: Master.TabletLocationsPB.Builder tabletPb = Master.TabletLocationsPB.newBuilder(); nit: while it's nice we can reuse this, it seems less error prone to create a new builder in the loop -- To view, visit http://gerrit.cloudera.org:8080/17971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7d2e7d49d234e41e66e9c367e061fb7da32e9b47 Gerrit-Change-Number: 17971 Gerrit-PatchSet: 2 Gerrit-Owner: Hongjiang Zhang <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hongjiang Zhang <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 26 Oct 2021 17:19:56 +0000 Gerrit-HasComments: Yes
