Jean-Daniel Cryans has posted comments on this change. Change subject: KUDU-579 [java_client] Scanner fault tolerance ......................................................................
Patch Set 2: (18 comments) http://gerrit.cloudera.org:8080/#/c/6566/2/java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java File java/kudu-client/src/main/java/org/apache/kudu/client/AbstractKuduScannerBuilder.java: Line 78: * Return scan results in primary key sorted order. This javadoc needs to be re-written. Line 89: @InterfaceAudience.Private Remove all the Interface stuff, we don't want this to be private anymore. Line 91: S setFaultTolerant() { Make it public. http://gerrit.cloudera.org:8080/#/c/6566/2/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: Line 505: if (e instanceof ScannerExpiredException) { Shouldn't you only do this for fault tolerant scanners? Also it seems there's a type mismatch now that we're returning a Deferred. This change is a bit suspicious. http://gerrit.cloudera.org:8080/#/c/6566/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java: Line 113: @VisibleForTesting We put this above the method signature, below the javadoc. PS2, Line 115: tabletSlice RemoteTablet? http://gerrit.cloudera.org:8080/#/c/6566/2/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: Line 179: // if uuid is empty, it means the leader has been removed from I'm not following this. If the leader isn't known then we need to go ask the master. Using a different replication selection policy would probably violate what the user is asking for. http://gerrit.cloudera.org:8080/#/c/6566/2/java/kudu-client/src/main/java/org/apache/kudu/client/ScannerExpiredException.java File java/kudu-client/src/main/java/org/apache/kudu/client/ScannerExpiredException.java: Line 2: * Copyright (C) 2010-2012 The Async HBase Authors. All rights reserved. Use a difference license, like KuduSession has, since this file AFAIK isn't copied from asynchbase. PS2, Line 38: KuduException This should be a RecoverableException. PS2, Line 42: . Remove trailing periods from all @param lines PS2, Line 51: T Always start with a lower case. http://gerrit.cloudera.org:8080/#/c/6566/2/java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java File java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java: PS2, Line 515: TABLET_NOT_RUNNING I'm surprised that we've been missing this for so long, but it seems like it was indeed a problem. http://gerrit.cloudera.org:8080/#/c/6566/2/java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java File java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java: PS2, Line 377: . nit Line 393: private static LocatedTablet.Replica getLeaderRpcPort(RemoteTablet curTablet) Part of this is copied from killTabletLeader(KuduTable), can you do the required refactoring instead? PS2, Line 477: . nit Line 483: miniCluster.killTabletServerOnPort(port); Another refactoring opportunity. http://gerrit.cloudera.org:8080/#/c/6566/2/java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java File java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java: Line 53: builder.addHashPartitions(Lists.newArrayList(schema.getColumnByIndex(0).getName()), TABLET_COUNT); nit: now the line is too long. Line 152: public void testFaultTolerantScannerRestartTS() throws Exception { This should be refactor into one helper method and two tests that simple call the method to tell it to either kill or restart. -- To view, visit http://gerrit.cloudera.org:8080/6566 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I89d3634c4255b69e28f2de5412e6a5a9d34e931b Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
