[
https://issues.apache.org/jira/browse/QPID-3604?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13151278#comment-13151278
]
jirapos...@reviews.apache.org commented on QPID-3604:
-----------------------------------------------------
bq. On 2011-11-16 09:38:21, Keith Wall wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java,
line 128
bq. > <https://reviews.apache.org/r/2832/diff/1/?file=58393#file58393line128>
bq. >
bq. > Could this make use of AMQSession#rejectMessage?
bq. >
bq. > 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.
bq. On 2011-11-16 09:38:21, Keith Wall wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/systests/src/main/java/org/apache/qpid/client/prefetch/PrefetchBehaviourTest.java,
line 135
bq. > <https://reviews.apache.org/r/2832/diff/1/?file=58394#file58394line135>
bq. >
bq. > Could you not use the test utility method for the production of
these messages?
bq. > QpidBrokerTestCase#sendMessage
that could be done.
bq. On 2011-11-16 09:38:21, Keith Wall wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/systests/src/main/java/org/apache/qpid/client/prefetch/PrefetchBehaviourTest.java,
line 140
bq. > <https://reviews.apache.org/r/2832/diff/1/?file=58394#file58394line140>
bq. >
bq. > 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.
bq. >
bq. > 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.
bq. On 2011-11-16 09:38:21, Keith Wall wrote:
bq. >
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/systests/src/main/java/org/apache/qpid/client/prefetch/PrefetchBehaviourTest.java,
line 152
bq. > <https://reviews.apache.org/r/2832/diff/1/?file=58394#file58394line152>
bq. >
bq. > 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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/2832/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-11-15 15:36:36)
bq.
bq.
bq. Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, and
Oleksandr Rudyy.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. 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.
bq.
bq. This particular patch does the following.
bq. 1. When the connection is stopped, it sends message.stop() & releases all
messages in the prefetch buffer.
bq. 2. It will also release any messages (that were in flight) that comes
after the connection is stopped. (*)
bq.
bq. (*) This interferes with the immediate_prefetch feature. However I don't
know if immediate prefetch is really required in the 0-10 path.
bq.
bq. As always comments, suggestions & criticisms are equally welcomed.
bq.
bq.
bq. This addresses bug QPID-3604.
bq. https://issues.apache.org/jira/browse/QPID-3604
bq.
bq.
bq. Diffs
bq. -----
bq.
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java
1202228
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java
1202228
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/systests/src/main/java/org/apache/qpid/client/prefetch/PrefetchBehaviourTest.java
1202228
bq.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java
1202228
bq.
bq. Diff: https://reviews.apache.org/r/2832/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. See PrefetchBehaviourTest#testConnectionStop for more details.
bq.
bq.
bq. Thanks,
bq.
bq. rajith
bq.
bq.
> If the connection is stopped the client should release all it's messages in
> the prefetch buffer
> -----------------------------------------------------------------------------------------------
>
> Key: QPID-3604
> URL: https://issues.apache.org/jira/browse/QPID-3604
> Project: Qpid
> Issue Type: Bug
> Components: Java Client
> Affects Versions: 0.14
> Reporter: Rajith Attapattu
> Assignee: Rajith Attapattu
> Fix For: 0.15
>
>
> When connection.stop() is called, the JMS client should release all it's
> messages in the prefetch buffer.
> For all we know, the connection may never be started (depending on
> application logic) and those messages will be stuck on the prefetch buffer.
> Releasing it will allow another consumer to get them (in the case of a shared
> queue case).
> Another less severe but nevertheless an undesirable side affect of this is
> the client getting more messages than required by the capacity or prefetch
> arguments. See QPID-3602
> This may not be a big issue if the client is prefetching a few messages, but
> if prefetching something like 5000 messages, this could potentially cause a
> lethal spike in the clients memory usage.
> Even in low capacity/prefetch values, if the messages are large (say in the
> mega byte range) this could potentially put the client under memory pressure.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscr...@qpid.apache.org
