Alexey Serbin 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 Done 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: I like it better, thanks! Line 304: queuedMessages = null; > can you add an 'assert queuedMessages.empty()' here? Done Line 507: Preconditions.checkState(state == State.READY); > seems more like an assert to me Done Line 524: sendCallToWire(msg); > does this now risk that calls come across the wire in non-ascending callId Yes, that can happen now. But I don't see how it could break any assumptions in our code. Also, since we were not controlling the ordering of the messages at the Netty level (Channels.write() sends the message asynchronously) even before this change, I don't expect anything else to emerge here. -- 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: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
