Qpid
  1. Qpid
  2. QPID-3629

CPP Broker allows credit window to exceed the request size.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.14
    • Fix Version/s: 0.15
    • Component/s: C++ Broker
    • Labels:
      None

      Description

      Investigation of QPID-2703 revealed that the CPP Broker can be made to extend the window size beyond that requested by the client's message.flow command. To reproduce the problem, the client must perform the following steps:

      1) Receive messages
      2) Perform message stop
      3) Perform message flow
      4) Complete messages received by step 1)
      5) Observe the window is the sum of that requested by step 3 + the recredit of messages from 1)

      The window handling behaviour of the CPP Broker was the reason that QPID-2703 was not apparent against the CPP Broker but was against the Java Broker which does not expand the window in this way.

      The attached Python test demonstrates the issue by receiving more uncompleted message commands than the window should allow. Obviously it can be argued that the python test does not have the expected order of commands, but there is nothing stopping a client from performing such a sequence (e.g. the Qpid Java Client) so the broker should enforce the requested window size regardless.

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

          (Updated 2011-11-18 21:03:35.365601)

          Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, and Keith Wall.

          Changes
          -------

          Got the JIRA numbers muddled first time round; now fixed.

          Summary (updated)
          -------

          The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before).

          The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands.

          This addresses bug QPID-3629.
          https://issues.apache.org/jira/browse/QPID-3629

          Diffs


          /trunk/qpid/cpp/src/CMakeLists.txt 1203231
          /trunk/qpid/cpp/src/Makefile.am 1203231
          /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION
          /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION
          /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231
          /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231
          /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231
          /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231
          /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231
          /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231
          /trunk/qpid/cpp/xml/cluster.xml 1203231
          /trunk/qpid/python/qpid/testlib.py 1203231
          /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231

          Diff: https://reviews.apache.org/r/2880/diff

          Testing
          -------

          New test from Keith included in the patch. All existing tests pass.

          Thanks,

          Gordon

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2880/ ----------------------------------------------------------- (Updated 2011-11-18 21:03:35.365601) Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, and Keith Wall. Changes ------- Got the JIRA numbers muddled first time round; now fixed. Summary (updated) ------- The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before). The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands. This addresses bug QPID-3629 . https://issues.apache.org/jira/browse/QPID-3629 Diffs /trunk/qpid/cpp/src/CMakeLists.txt 1203231 /trunk/qpid/cpp/src/Makefile.am 1203231 /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231 /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231 /trunk/qpid/cpp/xml/cluster.xml 1203231 /trunk/qpid/python/qpid/testlib.py 1203231 /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231 Diff: https://reviews.apache.org/r/2880/diff Testing ------- New test from Keith included in the patch. All existing tests pass. Thanks, Gordon
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          /trunk/qpid/cpp/src/qpid/broker/Credit.cpp
          <https://reviews.apache.org/r/2880/#comment7591>

          Q: shouldn't we only set INFINITE_CREDIT if value == 0xFFFFFFFF explicitly? Otherwise, should we keep balance from ever getting > INFINITE_CREDIT - 1?

          /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp
          <https://reviews.apache.org/r/2880/#comment7592>

          Seems like we are no longer preserving the setting of INFINITE_CREDIT if we remove this check

          /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp
          <https://reviews.apache.org/r/2880/#comment7593>

          ditto

          /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp
          <https://reviews.apache.org/r/2880/#comment7594>

          ditto, etc...

          /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp
          <https://reviews.apache.org/r/2880/#comment7595>

          ditto tres...

          • Kenneth

          On 2011-11-18 21:03:35, Gordon Sim wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/2880/

          -----------------------------------------------------------

          (Updated 2011-11-18 21:03:35)

          Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, and Keith Wall.

          Summary

          -------

          The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before).

          The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands.

          This addresses bug QPID-3629.

          https://issues.apache.org/jira/browse/QPID-3629

          Diffs

          -----

          /trunk/qpid/cpp/src/CMakeLists.txt 1203231

          /trunk/qpid/cpp/src/Makefile.am 1203231

          /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION

          /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION

          /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231

          /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231

          /trunk/qpid/cpp/xml/cluster.xml 1203231

          /trunk/qpid/python/qpid/testlib.py 1203231

          /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231

          Diff: https://reviews.apache.org/r/2880/diff

          Testing

          -------

          New test from Keith included in the patch. All existing tests pass.

          Thanks,

          Gordon

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2880/#review3394 ----------------------------------------------------------- /trunk/qpid/cpp/src/qpid/broker/Credit.cpp < https://reviews.apache.org/r/2880/#comment7591 > Q: shouldn't we only set INFINITE_CREDIT if value == 0xFFFFFFFF explicitly? Otherwise, should we keep balance from ever getting > INFINITE_CREDIT - 1? /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp < https://reviews.apache.org/r/2880/#comment7592 > Seems like we are no longer preserving the setting of INFINITE_CREDIT if we remove this check /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp < https://reviews.apache.org/r/2880/#comment7593 > ditto /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp < https://reviews.apache.org/r/2880/#comment7594 > ditto, etc... /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp < https://reviews.apache.org/r/2880/#comment7595 > ditto tres... Kenneth On 2011-11-18 21:03:35, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2880/ ----------------------------------------------------------- (Updated 2011-11-18 21:03:35) Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, and Keith Wall. Summary ------- The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before). The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands. This addresses bug QPID-3629 . https://issues.apache.org/jira/browse/QPID-3629 Diffs ----- /trunk/qpid/cpp/src/CMakeLists.txt 1203231 /trunk/qpid/cpp/src/Makefile.am 1203231 /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231 /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231 /trunk/qpid/cpp/xml/cluster.xml 1203231 /trunk/qpid/python/qpid/testlib.py 1203231 /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231 Diff: https://reviews.apache.org/r/2880/diff Testing ------- New test from Keith included in the patch. All existing tests pass. Thanks, Gordon
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2011-11-21 15:03:45, Kenneth Giusti wrote:

          > /trunk/qpid/cpp/src/qpid/broker/Credit.cpp, line 36

          > <https://reviews.apache.org/r/2880/diff/2/?file=59378#file59378line36>

          >

          > Q: shouldn't we only set INFINITE_CREDIT if value == 0xFFFFFFFF explicitly? Otherwise, should we keep balance from ever getting > INFINITE_CREDIT - 1?

          >

          Yes, that would probably be better.

          On 2011-11-21 15:03:45, Kenneth Giusti wrote:

          > /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp, line 390

          > <https://reviews.apache.org/r/2880/diff/2/?file=59380#file59380line390>

          >

          > Seems like we are no longer preserving the setting of INFINITE_CREDIT if we remove this check

          Quite right, well spotted! I will fix that...

          • Gordon

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

          On 2011-11-18 21:03:35, Gordon Sim wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/2880/

          -----------------------------------------------------------

          (Updated 2011-11-18 21:03:35)

          Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, and Keith Wall.

          Summary

          -------

          The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before).

          The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands.

          This addresses bug QPID-3629.

          https://issues.apache.org/jira/browse/QPID-3629

          Diffs

          -----

          /trunk/qpid/cpp/src/CMakeLists.txt 1203231

          /trunk/qpid/cpp/src/Makefile.am 1203231

          /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION

          /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION

          /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231

          /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231

          /trunk/qpid/cpp/xml/cluster.xml 1203231

          /trunk/qpid/python/qpid/testlib.py 1203231

          /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231

          Diff: https://reviews.apache.org/r/2880/diff

          Testing

          -------

          New test from Keith included in the patch. All existing tests pass.

          Thanks,

          Gordon

          Show
          jiraposter@reviews.apache.org added a comment - On 2011-11-21 15:03:45, Kenneth Giusti wrote: > /trunk/qpid/cpp/src/qpid/broker/Credit.cpp, line 36 > < https://reviews.apache.org/r/2880/diff/2/?file=59378#file59378line36 > > > Q: shouldn't we only set INFINITE_CREDIT if value == 0xFFFFFFFF explicitly? Otherwise, should we keep balance from ever getting > INFINITE_CREDIT - 1? > Yes, that would probably be better. On 2011-11-21 15:03:45, Kenneth Giusti wrote: > /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp, line 390 > < https://reviews.apache.org/r/2880/diff/2/?file=59380#file59380line390 > > > Seems like we are no longer preserving the setting of INFINITE_CREDIT if we remove this check Quite right, well spotted! I will fix that... Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2880/#review3394 ----------------------------------------------------------- On 2011-11-18 21:03:35, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2880/ ----------------------------------------------------------- (Updated 2011-11-18 21:03:35) Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, and Keith Wall. Summary ------- The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before). The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands. This addresses bug QPID-3629 . https://issues.apache.org/jira/browse/QPID-3629 Diffs ----- /trunk/qpid/cpp/src/CMakeLists.txt 1203231 /trunk/qpid/cpp/src/Makefile.am 1203231 /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231 /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231 /trunk/qpid/cpp/xml/cluster.xml 1203231 /trunk/qpid/python/qpid/testlib.py 1203231 /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231 Diff: https://reviews.apache.org/r/2880/diff Testing ------- New test from Keith included in the patch. All existing tests pass. Thanks, Gordon
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          (Updated 2011-11-22 12:46:17.742722)

          Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, and Keith Wall.

          Changes
          -------

          Addressed Ken's points and removed false distinction between clear() and drain().

          Summary
          -------

          The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before).

          The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands.

          This addresses bug QPID-3629.
          https://issues.apache.org/jira/browse/QPID-3629

          Diffs (updated)


          /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231
          /trunk/qpid/cpp/src/CMakeLists.txt 1203231
          /trunk/qpid/cpp/src/Makefile.am 1203231
          /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION
          /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION
          /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231
          /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231
          /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231
          /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231
          /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231
          /trunk/qpid/cpp/src/tests/ClientSessionTest.cpp 1203231
          /trunk/qpid/cpp/xml/cluster.xml 1203231
          /trunk/qpid/python/qpid/testlib.py 1203231
          /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231

          Diff: https://reviews.apache.org/r/2880/diff

          Testing
          -------

          New test from Keith included in the patch. All existing tests pass.

          Thanks,

          Gordon

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2880/ ----------------------------------------------------------- (Updated 2011-11-22 12:46:17.742722) Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, and Keith Wall. Changes ------- Addressed Ken's points and removed false distinction between clear() and drain(). Summary ------- The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before). The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands. This addresses bug QPID-3629 . https://issues.apache.org/jira/browse/QPID-3629 Diffs (updated) /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231 /trunk/qpid/cpp/src/CMakeLists.txt 1203231 /trunk/qpid/cpp/src/Makefile.am 1203231 /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231 /trunk/qpid/cpp/src/tests/ClientSessionTest.cpp 1203231 /trunk/qpid/cpp/xml/cluster.xml 1203231 /trunk/qpid/python/qpid/testlib.py 1203231 /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231 Diff: https://reviews.apache.org/r/2880/diff Testing ------- New test from Keith included in the patch. All existing tests pass. Thanks, Gordon
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Ship it!

          /trunk/qpid/cpp/src/qpid/broker/Credit.cpp
          <https://reviews.apache.org/r/2880/#comment7671>

          Just for consideration - would it make sense to check for balance+value wrap, and pin balance to INFINITE_CREDIT-1 rather than use the wrapped value?

          • Kenneth

          On 2011-11-22 12:46:17, Gordon Sim wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/2880/

          -----------------------------------------------------------

          (Updated 2011-11-22 12:46:17)

          Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, and Keith Wall.

          Summary

          -------

          The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before).

          The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands.

          This addresses bug QPID-3629.

          https://issues.apache.org/jira/browse/QPID-3629

          Diffs

          -----

          /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231

          /trunk/qpid/cpp/src/CMakeLists.txt 1203231

          /trunk/qpid/cpp/src/Makefile.am 1203231

          /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION

          /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION

          /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231

          /trunk/qpid/cpp/src/tests/ClientSessionTest.cpp 1203231

          /trunk/qpid/cpp/xml/cluster.xml 1203231

          /trunk/qpid/python/qpid/testlib.py 1203231

          /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231

          Diff: https://reviews.apache.org/r/2880/diff

          Testing

          -------

          New test from Keith included in the patch. All existing tests pass.

          Thanks,

          Gordon

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2880/#review3430 ----------------------------------------------------------- Ship it! /trunk/qpid/cpp/src/qpid/broker/Credit.cpp < https://reviews.apache.org/r/2880/#comment7671 > Just for consideration - would it make sense to check for balance+value wrap, and pin balance to INFINITE_CREDIT-1 rather than use the wrapped value? Kenneth On 2011-11-22 12:46:17, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2880/ ----------------------------------------------------------- (Updated 2011-11-22 12:46:17) Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, and Keith Wall. Summary ------- The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before). The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands. This addresses bug QPID-3629 . https://issues.apache.org/jira/browse/QPID-3629 Diffs ----- /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231 /trunk/qpid/cpp/src/CMakeLists.txt 1203231 /trunk/qpid/cpp/src/Makefile.am 1203231 /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231 /trunk/qpid/cpp/src/tests/ClientSessionTest.cpp 1203231 /trunk/qpid/cpp/xml/cluster.xml 1203231 /trunk/qpid/python/qpid/testlib.py 1203231 /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231 Diff: https://reviews.apache.org/r/2880/diff Testing ------- New test from Keith included in the patch. All existing tests pass. Thanks, Gordon
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2011-11-22 14:34:00, Kenneth Giusti wrote:

          > /trunk/qpid/cpp/src/qpid/broker/Credit.cpp, line 38

          > <https://reviews.apache.org/r/2880/diff/2-3/?file=59378#file59378line38>

          >

          > Just for consideration - would it make sense to check for balance+value wrap, and pin balance to INFINITE_CREDIT-1 rather than use the wrapped value?

          Not sure I follow you, could you elaborate a little?

          • Gordon

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

          On 2011-11-22 12:46:17, Gordon Sim wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/2880/

          -----------------------------------------------------------

          (Updated 2011-11-22 12:46:17)

          Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, and Keith Wall.

          Summary

          -------

          The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before).

          The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands.

          This addresses bug QPID-3629.

          https://issues.apache.org/jira/browse/QPID-3629

          Diffs

          -----

          /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231

          /trunk/qpid/cpp/src/CMakeLists.txt 1203231

          /trunk/qpid/cpp/src/Makefile.am 1203231

          /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION

          /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION

          /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231

          /trunk/qpid/cpp/src/tests/ClientSessionTest.cpp 1203231

          /trunk/qpid/cpp/xml/cluster.xml 1203231

          /trunk/qpid/python/qpid/testlib.py 1203231

          /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231

          Diff: https://reviews.apache.org/r/2880/diff

          Testing

          -------

          New test from Keith included in the patch. All existing tests pass.

          Thanks,

          Gordon

          Show
          jiraposter@reviews.apache.org added a comment - On 2011-11-22 14:34:00, Kenneth Giusti wrote: > /trunk/qpid/cpp/src/qpid/broker/Credit.cpp, line 38 > < https://reviews.apache.org/r/2880/diff/2-3/?file=59378#file59378line38 > > > Just for consideration - would it make sense to check for balance+value wrap, and pin balance to INFINITE_CREDIT-1 rather than use the wrapped value? Not sure I follow you, could you elaborate a little? Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2880/#review3430 ----------------------------------------------------------- On 2011-11-22 12:46:17, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2880/ ----------------------------------------------------------- (Updated 2011-11-22 12:46:17) Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, and Keith Wall. Summary ------- The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before). The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands. This addresses bug QPID-3629 . https://issues.apache.org/jira/browse/QPID-3629 Diffs ----- /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231 /trunk/qpid/cpp/src/CMakeLists.txt 1203231 /trunk/qpid/cpp/src/Makefile.am 1203231 /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231 /trunk/qpid/cpp/src/tests/ClientSessionTest.cpp 1203231 /trunk/qpid/cpp/xml/cluster.xml 1203231 /trunk/qpid/python/qpid/testlib.py 1203231 /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231 Diff: https://reviews.apache.org/r/2880/diff Testing ------- New test from Keith included in the patch. All existing tests pass. Thanks, Gordon
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          (Updated 2011-11-22 14:55:19.940580)

          Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, rajith attapattu, and Keith Wall.

          Summary
          -------

          The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before).

          The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands.

          This addresses bug QPID-3629.
          https://issues.apache.org/jira/browse/QPID-3629

          Diffs


          /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231
          /trunk/qpid/cpp/src/CMakeLists.txt 1203231
          /trunk/qpid/cpp/src/Makefile.am 1203231
          /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION
          /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION
          /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231
          /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231
          /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231
          /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231
          /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231
          /trunk/qpid/cpp/src/tests/ClientSessionTest.cpp 1203231
          /trunk/qpid/cpp/xml/cluster.xml 1203231
          /trunk/qpid/python/qpid/testlib.py 1203231
          /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231

          Diff: https://reviews.apache.org/r/2880/diff

          Testing
          -------

          New test from Keith included in the patch. All existing tests pass.

          Thanks,

          Gordon

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2880/ ----------------------------------------------------------- (Updated 2011-11-22 14:55:19.940580) Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, rajith attapattu, and Keith Wall. Summary ------- The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before). The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands. This addresses bug QPID-3629 . https://issues.apache.org/jira/browse/QPID-3629 Diffs /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231 /trunk/qpid/cpp/src/CMakeLists.txt 1203231 /trunk/qpid/cpp/src/Makefile.am 1203231 /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231 /trunk/qpid/cpp/src/tests/ClientSessionTest.cpp 1203231 /trunk/qpid/cpp/xml/cluster.xml 1203231 /trunk/qpid/python/qpid/testlib.py 1203231 /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231 Diff: https://reviews.apache.org/r/2880/diff Testing ------- New test from Keith included in the patch. All existing tests pass. Thanks, Gordon
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2011-11-22 14:34:00, Kenneth Giusti wrote:

          > /trunk/qpid/cpp/src/qpid/broker/Credit.cpp, line 38

          > <https://reviews.apache.org/r/2880/diff/2-3/?file=59378#file59378line38>

          >

          > Just for consideration - would it make sense to check for balance+value wrap, and pin balance to INFINITE_CREDIT-1 rather than use the wrapped value?

          Gordon Sim wrote:

          Not sure I follow you, could you elaborate a little?

          I'm probably not understanding the credit granting constraints - but I' wondering if it's more correct to wrap the balance or pin the balance. e.g.:

          if (value == INFINITE_CREDIT)
          balance = INFINITE_CREDIT
          else if (value + balance < balance) // wrap
          balance = INFINITE_CREDIT - 1
          else balance += value

          I'm ok with the proposed solution, I'm just uncomfortable when absolute quantities wrap in general....

          • Kenneth

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

          On 2011-11-22 14:55:19, Gordon Sim wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/2880/

          -----------------------------------------------------------

          (Updated 2011-11-22 14:55:19)

          Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, rajith attapattu, and Keith Wall.

          Summary

          -------

          The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before).

          The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands.

          This addresses bug QPID-3629.

          https://issues.apache.org/jira/browse/QPID-3629

          Diffs

          -----

          /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231

          /trunk/qpid/cpp/src/CMakeLists.txt 1203231

          /trunk/qpid/cpp/src/Makefile.am 1203231

          /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION

          /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION

          /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231

          /trunk/qpid/cpp/src/tests/ClientSessionTest.cpp 1203231

          /trunk/qpid/cpp/xml/cluster.xml 1203231

          /trunk/qpid/python/qpid/testlib.py 1203231

          /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231

          Diff: https://reviews.apache.org/r/2880/diff

          Testing

          -------

          New test from Keith included in the patch. All existing tests pass.

          Thanks,

          Gordon

          Show
          jiraposter@reviews.apache.org added a comment - On 2011-11-22 14:34:00, Kenneth Giusti wrote: > /trunk/qpid/cpp/src/qpid/broker/Credit.cpp, line 38 > < https://reviews.apache.org/r/2880/diff/2-3/?file=59378#file59378line38 > > > Just for consideration - would it make sense to check for balance+value wrap, and pin balance to INFINITE_CREDIT-1 rather than use the wrapped value? Gordon Sim wrote: Not sure I follow you, could you elaborate a little? I'm probably not understanding the credit granting constraints - but I' wondering if it's more correct to wrap the balance or pin the balance. e.g.: if (value == INFINITE_CREDIT) balance = INFINITE_CREDIT else if (value + balance < balance) // wrap balance = INFINITE_CREDIT - 1 else balance += value I'm ok with the proposed solution, I'm just uncomfortable when absolute quantities wrap in general.... Kenneth ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2880/#review3430 ----------------------------------------------------------- On 2011-11-22 14:55:19, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2880/ ----------------------------------------------------------- (Updated 2011-11-22 14:55:19) Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, rajith attapattu, and Keith Wall. Summary ------- The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before). The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands. This addresses bug QPID-3629 . https://issues.apache.org/jira/browse/QPID-3629 Diffs ----- /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231 /trunk/qpid/cpp/src/CMakeLists.txt 1203231 /trunk/qpid/cpp/src/Makefile.am 1203231 /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231 /trunk/qpid/cpp/src/tests/ClientSessionTest.cpp 1203231 /trunk/qpid/cpp/xml/cluster.xml 1203231 /trunk/qpid/python/qpid/testlib.py 1203231 /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231 Diff: https://reviews.apache.org/r/2880/diff Testing ------- New test from Keith included in the patch. All existing tests pass. Thanks, Gordon
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2011-11-22 14:34:00, Kenneth Giusti wrote:

          > /trunk/qpid/cpp/src/qpid/broker/Credit.cpp, line 38

          > <https://reviews.apache.org/r/2880/diff/2-3/?file=59378#file59378line38>

          >

          > Just for consideration - would it make sense to check for balance+value wrap, and pin balance to INFINITE_CREDIT-1 rather than use the wrapped value?

          Gordon Sim wrote:

          Not sure I follow you, could you elaborate a little?

          Kenneth Giusti wrote:

          I'm probably not understanding the credit granting constraints - but I' wondering if it's more correct to wrap the balance or pin the balance. e.g.:

          if (value == INFINITE_CREDIT)

          balance = INFINITE_CREDIT

          else if (value + balance < balance) // wrap

          balance = INFINITE_CREDIT - 1

          else balance += value

          I'm ok with the proposed solution, I'm just uncomfortable when absolute quantities wrap in general....

          You are right as always! Better to pin the balance rather than let it wrap around. Fix coming up.

          • Gordon

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

          On 2011-11-22 14:55:19, Gordon Sim wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/2880/

          -----------------------------------------------------------

          (Updated 2011-11-22 14:55:19)

          Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, rajith attapattu, and Keith Wall.

          Summary

          -------

          The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before).

          The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands.

          This addresses bug QPID-3629.

          https://issues.apache.org/jira/browse/QPID-3629

          Diffs

          -----

          /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231

          /trunk/qpid/cpp/src/CMakeLists.txt 1203231

          /trunk/qpid/cpp/src/Makefile.am 1203231

          /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION

          /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION

          /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231

          /trunk/qpid/cpp/src/tests/ClientSessionTest.cpp 1203231

          /trunk/qpid/cpp/xml/cluster.xml 1203231

          /trunk/qpid/python/qpid/testlib.py 1203231

          /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231

          Diff: https://reviews.apache.org/r/2880/diff

          Testing

          -------

          New test from Keith included in the patch. All existing tests pass.

          Thanks,

          Gordon

          Show
          jiraposter@reviews.apache.org added a comment - On 2011-11-22 14:34:00, Kenneth Giusti wrote: > /trunk/qpid/cpp/src/qpid/broker/Credit.cpp, line 38 > < https://reviews.apache.org/r/2880/diff/2-3/?file=59378#file59378line38 > > > Just for consideration - would it make sense to check for balance+value wrap, and pin balance to INFINITE_CREDIT-1 rather than use the wrapped value? Gordon Sim wrote: Not sure I follow you, could you elaborate a little? Kenneth Giusti wrote: I'm probably not understanding the credit granting constraints - but I' wondering if it's more correct to wrap the balance or pin the balance. e.g.: if (value == INFINITE_CREDIT) balance = INFINITE_CREDIT else if (value + balance < balance) // wrap balance = INFINITE_CREDIT - 1 else balance += value I'm ok with the proposed solution, I'm just uncomfortable when absolute quantities wrap in general.... You are right as always! Better to pin the balance rather than let it wrap around. Fix coming up. Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2880/#review3430 ----------------------------------------------------------- On 2011-11-22 14:55:19, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2880/ ----------------------------------------------------------- (Updated 2011-11-22 14:55:19) Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, rajith attapattu, and Keith Wall. Summary ------- The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before). The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands. This addresses bug QPID-3629 . https://issues.apache.org/jira/browse/QPID-3629 Diffs ----- /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231 /trunk/qpid/cpp/src/CMakeLists.txt 1203231 /trunk/qpid/cpp/src/Makefile.am 1203231 /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231 /trunk/qpid/cpp/src/tests/ClientSessionTest.cpp 1203231 /trunk/qpid/cpp/xml/cluster.xml 1203231 /trunk/qpid/python/qpid/testlib.py 1203231 /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231 Diff: https://reviews.apache.org/r/2880/diff Testing ------- New test from Keith included in the patch. All existing tests pass. Thanks, Gordon
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          (Updated 2011-11-22 17:10:06.887022)

          Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, rajith attapattu, and Keith Wall.

          Changes
          -------

          Prevented wraparound of credit balance and added test to check for that. (Thanks again, Ken!)

          Summary
          -------

          The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before).

          The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands.

          This addresses bug QPID-3629.
          https://issues.apache.org/jira/browse/QPID-3629

          Diffs (updated)


          /trunk/qpid/cpp/src/CMakeLists.txt 1203231
          /trunk/qpid/cpp/src/Makefile.am 1203231
          /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION
          /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION
          /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231
          /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231
          /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231
          /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231
          /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231
          /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231
          /trunk/qpid/cpp/src/tests/ClientSessionTest.cpp 1203231
          /trunk/qpid/cpp/xml/cluster.xml 1203231
          /trunk/qpid/python/qpid/testlib.py 1203231
          /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231

          Diff: https://reviews.apache.org/r/2880/diff

          Testing
          -------

          New test from Keith included in the patch. All existing tests pass.

          Thanks,

          Gordon

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2880/ ----------------------------------------------------------- (Updated 2011-11-22 17:10:06.887022) Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, rajith attapattu, and Keith Wall. Changes ------- Prevented wraparound of credit balance and added test to check for that. (Thanks again, Ken!) Summary ------- The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before). The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands. This addresses bug QPID-3629 . https://issues.apache.org/jira/browse/QPID-3629 Diffs (updated) /trunk/qpid/cpp/src/CMakeLists.txt 1203231 /trunk/qpid/cpp/src/Makefile.am 1203231 /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231 /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231 /trunk/qpid/cpp/src/tests/ClientSessionTest.cpp 1203231 /trunk/qpid/cpp/xml/cluster.xml 1203231 /trunk/qpid/python/qpid/testlib.py 1203231 /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231 Diff: https://reviews.apache.org/r/2880/diff Testing ------- New test from Keith included in the patch. All existing tests pass. Thanks, Gordon
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Ship it!

          • Kenneth

          On 2011-11-22 17:10:06, Gordon Sim wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/2880/

          -----------------------------------------------------------

          (Updated 2011-11-22 17:10:06)

          Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, rajith attapattu, and Keith Wall.

          Summary

          -------

          The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before).

          The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands.

          This addresses bug QPID-3629.

          https://issues.apache.org/jira/browse/QPID-3629

          Diffs

          -----

          /trunk/qpid/cpp/src/CMakeLists.txt 1203231

          /trunk/qpid/cpp/src/Makefile.am 1203231

          /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION

          /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION

          /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231

          /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231

          /trunk/qpid/cpp/src/tests/ClientSessionTest.cpp 1203231

          /trunk/qpid/cpp/xml/cluster.xml 1203231

          /trunk/qpid/python/qpid/testlib.py 1203231

          /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231

          Diff: https://reviews.apache.org/r/2880/diff

          Testing

          -------

          New test from Keith included in the patch. All existing tests pass.

          Thanks,

          Gordon

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2880/#review3440 ----------------------------------------------------------- Ship it! Kenneth On 2011-11-22 17:10:06, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2880/ ----------------------------------------------------------- (Updated 2011-11-22 17:10:06) Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, rajith attapattu, and Keith Wall. Summary ------- The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before). The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands. This addresses bug QPID-3629 . https://issues.apache.org/jira/browse/QPID-3629 Diffs ----- /trunk/qpid/cpp/src/CMakeLists.txt 1203231 /trunk/qpid/cpp/src/Makefile.am 1203231 /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231 /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231 /trunk/qpid/cpp/src/tests/ClientSessionTest.cpp 1203231 /trunk/qpid/cpp/xml/cluster.xml 1203231 /trunk/qpid/python/qpid/testlib.py 1203231 /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231 Diff: https://reviews.apache.org/r/2880/diff Testing ------- New test from Keith included in the patch. All existing tests pass. Thanks, Gordon
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          There is good news and bad news.
          This patch makes sure that the number of messages in the clients prefetch buffer never exceeds the max-prefetch value.
          So this fix is very important for QPID-3602

          The bad news is that this patch seems to not send messages to the client even when there are messages in the queue.
          This could be due to,
          1. An error introduced by the patch.
          2. The patch has unearthed a client side issue that went unnoticed before.

          At this point I'm not sure which one it is .
          I can easily reproduce this issue by running against a broker with the patch applied. This never happens without the patch.

          I have attached my test case to https://issues.apache.org/jira/browse/QPID-3640

          The client I'm using is with some patches I've worked for QPID-3602
          I've attached those patches in a private email to you. If you have trouble applying those patches, I've got the client and common jar attached with the same email.
          You can just drop them into your classpath and run it.

          I will continue to investigate this issue and will update if I come across something.

          • rajith

          On 2011-11-22 17:10:06, Gordon Sim wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/2880/

          -----------------------------------------------------------

          (Updated 2011-11-22 17:10:06)

          Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, rajith attapattu, and Keith Wall.

          Summary

          -------

          The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before).

          The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands.

          This addresses bug QPID-3629.

          https://issues.apache.org/jira/browse/QPID-3629

          Diffs

          -----

          /trunk/qpid/cpp/src/CMakeLists.txt 1203231

          /trunk/qpid/cpp/src/Makefile.am 1203231

          /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION

          /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION

          /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231

          /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231

          /trunk/qpid/cpp/src/tests/ClientSessionTest.cpp 1203231

          /trunk/qpid/cpp/xml/cluster.xml 1203231

          /trunk/qpid/python/qpid/testlib.py 1203231

          /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231

          Diff: https://reviews.apache.org/r/2880/diff

          Testing

          -------

          New test from Keith included in the patch. All existing tests pass.

          Thanks,

          Gordon

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2880/#review3445 ----------------------------------------------------------- There is good news and bad news. This patch makes sure that the number of messages in the clients prefetch buffer never exceeds the max-prefetch value. So this fix is very important for QPID-3602 The bad news is that this patch seems to not send messages to the client even when there are messages in the queue. This could be due to, 1. An error introduced by the patch. 2. The patch has unearthed a client side issue that went unnoticed before. At this point I'm not sure which one it is . I can easily reproduce this issue by running against a broker with the patch applied. This never happens without the patch. I have attached my test case to https://issues.apache.org/jira/browse/QPID-3640 The client I'm using is with some patches I've worked for QPID-3602 I've attached those patches in a private email to you. If you have trouble applying those patches, I've got the client and common jar attached with the same email. You can just drop them into your classpath and run it. I will continue to investigate this issue and will update if I come across something. rajith On 2011-11-22 17:10:06, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2880/ ----------------------------------------------------------- (Updated 2011-11-22 17:10:06) Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, rajith attapattu, and Keith Wall. Summary ------- The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before). The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands. This addresses bug QPID-3629 . https://issues.apache.org/jira/browse/QPID-3629 Diffs ----- /trunk/qpid/cpp/src/CMakeLists.txt 1203231 /trunk/qpid/cpp/src/Makefile.am 1203231 /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231 /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231 /trunk/qpid/cpp/src/tests/ClientSessionTest.cpp 1203231 /trunk/qpid/cpp/xml/cluster.xml 1203231 /trunk/qpid/python/qpid/testlib.py 1203231 /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231 Diff: https://reviews.apache.org/r/2880/diff Testing ------- New test from Keith included in the patch. All existing tests pass. Thanks, Gordon
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2011-11-22 22:34:38, rajith attapattu wrote:

          > There is good news and bad news.

          > This patch makes sure that the number of messages in the clients prefetch buffer never exceeds the max-prefetch value.

          > So this fix is very important for QPID-3602

          >

          > The bad news is that this patch seems to not send messages to the client even when there are messages in the queue.

          > This could be due to,

          > 1. An error introduced by the patch.

          > 2. The patch has unearthed a client side issue that went unnoticed before.

          >

          > At this point I'm not sure which one it is .

          > I can easily reproduce this issue by running against a broker with the patch applied. This never happens without the patch.

          >

          > I have attached my test case to https://issues.apache.org/jira/browse/QPID-3640

          >

          > The client I'm using is with some patches I've worked for QPID-3602

          > I've attached those patches in a private email to you. If you have trouble applying those patches, I've got the client and common jar attached with the same email.

          > You can just drop them into your classpath and run it.

          >

          > I will continue to investigate this issue and will update if I come across something.

          The client is not issuing session-completed controls correctly. There are un-completed messages at the point the message-stop and subsequent message-flows are issued, equal in number to the window size, so until these are completed no further messages are sent. The test cases where this is observed fail in the same manner against the java broker.

          Regarding the number of redelivered messages, that number matches exactly the number of message-release commands issued by the client with the redelivered-flag set to 1. I.e. it is as expected from the broker side.

          • Gordon

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

          On 2011-11-22 17:10:06, Gordon Sim wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/2880/

          -----------------------------------------------------------

          (Updated 2011-11-22 17:10:06)

          Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, rajith attapattu, and Keith Wall.

          Summary

          -------

          The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before).

          The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands.

          This addresses bug QPID-3629.

          https://issues.apache.org/jira/browse/QPID-3629

          Diffs

          -----

          /trunk/qpid/cpp/src/CMakeLists.txt 1203231

          /trunk/qpid/cpp/src/Makefile.am 1203231

          /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION

          /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION

          /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231

          /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231

          /trunk/qpid/cpp/src/tests/ClientSessionTest.cpp 1203231

          /trunk/qpid/cpp/xml/cluster.xml 1203231

          /trunk/qpid/python/qpid/testlib.py 1203231

          /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231

          Diff: https://reviews.apache.org/r/2880/diff

          Testing

          -------

          New test from Keith included in the patch. All existing tests pass.

          Thanks,

          Gordon

          Show
          jiraposter@reviews.apache.org added a comment - On 2011-11-22 22:34:38, rajith attapattu wrote: > There is good news and bad news. > This patch makes sure that the number of messages in the clients prefetch buffer never exceeds the max-prefetch value. > So this fix is very important for QPID-3602 > > The bad news is that this patch seems to not send messages to the client even when there are messages in the queue. > This could be due to, > 1. An error introduced by the patch. > 2. The patch has unearthed a client side issue that went unnoticed before. > > At this point I'm not sure which one it is . > I can easily reproduce this issue by running against a broker with the patch applied. This never happens without the patch. > > I have attached my test case to https://issues.apache.org/jira/browse/QPID-3640 > > The client I'm using is with some patches I've worked for QPID-3602 > I've attached those patches in a private email to you. If you have trouble applying those patches, I've got the client and common jar attached with the same email. > You can just drop them into your classpath and run it. > > I will continue to investigate this issue and will update if I come across something. The client is not issuing session-completed controls correctly. There are un-completed messages at the point the message-stop and subsequent message-flows are issued, equal in number to the window size, so until these are completed no further messages are sent. The test cases where this is observed fail in the same manner against the java broker. Regarding the number of redelivered messages, that number matches exactly the number of message-release commands issued by the client with the redelivered-flag set to 1. I.e. it is as expected from the broker side. Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2880/#review3445 ----------------------------------------------------------- On 2011-11-22 17:10:06, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2880/ ----------------------------------------------------------- (Updated 2011-11-22 17:10:06) Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, rajith attapattu, and Keith Wall. Summary ------- The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before). The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands. This addresses bug QPID-3629 . https://issues.apache.org/jira/browse/QPID-3629 Diffs ----- /trunk/qpid/cpp/src/CMakeLists.txt 1203231 /trunk/qpid/cpp/src/Makefile.am 1203231 /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231 /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231 /trunk/qpid/cpp/src/tests/ClientSessionTest.cpp 1203231 /trunk/qpid/cpp/xml/cluster.xml 1203231 /trunk/qpid/python/qpid/testlib.py 1203231 /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231 Diff: https://reviews.apache.org/r/2880/diff Testing ------- New test from Keith included in the patch. All existing tests pass. Thanks, Gordon
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Ship it!

          /trunk/qpid/cpp/src/qpid/broker/Credit.cpp
          <https://reviews.apache.org/r/2880/#comment7740>

          Shouldn't this be an exception? You can't allocate the request credit so you can't fulfill your contract.

          • Alan

          On 2011-11-22 17:10:06, Gordon Sim wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/2880/

          -----------------------------------------------------------

          (Updated 2011-11-22 17:10:06)

          Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, rajith attapattu, and Keith Wall.

          Summary

          -------

          The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before).

          The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands.

          This addresses bug QPID-3629.

          https://issues.apache.org/jira/browse/QPID-3629

          Diffs

          -----

          /trunk/qpid/cpp/src/CMakeLists.txt 1203231

          /trunk/qpid/cpp/src/Makefile.am 1203231

          /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION

          /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION

          /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231

          /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231

          /trunk/qpid/cpp/src/tests/ClientSessionTest.cpp 1203231

          /trunk/qpid/cpp/xml/cluster.xml 1203231

          /trunk/qpid/python/qpid/testlib.py 1203231

          /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231

          Diff: https://reviews.apache.org/r/2880/diff

          Testing

          -------

          New test from Keith included in the patch. All existing tests pass.

          Thanks,

          Gordon

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2880/#review3473 ----------------------------------------------------------- Ship it! /trunk/qpid/cpp/src/qpid/broker/Credit.cpp < https://reviews.apache.org/r/2880/#comment7740 > Shouldn't this be an exception? You can't allocate the request credit so you can't fulfill your contract. Alan On 2011-11-22 17:10:06, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2880/ ----------------------------------------------------------- (Updated 2011-11-22 17:10:06) Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, rajith attapattu, and Keith Wall. Summary ------- The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before). The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands. This addresses bug QPID-3629 . https://issues.apache.org/jira/browse/QPID-3629 Diffs ----- /trunk/qpid/cpp/src/CMakeLists.txt 1203231 /trunk/qpid/cpp/src/Makefile.am 1203231 /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231 /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231 /trunk/qpid/cpp/src/tests/ClientSessionTest.cpp 1203231 /trunk/qpid/cpp/xml/cluster.xml 1203231 /trunk/qpid/python/qpid/testlib.py 1203231 /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231 Diff: https://reviews.apache.org/r/2880/diff Testing ------- New test from Keith included in the patch. All existing tests pass. Thanks, Gordon
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2011-11-23 15:03:37, Alan Conway wrote:

          > /trunk/qpid/cpp/src/qpid/broker/Credit.cpp, line 38

          > <https://reviews.apache.org/r/2880/diff/4/?file=59810#file59810line38>

          >

          > Shouldn't this be an exception? You can't allocate the request credit so you can't fulfill your contract.

          The spec doesn't define it as an error/exception - its very vague on details. I would argue that the fact that 0xFFFFFFFF is defined as an infinite amount of credit implies that you can only have a non-infinite balance of up to 0xFFFFFFFE, which is what this does. In the same way that adding credit to an infinite balance is simply ignored, I think it is reasonable to simply ignore requests to extend the non-infinite balance beyond what is a fairly obvious limit given the spec.

          In practice I think it unlikely that this bothers anyone either way. In theory I prefer to allow the session to continue, even if the allocated credit limit could conceivably be less than a bizarre interpretation of the spec could lead one to believe.

          • Gordon

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

          On 2011-11-22 17:10:06, Gordon Sim wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/2880/

          -----------------------------------------------------------

          (Updated 2011-11-22 17:10:06)

          Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, rajith attapattu, and Keith Wall.

          Summary

          -------

          The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before).

          The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands.

          This addresses bug QPID-3629.

          https://issues.apache.org/jira/browse/QPID-3629

          Diffs

          -----

          /trunk/qpid/cpp/src/CMakeLists.txt 1203231

          /trunk/qpid/cpp/src/Makefile.am 1203231

          /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION

          /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION

          /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231

          /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231

          /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231

          /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231

          /trunk/qpid/cpp/src/tests/ClientSessionTest.cpp 1203231

          /trunk/qpid/cpp/xml/cluster.xml 1203231

          /trunk/qpid/python/qpid/testlib.py 1203231

          /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231

          Diff: https://reviews.apache.org/r/2880/diff

          Testing

          -------

          New test from Keith included in the patch. All existing tests pass.

          Thanks,

          Gordon

          Show
          jiraposter@reviews.apache.org added a comment - On 2011-11-23 15:03:37, Alan Conway wrote: > /trunk/qpid/cpp/src/qpid/broker/Credit.cpp, line 38 > < https://reviews.apache.org/r/2880/diff/4/?file=59810#file59810line38 > > > Shouldn't this be an exception? You can't allocate the request credit so you can't fulfill your contract. The spec doesn't define it as an error/exception - its very vague on details. I would argue that the fact that 0xFFFFFFFF is defined as an infinite amount of credit implies that you can only have a non-infinite balance of up to 0xFFFFFFFE, which is what this does. In the same way that adding credit to an infinite balance is simply ignored, I think it is reasonable to simply ignore requests to extend the non-infinite balance beyond what is a fairly obvious limit given the spec. In practice I think it unlikely that this bothers anyone either way. In theory I prefer to allow the session to continue, even if the allocated credit limit could conceivably be less than a bizarre interpretation of the spec could lead one to believe. Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2880/#review3473 ----------------------------------------------------------- On 2011-11-22 17:10:06, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2880/ ----------------------------------------------------------- (Updated 2011-11-22 17:10:06) Review request for Alan Conway, Kenneth Giusti, Robbie Gemmell, rajith attapattu, and Keith Wall. Summary ------- The c++ broker does not track the actual credit window as distinct from the remaining credit. This means that when a message-stop is received followed by a message-flow, the total number of outstanding, uncompleted messages for the subscription is larger than the window. Fixing the corner cases requires tracking some extra state in window mode. This patch modifies thing s to track the window size and the current used credit in that case. (In explicit mode we just track the available credit as before). I've moved all the credit logic out into its own pair of files and attempted to reduce a lot of the repetitiveness (once the changes needed to address the issue in the JIRA, the code was even messier than before). The extra state also needs to be communicated to cluster updatees, so there is a small modification to the update process and one of the update commands. This addresses bug QPID-3629 . https://issues.apache.org/jira/browse/QPID-3629 Diffs ----- /trunk/qpid/cpp/src/CMakeLists.txt 1203231 /trunk/qpid/cpp/src/Makefile.am 1203231 /trunk/qpid/cpp/src/qpid/broker/Credit.h PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/Credit.cpp PRE-CREATION /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1203231 /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1203231 /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1203231 /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1203231 /trunk/qpid/cpp/src/tests/ClientSessionTest.cpp 1203231 /trunk/qpid/cpp/xml/cluster.xml 1203231 /trunk/qpid/python/qpid/testlib.py 1203231 /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/message.py 1203231 Diff: https://reviews.apache.org/r/2880/diff Testing ------- New test from Keith included in the patch. All existing tests pass. Thanks, Gordon

            People

            • Assignee:
              Gordon Sim
              Reporter:
              Keith Wall
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development