> 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? > > rajith attapattu wrote: > See the above for an explanation for why this is needed. > > Gordon Sim wrote: > You mean this is here because of the lack of synchronization with the > dispatcher thread? If so that seems a little nasty to me... anyway to do this > more cleanly? > > rajith attapattu wrote: > That is precisely the reason. This also makes the sync call redundant. I > started with the sync() and realized that it wasn't sufficient, hence added > this. > As explained above, I'm not sure if there is a reasonable way to > synchronize with the message delivery thread. > > One possible approach might be is to do something like the > syncDispatchQueue() method. Where we push a certain marker message into the > queue and then we get that we know there are no more messages in the > pipeline. But I'm concerned about the safety and feasibility of such an > approach. > > Robbie I believe is one person who have looked at this code more > extensively in the last little while. So waiting to hear from him about his > ideas as well. I'm open to suggestions on this area. Lets see if we can > collectively figure out a better solution. > > Robbie Gemmell wrote: > (just noticed I didnt press publish yesterday morning on this...oops) > > > One possible approach might be is to do something like the > syncDispatchQueue() method. > > This is exactly the comment I was going to make. Its not the nicest thing > in the world, but I think its better than holding yet more locks. Ensuring > that the broker has finished sending you messages on the stopped session and > then having the Dispatcher do the work and tell you that there isnt anything > left to deliver seems the easiest to reason about, and we already do that > elsewhere so reusing the idea seems like the way to go.
Let me work this out and see how it goes. - 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 > >
