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 Wed, 16 Nov 2011 15:49:36 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?

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.


> On 2011-11-16 09:38:21, Keith Wall wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/systests/src/main/java/org/apache/qpid/client/prefetch/PrefetchBehaviourTest.java,
line 135
> > <https://reviews.apache.org/r/2832/diff/1/?file=58394#file58394line135>
> >
> >     Could you not use the test utility method for the production of these messages?
> >     QpidBrokerTestCase#sendMessage

that could be done.


> On 2011-11-16 09:38:21, Keith Wall wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/systests/src/main/java/org/apache/qpid/client/prefetch/PrefetchBehaviourTest.java,
line 140
> > <https://reviews.apache.org/r/2832/diff/1/?file=58394#file58394line140>
> >
> >     I think using a timeout here would be preferable. IMHO we should avoid writing
unit tests that can hang indefinitely in favour of those that will always fail with a useful
assertion-fail.
> >     
> >     Also typo in assertion message (once -> one).

Ah, I did have a timeout and the typo was correct (as it was pointed out by someone else too),
it seems like I generated the patches before these changes.
Definitely we should always use a timeout. This will be corrected.


> On 2011-11-16 09:38:21, Keith Wall wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/systests/src/main/java/org/apache/qpid/client/prefetch/PrefetchBehaviourTest.java,
line 152
> > <https://reviews.apache.org/r/2832/diff/1/?file=58394#file58394line152>
> >
> >     You've got a couple of whitespace issues that make the patch slightly larger
than need be :)

It seems eclipse is doing this. I tried using the AnyEdit plugging and it made matters worse.
I will try to edit this on the command line before I do the final commit :)


- rajith


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


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