Adar Dembo has posted comments on this change. Change subject: [java client] RPCs can get lost in a TabletClient race ......................................................................
Patch Set 1: (4 comments) What releases (if any) are affected by this? We've been applying bandaid after bandaid for these concurrency issues, and the lack of regression tests is increasingly concerning. Is there something missing from the client that'd make writing such tests easier? 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 not clear what the race is from this terse description. Can you rewrite with a concrete example? 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 am I missing? Line 685: // After this, rpcs_inflight might still have entries since it could have been added Nit: it -> they Line 686: // concurrently, and those RPCs will be handled by their called in sendRpc. Nit: "their called"? Maybe just rewrite this paragraph to make it more clear. -- 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: Kudu Jenkins Gerrit-HasComments: Yes
