----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2853/#review3325 -----------------------------------------------------------
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. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java <https://reviews.apache.org/r/2853/#comment7415> whitespace error http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java <https://reviews.apache.org/r/2853/#comment7416> another whitespace error http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java <https://reviews.apache.org/r/2853/#comment7417> another whitespace error http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java <https://reviews.apache.org/r/2853/#comment7418> I'll stop after this one...another whitespace error :) http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java <https://reviews.apache.org/r/2853/#comment7419> The change to this method mean it no longer needs to exist and should be removed. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java <https://reviews.apache.org/r/2853/#comment7420> Do we need to do this anymore considering that it just sends a completion, given the processCompletions call added on L420 ? http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/systests/src/main/java/org/apache/qpid/client/prefetch/PrefetchBehaviourTest.java <https://reviews.apache.org/r/2853/#comment7421> Some onMessage() tests would be good, given recent disparity between its behaviour and receive() for things like this. - Robbie 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 > >
