> On 2011-11-15 16:41:28, Gordon Sim wrote: > > I don't have a clear enough mental picture of the code you are modifying, > > so am really only able to ask some questions...
Thanks a lot for taking the time to review. I really appreciate it. > On 2011-11-15 16:41:28, Gordon Sim wrote: > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java, > > line 787 > > <https://reviews.apache.org/r/2832/diff/1/?file=58392#file58392line787> > > > > Are there any other locks acquired as part of the block here? If so are > > there any lock ordering issues where you could be introducing a deadlock? Not that I could think of. The message-delivery-lock is taken to ensure that no messages are being served while we start pulling them out of the queue. In my tests so far, I haven't encountered any issues. However I plan to have more manual tests - ex. Trying to stop the connection while the message consumers are in full flight. > On 2011-11-15 16:41:28, Gordon Sim wrote: > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java, > > line 796 > > <https://reviews.apache.org/r/2832/diff/1/?file=58392#file58392line796> > > > > You are syncing here while holding the delivery lock, could that cause > > any problems? So far I haven't encountered any issues. However things like failover, session exceptions etc..could cause issues. I'm planning more thorough longer running tests. Another thing I am considering is to not use a sync() at all. I'm not quite convinced that it's of much value here. I've noticed that the client continues to get messages into it's queue even after the code returns from the sync call. Hence the code snippet to release any messages received after the connection is stopped. I was expecting the brokers response to the sync command to be received after the client has got all the messages that were in flight. So after I sync I could just release the messages in the queue and be done with it. But that's not the case. It seems that the dispatcher thread takes a bit of time to process the UnprocessedMessages into the correct JMSMessages and put them onto the queue. So the sync() really doesn't add much value here. > On 2011-11-15 16:41:28, Gordon Sim wrote: > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java, > > line 126 > > <https://reviews.apache.org/r/2832/diff/1/?file=58393#file58393line126> > > > > What case(s) is this code required for? You are releasing a message you > > have just received, right? When is that required? See the above for an explanation for why this is needed. - rajith ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2832/#review3264 ----------------------------------------------------------- On 2011-11-15 15:36:36, rajith attapattu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/2832/ > ----------------------------------------------------------- > > (Updated 2011-11-15 15:36:36) > > > Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, and > Oleksandr Rudyy. > > > Summary > ------- > > This attempts to fix one of the issues related to the handling of Message > credits. See QPID-3602 for an overall picture of the various issues. > > This particular patch does the following. > 1. When the connection is stopped, it sends message.stop() & releases all > messages in the prefetch buffer. > 2. It will also release any messages (that were in flight) that comes after > the connection is stopped. (*) > > (*) This interferes with the immediate_prefetch feature. However I don't know > if immediate prefetch is really required in the 0-10 path. > > As always comments, suggestions & criticisms are equally welcomed. > > > This addresses bug QPID-3604. > https://issues.apache.org/jira/browse/QPID-3604 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java > 1202228 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java > 1202228 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/systests/src/main/java/org/apache/qpid/client/prefetch/PrefetchBehaviourTest.java > 1202228 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java > 1202228 > > Diff: https://reviews.apache.org/r/2832/diff > > > Testing > ------- > > See PrefetchBehaviourTest#testConnectionStop for more details. > > > Thanks, > > rajith > >
