Qpid
  1. Qpid
  2. QPID-3602

Fix Consumer message credit issues in 0-10 codepath

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 0.14
    • Fix Version/s: Future
    • Component/s: Java Client
    • Labels:

      Description

      Currently there are several issues related to message credits.

      1. QPID-2604 - Getting more messages than required by the prefetch value.

      2. QPID-3604 - If connection is started and stopped, the client may get more messages than required by the prefetch value.

      3. QPID-3562 - Prefetch=1 case doesn't work properly.

      4. Prefetch-0 case doesn't work properly (well completely broken).

      5. QPID-3612 -Message credits are affected by Command Completions and not message-acks. However these two are intertwined in the logic causing some issues. For example when in client-ack mode or using transactions, if the client has exhausted the credits, but is waiting for more messages to come before it acks or commits a transaction, then the client will appear hung (This issue is currently masked due to some of the above bugs).

      6. QPID-3613 Credit should be managed on a per subscription basis than on a per session basis.

        Issue Links

          Activity

          Hide
          jiraposter@reviews.apache.org added a comment -

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

          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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2853/ ----------------------------------------------------------- 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
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- 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
          Hide
          jiraposter@reviews.apache.org added a comment -

          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

          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

          Show
          jiraposter@reviews.apache.org added a comment - 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 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
          Hide
          jiraposter@reviews.apache.org added a comment -

          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 ?

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

          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 ?

          rajith attapattu wrote:

          I think you are right good catch !

          But let me verify this.

          In retrospect, whilst you might not need to do that completion, you probably do need to do 'clever stuff' regarding recovery for the processCompletions() call.

          • Robbie

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

          Show
          jiraposter@reviews.apache.org added a comment - 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 ? 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). 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 ? rajith attapattu wrote: I think you are right good catch ! But let me verify this. In retrospect, whilst you might not need to do that completion, you probably do need to do 'clever stuff' regarding recovery for the processCompletions() call. Robbie ----------------------------------------------------------- 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
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Also forgot to mention earlier, I think that you will need to consider clearing the contents of your completions list during failover. When Keith and I made the changes we did, we had to update the patch to ensure that it didnt try completing messages after failover that had been received from the old broker and so would now be considered to have invalid command IDs based on the tracking the Session does.

          • 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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2853/#review3330 ----------------------------------------------------------- Also forgot to mention earlier, I think that you will need to consider clearing the contents of your completions list during failover. When Keith and I made the changes we did, we had to update the patch to ensure that it didnt try completing messages after failover that had been received from the old broker and so would now be considered to have invalid command IDs based on the tracking the Session does. 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
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Show
          jiraposter@reviews.apache.org added a comment - 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 , 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
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2011-11-17 22:50:45, Robbie Gemmell wrote:

          > Also forgot to mention earlier, I think that you will need to consider clearing the contents of your completions list during failover. When Keith and I made the changes we did, we had to update the patch to ensure that it didnt try completing messages after failover that had been received from the old broker and so would now be considered to have invalid command IDs based on the tracking the Session does.

          correct ! – this is something that I've missed. Thanks again for the feedback.
          Hope to repost the patch with the changes we've discussed late friday or monday morning.

          • rajith

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

          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

          Show
          jiraposter@reviews.apache.org added a comment - On 2011-11-17 22:50:45, Robbie Gemmell wrote: > Also forgot to mention earlier, I think that you will need to consider clearing the contents of your completions list during failover. When Keith and I made the changes we did, we had to update the patch to ensure that it didnt try completing messages after failover that had been received from the old broker and so would now be considered to have invalid command IDs based on the tracking the Session does. correct ! – this is something that I've missed. Thanks again for the feedback. Hope to repost the patch with the changes we've discussed late friday or monday morning. rajith ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2853/#review3330 ----------------------------------------------------------- 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
          Hide
          Rajith Attapattu added a comment -

          There is an issue with the broker where it sends more messages than it should. A fix for the above issue is important to provide a complete solution to QPID-3602

          Show
          Rajith Attapattu added a comment - There is an issue with the broker where it sends more messages than it should. A fix for the above issue is important to provide a complete solution to QPID-3602

            People

            • Assignee:
              Rajith Attapattu
              Reporter:
              Rajith Attapattu
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:

                Development