Jean-Daniel Cryans has posted comments on this change. Change subject: KUDU-1364. Don't clear the cache when a server disconnect ......................................................................
Patch Set 3: (15 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 remo derp, thanks Line 664: // Let fall through. > Is this supposed to fall through to the locateTablet() on L700? Because it' newTabletClient should be null, then fall through. Maybe change the comment to make this clearer? Line 1860: > Was "leaderIndex = 0" here previously a bug? Yup. Line 1906: int index = tabletServers.indexOf(staleTs); > Is it possible that staleTs has already been removed at this point? It's a good point, there's going to be some churn if multiple threads are getting that staleTs. It should be fine early exiting. Line 1912: boolean removed = removeTabletServer(staleTs); > It's confusing to see "removeTabletServer" and "addTabletClient" when both Good idea. 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)? That's the scan bug I'm talking about in the commit message. Any disconnection was able to kill a scanner, even if the server didn't actually die. 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()? Doesn't seem worth it, might end up being more code. Line 265: Channel chancopy = chan; > Why is the copy necessary here? This is just another reference to the same chan can be set to null in channelClosed and channelDisconnected, so you might check for null but then get an NPE on the next line. 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 does For that kind of comment I've actually seen that this, which is why I did it. But it's easy to git blame I guess, so I'll remove. 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. I had to look at the code to remember which unit to use :P Line 52: // Hatch used to track if an error occurred and we need to stop the test early. > Is Hatch a typo? No? Ok yes. Also the comments are reversed. Line 53: private static final AtomicBoolean KEEP_RUNNING = new AtomicBoolean(true); > If you make this a CountDownLatch too, the individual threads can both test That's brilliant. Line 112: for (Thread thread : threads) { : thread.interrupt(); : } > Shouldn't be necessary; you've signaled to the threads to stop via KEEP_RUN The threads could also be stuck in sleeps inside the clients, even after giving them a final chance at line 110. Line 176: Random random = new Random(); > Store the Random in class state, don't recreate it with each call. Done Line 201: restartTabletServer(table); > Prefix this with BaseKuduTest? Done -- 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: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
