Adar Dembo has posted comments on this change. Change subject: KUDU-1364. Don't clear the cache when a server disconnect ......................................................................
Patch Set 3: (12 comments) http://gerrit.cloudera.org:8080/#/c/2449/3/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java: Line 658: } else { There's a lot of indentation here, but one level can be removed if you remove the 'else', which isn't needed (because the 'if' returns). Line 664: // Let fall through. Is this supposed to fall through to the locateTablet() on L700? Because it's going to run through the rest of this new code on the way, and it's not clear if it's supposed to or not. Line 1860: Was "leaderIndex = 0" here previously a bug? Line 1912: boolean removed = removeTabletServer(staleTs); It's confusing to see "removeTabletServer" and "addTabletClient" when both operate on TabletClient. Could you rename one? http://gerrit.cloudera.org:8080/#/c/2449/3/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduScanner.java File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduScanner.java: Line 746: setTablet(AsyncKuduScanner.this.tablet); What was the effect of missing this line (and L752)? http://gerrit.cloudera.org:8080/#/c/2449/3/java/kudu-client/src/main/java/org/kududb/client/TabletClient.java File java/kudu-client/src/main/java/org/kududb/client/TabletClient.java: Line 264: void disconnect() { Can you reuse this within shutdown()? Line 691: // Note JD 03/11/16: null tablets doesn't make sense, if we hit this it's because we didn't Nit: // Note: As of the time of writing (03/11/16), a null tablet here doesn't make sense. If we see a null tablet it's because we didn't set it properly before calling sendRpc(). (it's atypical to put our own names in comments). http://gerrit.cloudera.org:8080/#/c/2449/3/java/kudu-client/src/test/java/org/kududb/client/ITClient.java File java/kudu-client/src/test/java/org/kududb/client/ITClient.java: Line 48: private static final long TEST_TIMEOUT = 600000; Nit: rename to include units. Line 53: private static final AtomicBoolean KEEP_RUNNING = new AtomicBoolean(true); If you make this a CountDownLatch too, the individual threads can both test it and wait on it. That is, the threads won't need to call Thread.sleep(); they can sleep directly via CountDownLatch.await(...). Line 112: for (Thread thread : threads) { : thread.interrupt(); : } Shouldn't be necessary; you've signaled to the threads to stop via KEEP_RUNNING.set(false). If you modify all of the threads to await() on KEEP_RUNNING (and make it a latch), they'll be signaled instantly when KEEP_RUNNING's value changes. Line 176: Random random = new Random(); Store the Random in class state, don't recreate it with each call. Line 201: restartTabletServer(table); Prefix this with BaseKuduTest? -- To view, visit http://gerrit.cloudera.org:8080/2449 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8606bfcedb09af57b66ba0f065067f0f3335a4a8 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
