Jean-Daniel Cryans has posted comments on this change. Change subject: [java client] RPCs can get lost in a TabletClient race ......................................................................
Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/3541/1/java/kudu-client/src/main/java/org/kududb/client/TabletClient.java File java/kudu-client/src/main/java/org/kududb/client/TabletClient.java: PS1, Line 187: We got disconnected, cleanup ran, then we added the rpc to rpcs_inflight, so we need to : // manually failOrRetry it. > I think this is too vague. Multiple threads are at work here, right? It's n Done PS1, Line 678: // Removing from the iterator because ConcurrentHashMap doesn't have a way to retrieve and : // remove all the entries atomically. : for (Iterator<KuduRpc<?>> iterator = rpcs_inflight.values().iterator(); iterator.hasNext();) { : KuduRpc<?> rpc = iterator.next(); : rpcs.add(rpc); : iterator.remove(); : } > I don't get it; how is this any more "atomic" than the previous code? What It's not more atomic, we get around this by assuming we might be missing some rpcs that got added while iterating, hence the check in sendRpc above. I removed the comment, it's not really making things clearer. Line 685: // After this, rpcs_inflight might still have entries since it could have been added > Nit: it -> they Done Line 686: // concurrently, and those RPCs will be handled by their called in sendRpc. > Nit: "their called"? Done -- To view, visit http://gerrit.cloudera.org:8080/3541 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaff89eb832d0d6f0dede198661856fae1a8585a0 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dan Burkert <d...@cloudera.com> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
