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: Sending of completions should be independent of sending message acks
Date Thu, 17 Nov 2011 22:27:55 GMT


> On 2011-11-17 21:22:31, Robbie Gemmell wrote:
> > In addition to the code-specific items already noted, I thought id mention that
it would be good if this change pulled in the rollback/recover related completion issue Keith
and I patched today. As I mentioned on the phone earlier, we went for the simplest (albeit
hacky) solution we could to make it clearer for inclusion in the release, but it would be
nice to see it cleaned up. This change looks to take care of the messages which were actually
delivered before the rollback was performed, but it still wont handle the ones which were
only prefetched.
> > 
> > I noticed that you stopped the flusher thread running for modes other than DUPS_OK,
which is nice. Alex questioned whether this change would affect browsers or not because they
ignore the session acknowledge mode and go with NO_ACK behaviour. Looking it over I think
it seems fine due to the processCompletions call in postDeliver(), but its possibly still
worth a check.

Your observation is correct, this deals with messages that were already received by the application.
For recover(), we really don't need to send completions for the transfers we get bcos we do
a stop and start which re-issues the credit. But for correctness we should mark them complete.
As for rollback we definitely should mark the messages that were in the prefetch buffer as
complete so we get re-credited. 
Let me pull in the change and see how I could integrate it with some cleanup.

Starting the flusher thread only for DUPS_OK was overdue :)
Do we have any existing tests that test this specific aspect of QueueBrowsers ?


> On 2011-11-17 21:22:31, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java,
line 194
> > <https://reviews.apache.org/r/2853/diff/1/?file=58798#file58798line194>
> >
> >     another whitespace error

:( this is killing me.


> On 2011-11-17 21:22:31, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java,
line 284
> > <https://reviews.apache.org/r/2853/diff/1/?file=58798#file58798line284>
> >
> >     I'll stop after this one...another whitespace error :)

Have mercy on me pls :D


> On 2011-11-17 21:22:31, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java,
lines 188-202
> > <https://reviews.apache.org/r/2853/diff/1/?file=58799#file58799line188>
> >
> >     The change to this method mean it no longer needs to exist and should be removed.

Agreed. Left it there to better illustrate the diff. Will get rid of this before committing.


> On 2011-11-17 21:22:31, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java,
lines 441-444
> > <https://reviews.apache.org/r/2853/diff/1/?file=58799#file58799line441>
> >
> >     Do we need to do this anymore considering that it just sends a completion, given
the processCompletions call added on L420 ?

I think you are right good catch !
But let me verify this.


> On 2011-11-17 21:22:31, Robbie Gemmell wrote:
> > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/systests/src/main/java/org/apache/qpid/client/prefetch/PrefetchBehaviourTest.java,
line 173
> > <https://reviews.apache.org/r/2853/diff/1/?file=58800#file58800line173>
> >
> >     Some onMessage() tests would be good, given recent disparity between its behaviour
and receive() for things like this.

Definitely this is in the cards for sure!
I'm going to spend some time to see how best I can reuse the test code to make it more compact
and test more combinations/variations.


- rajith


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2853/#review3325
-----------------------------------------------------------


On 2011-11-16 18:31:12, rajith attapattu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/2853/
> -----------------------------------------------------------
> 
> (Updated 2011-11-16 18:31:12)
> 
> 
> Review request for qpid, Gordon Sim, Robbie Gemmell, Weston Price, Keith Wall, and Oleksandr
Rudyy.
> 
> 
> Summary
> -------
> 
> This patch is for both QPID-3612 & QPID-3613. Please note QPID-3602 is the main JIRA
for credit related issues.
> 
> Message completions are now handled independent of the Message acks.
> 
> 1. The consumer now keeps track of credits on a per subscription basis using the RangeSet
"completions" and sends completions,
>    if the unCompletedCount >= _capacity/2
>    For capacity = 0 or capacity = 1, we use the flag "sendCompleteForEveryMsg" to send
a completion for each message.
> 
> 2. Cleanedup the acking process in AMQSession_0_10.java to make acking independent of
completions.
>    2.1 For AUTO_ACK we send an ack after every message.
>    2.1 For DUPS_OK we ack if,
>           maxAckDelay <= 0 OR
>           unackedCount >= ackBatchingThreshold, where ackBatchingThreshold == prefetch/2
> 
> 
> This addresses bug QPID-3602.
>     https://issues.apache.org/jira/browse/QPID-3602
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java
1202779 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java
1202779 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/systests/src/main/java/org/apache/qpid/client/prefetch/PrefetchBehaviourTest.java
1202779 
> 
> Diff: https://reviews.apache.org/r/2853/diff
> 
> 
> Testing
> -------
> 
> Added two basic test cases (transacted and client-ack modes) to ensure completions are
sent in time to ensure credits don't dry up.
> 
> 
> Thanks,
> 
> rajith
> 
>


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