Todd Lipcon has posted comments on this change. Change subject: KUDU-1894 fixed deadlock in client.Connection ......................................................................
Patch Set 4: (5 comments) http://gerrit.cloudera.org:8080/#/c/7765/4/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java File java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java: PS4, Line 274: while (true) { : List<QueuedMessage> queued = Preconditions.checkNotNull(queuedMessages); : if (queued.isEmpty()) { : break; even though we'll lose the Preconditions check here, I think this would be easier to follow with just a : while (!queuedMessages.empty()) { ...} in terms of the exception, it'll throw an NPE either way, so I dont think it's much of a loss. PS4, Line 290: lock.unlock(); : needs_unlock = false; : // Send out the enqueued messages while not holding the lock. This is to avoid : // deadlock if channelDisconnected/channelClosed event happens and cleanup() is called. : for (final QueuedMessage qm : queued) { : sendCallToWire(qm.message); : } : lock.lock(); : needs_unlock = true; I think it's slightly more idiomatic to do: lock.unlock(); try { } finally { lock.lock(); } for the 'unlocked' periodic here, even though in some rare circumstance in which there is an exception thrown, it would cause an extra lock/unlock Line 304: queuedMessages = null; can you add an 'assert queuedMessages.empty()' here? Line 507: Preconditions.checkState(state == State.READY); seems more like an assert to me Line 524: sendCallToWire(msg); does this now risk that calls come across the wire in non-ascending callId order? Does that break any assumptions elsewhere in the code? -- To view, visit http://gerrit.cloudera.org:8080/7765 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1a0edc3e3accbcff60f2cde641ee470312bbd27a Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
