Qpid
  1. Qpid
  2. QPID-3252

Regression: broker no longer explicitly flushes messages to store on execution.sync or when sync command bit set.

    Details

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

      Description

      In the previous release of the broker, an execution.sync command, or a message.transfer command with a sync flag would cause the broker to explicitly flush the message(s) and pend until the flushes complete. In the new asynchronous model, the broker does not initiate a flush under these scenarios. Without explicitly flushing, the completion of a message.transfer/execution.sync may delay for an unacceptable amount of time (ex. 1sec+) when used with store.

        Issue Links

          Activity

          Hide
          Ken Giusti added a comment -

          Fix completed with patch to QPID-3394

          Show
          Ken Giusti added a comment - Fix completed with patch to QPID-3394
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Ship it!

          • Gordon

          On 2011-05-10 21:25:11, Kenneth Giusti wrote:

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

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

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

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

          (Updated 2011-05-10 21:25:11)

          Review request for qpid, Alan Conway, Gordon Sim, and Kim van der Riet.

          Summary

          -------

          Broker now issues an explicit flush for each received message that has the sync flag set in the message.transfer command.

          Broker also tracks all messages received that do not have the sync flag set, and will flush these messages on receipt of an execution.sync.

          This addresses bug QPID-3252.

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

          Diffs

          -----

          /trunk/qpid/cpp/src/qpid/broker/SessionState.h 1101542

          /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1101542

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

          Testing

          -------

          qpid unit & store unit

          Thanks,

          Kenneth

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/701/#review664 ----------------------------------------------------------- Ship it! Gordon On 2011-05-10 21:25:11, Kenneth Giusti wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/701/ ----------------------------------------------------------- (Updated 2011-05-10 21:25:11) Review request for qpid, Alan Conway, Gordon Sim, and Kim van der Riet. Summary ------- Broker now issues an explicit flush for each received message that has the sync flag set in the message.transfer command. Broker also tracks all messages received that do not have the sync flag set, and will flush these messages on receipt of an execution.sync. This addresses bug QPID-3252 . https://issues.apache.org/jira/browse/QPID-3252 Diffs ----- /trunk/qpid/cpp/src/qpid/broker/SessionState.h 1101542 /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1101542 Diff: https://reviews.apache.org/r/701/diff Testing ------- qpid unit & store unit Thanks, Kenneth
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          (Updated 2011-05-10 21:25:11.588275)

          Review request for qpid, Alan Conway, Gordon Sim, and Kim van der Riet.

          Changes
          -------

          This refactor brings transient traffic performance back to pre-patch levels. I've moved the whole flush-or-save logic into the clone() callback which is invoked only if the message.transfer cannot be completed asynchronously. This removes it from the transient fast-path.

          Let me know what you think.

          Summary
          -------

          Broker now issues an explicit flush for each received message that has the sync flag set in the message.transfer command.
          Broker also tracks all messages received that do not have the sync flag set, and will flush these messages on receipt of an execution.sync.

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

          Diffs (updated)


          /trunk/qpid/cpp/src/qpid/broker/SessionState.h 1101542
          /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1101542

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

          Testing
          -------

          qpid unit & store unit

          Thanks,

          Kenneth

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/701/ ----------------------------------------------------------- (Updated 2011-05-10 21:25:11.588275) Review request for qpid, Alan Conway, Gordon Sim, and Kim van der Riet. Changes ------- This refactor brings transient traffic performance back to pre-patch levels. I've moved the whole flush-or-save logic into the clone() callback which is invoked only if the message.transfer cannot be completed asynchronously. This removes it from the transient fast-path. Let me know what you think. Summary ------- Broker now issues an explicit flush for each received message that has the sync flag set in the message.transfer command. Broker also tracks all messages received that do not have the sync flag set, and will flush these messages on receipt of an execution.sync. This addresses bug QPID-3252 . https://issues.apache.org/jira/browse/QPID-3252 Diffs (updated) /trunk/qpid/cpp/src/qpid/broker/SessionState.h 1101542 /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1101542 Diff: https://reviews.apache.org/r/701/diff Testing ------- qpid unit & store unit Thanks, Kenneth
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2011-05-10 14:36:31, Alan Conway wrote:

          > Does this affect performance at all?

          > Looks ok to me but I'm not up on why we need to flush here.

          Performance?

          Dang you, Conway! Turns out it does affect the performance of the transient case - about 10% hit.

          I've got a refactored patch that avoids any unnecessary overhead in the path where messages complete synchronously. I'll upload that now.

          • Kenneth

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

          On 2011-05-09 21:22:38, Kenneth Giusti wrote:

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

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

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

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

          (Updated 2011-05-09 21:22:38)

          Review request for qpid, Alan Conway, Gordon Sim, and Kim van der Riet.

          Summary

          -------

          Broker now issues an explicit flush for each received message that has the sync flag set in the message.transfer command.

          Broker also tracks all messages received that do not have the sync flag set, and will flush these messages on receipt of an execution.sync.

          This addresses bug QPID-3252.

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

          Diffs

          -----

          /trunk/qpid/cpp/src/qpid/broker/SessionState.h 1101206

          /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1101206

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

          Testing

          -------

          qpid unit & store unit

          Thanks,

          Kenneth

          Show
          jiraposter@reviews.apache.org added a comment - On 2011-05-10 14:36:31, Alan Conway wrote: > Does this affect performance at all? > Looks ok to me but I'm not up on why we need to flush here. Performance? Dang you, Conway! Turns out it does affect the performance of the transient case - about 10% hit. I've got a refactored patch that avoids any unnecessary overhead in the path where messages complete synchronously. I'll upload that now. Kenneth ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/701/#review657 ----------------------------------------------------------- On 2011-05-09 21:22:38, Kenneth Giusti wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/701/ ----------------------------------------------------------- (Updated 2011-05-09 21:22:38) Review request for qpid, Alan Conway, Gordon Sim, and Kim van der Riet. Summary ------- Broker now issues an explicit flush for each received message that has the sync flag set in the message.transfer command. Broker also tracks all messages received that do not have the sync flag set, and will flush these messages on receipt of an execution.sync. This addresses bug QPID-3252 . https://issues.apache.org/jira/browse/QPID-3252 Diffs ----- /trunk/qpid/cpp/src/qpid/broker/SessionState.h 1101206 /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1101206 Diff: https://reviews.apache.org/r/701/diff Testing ------- qpid unit & store unit Thanks, Kenneth
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Does this affect performance at all?
          Looks ok to me but I'm not up on why we need to flush here.

          • Alan

          On 2011-05-09 21:22:38, Kenneth Giusti wrote:

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

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

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

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

          (Updated 2011-05-09 21:22:38)

          Review request for qpid, Alan Conway, Gordon Sim, and Kim van der Riet.

          Summary

          -------

          Broker now issues an explicit flush for each received message that has the sync flag set in the message.transfer command.

          Broker also tracks all messages received that do not have the sync flag set, and will flush these messages on receipt of an execution.sync.

          This addresses bug QPID-3252.

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

          Diffs

          -----

          /trunk/qpid/cpp/src/qpid/broker/SessionState.h 1101206

          /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1101206

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

          Testing

          -------

          qpid unit & store unit

          Thanks,

          Kenneth

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/701/#review657 ----------------------------------------------------------- Does this affect performance at all? Looks ok to me but I'm not up on why we need to flush here. Alan On 2011-05-09 21:22:38, Kenneth Giusti wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/701/ ----------------------------------------------------------- (Updated 2011-05-09 21:22:38) Review request for qpid, Alan Conway, Gordon Sim, and Kim van der Riet. Summary ------- Broker now issues an explicit flush for each received message that has the sync flag set in the message.transfer command. Broker also tracks all messages received that do not have the sync flag set, and will flush these messages on receipt of an execution.sync. This addresses bug QPID-3252 . https://issues.apache.org/jira/browse/QPID-3252 Diffs ----- /trunk/qpid/cpp/src/qpid/broker/SessionState.h 1101206 /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1101206 Diff: https://reviews.apache.org/r/701/diff Testing ------- qpid unit & store unit Thanks, Kenneth
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2011-05-10 08:35:04, Gordon Sim wrote:

          > /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp, line 538

          > <https://reviews.apache.org/r/701/diff/1/?file=18450#file18450line538>

          >

          > Maybe use swap() here?

          Good point - done!

          • Kenneth

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

          On 2011-05-09 21:22:38, Kenneth Giusti wrote:

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

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

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

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

          (Updated 2011-05-09 21:22:38)

          Review request for qpid, Alan Conway, Gordon Sim, and Kim van der Riet.

          Summary

          -------

          Broker now issues an explicit flush for each received message that has the sync flag set in the message.transfer command.

          Broker also tracks all messages received that do not have the sync flag set, and will flush these messages on receipt of an execution.sync.

          This addresses bug QPID-3252.

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

          Diffs

          -----

          /trunk/qpid/cpp/src/qpid/broker/SessionState.h 1101206

          /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1101206

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

          Testing

          -------

          qpid unit & store unit

          Thanks,

          Kenneth

          Show
          jiraposter@reviews.apache.org added a comment - On 2011-05-10 08:35:04, Gordon Sim wrote: > /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp, line 538 > < https://reviews.apache.org/r/701/diff/1/?file=18450#file18450line538 > > > Maybe use swap() here? Good point - done! Kenneth ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/701/#review653 ----------------------------------------------------------- On 2011-05-09 21:22:38, Kenneth Giusti wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/701/ ----------------------------------------------------------- (Updated 2011-05-09 21:22:38) Review request for qpid, Alan Conway, Gordon Sim, and Kim van der Riet. Summary ------- Broker now issues an explicit flush for each received message that has the sync flag set in the message.transfer command. Broker also tracks all messages received that do not have the sync flag set, and will flush these messages on receipt of an execution.sync. This addresses bug QPID-3252 . https://issues.apache.org/jira/browse/QPID-3252 Diffs ----- /trunk/qpid/cpp/src/qpid/broker/SessionState.h 1101206 /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1101206 Diff: https://reviews.apache.org/r/701/diff Testing ------- qpid unit & store unit Thanks, Kenneth
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp
          <https://reviews.apache.org/r/701/#comment1310>

          Maybe use swap() here?

          /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp
          <https://reviews.apache.org/r/701/#comment1311>

          Looks good to me

          • Gordon

          On 2011-05-09 21:22:38, Kenneth Giusti wrote:

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

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

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

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

          (Updated 2011-05-09 21:22:38)

          Review request for qpid, Alan Conway, Gordon Sim, and Kim van der Riet.

          Summary

          -------

          Broker now issues an explicit flush for each received message that has the sync flag set in the message.transfer command.

          Broker also tracks all messages received that do not have the sync flag set, and will flush these messages on receipt of an execution.sync.

          This addresses bug QPID-3252.

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

          Diffs

          -----

          /trunk/qpid/cpp/src/qpid/broker/SessionState.h 1101206

          /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1101206

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

          Testing

          -------

          qpid unit & store unit

          Thanks,

          Kenneth

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/701/#review653 ----------------------------------------------------------- /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp < https://reviews.apache.org/r/701/#comment1310 > Maybe use swap() here? /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp < https://reviews.apache.org/r/701/#comment1311 > Looks good to me Gordon On 2011-05-09 21:22:38, Kenneth Giusti wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/701/ ----------------------------------------------------------- (Updated 2011-05-09 21:22:38) Review request for qpid, Alan Conway, Gordon Sim, and Kim van der Riet. Summary ------- Broker now issues an explicit flush for each received message that has the sync flag set in the message.transfer command. Broker also tracks all messages received that do not have the sync flag set, and will flush these messages on receipt of an execution.sync. This addresses bug QPID-3252 . https://issues.apache.org/jira/browse/QPID-3252 Diffs ----- /trunk/qpid/cpp/src/qpid/broker/SessionState.h 1101206 /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1101206 Diff: https://reviews.apache.org/r/701/diff Testing ------- qpid unit & store unit Thanks, Kenneth
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Review request for qpid, Alan Conway, Gordon Sim, and Kim van der Riet.

          Summary
          -------

          Broker now issues an explicit flush for each received message that has the sync flag set in the message.transfer command.
          Broker also tracks all messages received that do not have the sync flag set, and will flush these messages on receipt of an execution.sync.

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

          Diffs


          /trunk/qpid/cpp/src/qpid/broker/SessionState.h 1101206
          /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1101206

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

          Testing
          -------

          qpid unit & store unit

          Thanks,

          Kenneth

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/701/ ----------------------------------------------------------- Review request for qpid, Alan Conway, Gordon Sim, and Kim van der Riet. Summary ------- Broker now issues an explicit flush for each received message that has the sync flag set in the message.transfer command. Broker also tracks all messages received that do not have the sync flag set, and will flush these messages on receipt of an execution.sync. This addresses bug QPID-3252 . https://issues.apache.org/jira/browse/QPID-3252 Diffs /trunk/qpid/cpp/src/qpid/broker/SessionState.h 1101206 /trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1101206 Diff: https://reviews.apache.org/r/701/diff Testing ------- qpid unit & store unit Thanks, Kenneth

            People

            • Assignee:
              Ken Giusti
              Reporter:
              Ken Giusti
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development