Hao Hao has posted comments on this change. Change subject: KUDU-579 [java_client] Scanner fault tolerance ......................................................................
Patch Set 3: (17 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: * Ensures the scanner to be fault tolerant, and returns scan results in primary > This javadoc needs to be re-written. Done Line 89: return (S) this; > Remove all the Interface stuff, we don't want this to be private anymore. Done Line 91: > Make it public. Done 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: // is fault tolerant. > Shouldn't you only do this for fault tolerant scanners? Also it seems there Done 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: /** > We put this above the method signature, below the javadoc. Done PS2, Line 115: > RemoteTablet? Done 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 th I also do not have a good feeling about this. However, without this change, ITScannerMultiTablet.testFaultTolerantScannerKillTS would keep on retry RPC until timeout and eventually fail. It seems it can never get the correct leader information. Any suggestions on this? :) 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: // or more contributor license agreements. See the NOTICE file > Use a difference license, like KuduSession has, since this file AFAIK isn't Done PS2, Line 38: > This should be a RecoverableException. Done PS2, Line 42: m > Remove trailing periods from all @param lines Done PS2, Line 51: > Always start with a lower case. Done 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: l > nit Done Line 393: } > Part of this is copied from killTabletLeader(KuduTable), can you do the req Done PS2, Line 477: > nit Done Line 483: } > Another refactoring opportunity. Done 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( > nit: now the line is too long. Done Line 152: killTabletLeader(scanner.currentTablet()); > This should be refactor into one helper method and two tests that simple ca Done -- 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: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
