> 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. > > rajith attapattu wrote: > 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 ? > > Robbie Gemmell wrote: > >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. > > I dont think that is correct. There is a discussion on the mailing lists > at the moment about the behavioural differences in credit window sizing > between the Java and C++ brokers, and the initial outcome looks like making > the C++ broker move to doing what the Java broker does. In that case, it is > necessary for the client to complete all the commands during recover, or the > broker will simply consider that you have already used [some of] the new > credit you are issuing because you still havent completed the outstanding > messages (as very explicitly mentioned in the 0-10 spec, releasing etc a > message does not complete it). If it did anything else, you would effectively > be allowing the broker to breach the requested credit window size in certain > situations (as the C++ broker currently does).
I don't think it's correct either :D , hence me mentioning "But for correctness we should mark them complete". Yes I was following closely that thread and I agree that the C++ broker should do the same as the Java broker. - 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 > >
