Jean-Daniel Cryans has posted comments on this change. Change subject: [java client] Decouple TabletClient from RemoteTablet ......................................................................
Patch Set 1: (14 comments) http://gerrit.cloudera.org:8080/#/c/4757/1/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: PS1, Line 1287: if (!lookupExceptions.isEmpty() && : lookupExceptions.size() == tabletPb.getReplicasCount()) { : Status statusIOE = Status.IOError("Couldn't find any valid locations, exceptions: " + : lookupExceptions); : throw new NonRecoverableException(statusIOE); : } > Shouldn't this be outside the inner loop? I guess it'd be cleaner, but as far as I can tell it does the same thing. http://gerrit.cloudera.org:8080/#/c/4757/1/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java File java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java: PS1, Line 77: private final Lock readLock = lock.readLock(); : private final Lock writeLock = lock.readLock(); > This is just for shorthand, right? Yup! Line 127: uuid2client.put(uuid, client); > So if we reconnect to a server we thought was "dead", we'll evict it from u Yes, should I add a comment? Line 187: ArrayList<Deferred<Void>> deferreds = new ArrayList<>(); > Nit: could preallocate this list with uuid2client.size(). That'd require locking twice... didn't think it was a big deal since this happens only when shutting down the client. PS1, Line 200: Modifying the list returned by this method won't affect this cache > Maybe return a Guava ImmutableList (either directly, or hidden behind a Lis Done Line 261: private final class TabletClientPipeline extends DefaultChannelPipeline { > What was the effect of removing all the code here? We used to have to lookup the hostnames, but it wasn't necessary since a while ago I added them in TabletClient. Then it doesn't really matter if we remove all the TabletClients or not, as long as we shut them down, which is why I added a unit test that verifies that we do it. http://gerrit.cloudera.org:8080/#/c/4757/1/java/kudu-client/src/main/java/org/apache/kudu/client/PingRequest.java File java/kudu-client/src/main/java/org/apache/kudu/client/PingRequest.java: Line 27: * > ? Fill it with your imagination. I added something at some point, then ctrl-z'd a bunch, forgot I had to add that back. http://gerrit.cloudera.org:8080/#/c/4757/1/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 56: private final ArrayList<String> tabletServers = new ArrayList<>(); > Would it be simpler to make this a Set? Certainly add/remove will get simpl Indeed, that's even better. Line 70: synchronized (tabletServers) { > Shouldn't be necessary in the constructor; no other thread could have a ref Won't GuardedBy complain? PS1, Line 71: tabletServers.clear(); : leaderIndex = NO_LEADER_INDEX; > Also unnecessary; these are their initial values. Forgot to remove them, thanks. http://gerrit.cloudera.org:8080/#/c/4757/1/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: Line 109: allDead = true; > I'm confused by this; why does the fact that there was at least one tablet err allDead should be next to break below. We're failing if we don't have servers because we're expecting to have connections to disconnect. http://gerrit.cloudera.org:8080/#/c/4757/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java: Line 27: public class TestConnectionCache { > Why not use BaseKuduTest or whatever for this test? To be able to use the mini cluster directly. PS1, Line 54: waitForConnectionToDie > Somewhat odd that join() on shutdown() doesn't automatically take care of t Yeah I spent some time trying to figure that one out... something something shenanigans. PS1, Line 77: 5000 > Not DEFAULT_SLEEP? Right without BaseKuduTest that's not reachable. -- To view, visit http://gerrit.cloudera.org:8080/4757 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If3ad2190c7e2c7f51cb9ffe6ed3348b62488e675 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
