qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "rajith attapattu" <rajit...@gmail.com>
Subject Re: Review Request: QPID-3604 - If connection is started and stopped, the client may get more messages than required by the prefetch value
Date Thu, 17 Nov 2011 22:01:21 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message