qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ken Giusti <kgiu...@redhat.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 14:05:30 GMT
> 
> 
> > On 2011-11-16 09:38:21, Keith Wall wrote:
> > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java,
> > > line 128
> > > <https://reviews.apache.org/r/2832/diff/1/?file=58393#file58393line128>
> > >
> > >     Could this make use of AMQSession#rejectMessage?
> > >     
> > >     I wonder also if this logic sit better in
> > >     AMQSession#notifyConsumer().  It already rejects messages if
> > >     the consumer is closed.  Could it not also reject messages
> > >     if the connection is no longer started?
> > 
> > rajith attapattu wrote:
> >     Keith if you look at the rejectMessage method, it sets the
> >     redelivery option. In this case we should not be setting the
> >     redelivery option bcos the the application did not even see
> >     the message.
> >     
> >     I think we need to make a clear distinction btw reject and this
> >     case. If we are rejecting a message then we need to set
> >     redelivery. In other words the application had a look at it
> >     but decided not to use it. However in JMS you can't reject a
> >     message. So I'm not sure if setting redelivery in the
> >     rejectMessage is correct either.
> >     
> >     IMO the only time we should mark a message redelivered is when
> >     the application has seen a message but has not yet
> >     acknowledged. Ex consuming a bunch of messages in CLIENT_ACK
> >     and closing the consumer without acking any of the messages.
> >     
> >     Messages in the prefetch buffer should not be marked
> >     redelivered.  I see there a few places where the rejectMessage
> >     method being used, and I don't think this is correct. Ex when
> >     we set a MessageListener we remove all messages in the
> >     internal queue and release them by setting the redelivery
> >     option.
> > 
> > rajith attapattu wrote:
> >     Actually disregard the above comment. I totally forgot that the
> >     broker will mark all released messages as redelivered. So what
> >     the client sets doesn't really matter.
> >
> 
> Re: "the broker will mark all released messages as redelivered. So
> what the client sets doesn't really matter."
> 
> That is not the case. The broker does what the client tells it to via
> the set-redelivered field of the message-release command.
> 
> 
> - Gordon

Rajith - mea culpa, you specifically asked me about this behavior yesterday, and I missed
the fact that Gordon points out above: the client can optionally tell the broker -not- to
set the redelivered-flag when it releases the message.

Sorry,

-K



> 
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/2832/#review3294
> -----------------------------------------------------------
> 
> 
> 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
> > 
> >
> 
> 

---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project:      http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org


Mime
View raw message