You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Alan Conway <ac...@redhat.com> on 2014/08/20 23:35:46 UTC
Re: Review Request 23096: QPID-5855: JAVA Client Can not recieve
message
with qpid ha cluster "Session exception occured while trying to commit"
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23096/
-----------------------------------------------------------
(Updated Aug. 20, 2014, 9:35 p.m.)
Review request for qpid, Gordon Sim, rajith attapattu, and Robbie Gemmell.
Changes
-------
Analysys of problem and broker side fix.
Summary (updated)
-----------------
QPID-5855: JAVA Client Can not recieve message with qpid ha cluster "Session exception occured while trying to commit"
Bugs: QPID-5855
https://issues.apache.org/jira/browse/QPID-5855
Repository: qpid
Description (updated)
-------
The problem: the java client sets the sync flag on tx.commit and then waits for
completion of the entire transaction. With HA enabled, both transfer and commit
are asynchronously completed and it is possible for the commit to
complete *before* the preceeding transfer. In that case the C++ broker sends
completions immediately for the commit (because it has the sync flag), but there
is a hole for the incomplete transfer. The transfer does *not* have the sync
flag so when it completes the broker does not immediately send a completion and
we hang.
Its not completely clear which of the broker or the client is strictly correct
in this case, but it seems prudent to fix it on the broker. The fix: when a
command with the sync flag completes, if there are any "holes" (earlier commands
not yet complete) then the broker records the current command ID as if it were
an execution.sync. Thus when the holes are filled the broker will send the
completions immediately.
Diffs (updated)
-----
trunk/qpid/cpp/src/qpid/broker/SessionState.h 1619236
trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1619236
Diff: https://reviews.apache.org/r/23096/diff/
Testing (updated)
-------
Passes reproducer and ctest
Thanks,
Alan Conway
Re: Review Request 23096: QPID-5855: JAVA Client Can not recieve
message
with qpid ha cluster "Session exception occured while trying to commit"
Posted by Alan Conway <ac...@redhat.com>.
> On Aug. 21, 2014, 11:13 a.m., Robbie Gemmell wrote:
> > Looking at the client code (I'll leave the code for people familiar with it), I can see where it (o.a.q.transport.Session#complete) would be aware of the gap and make the sync() method hold awaiting it being filled, and I think what you describe sounds like it should work in that as the completion for the transfer is issued before the timeout period elapses then the client should be happy and return from the commit() call.
> >
> > That said, I think the 0-10 spec does says that what the client is doing is ok and what the broker is [going to be] doing [still] isn't: for tx.commit it has the statement that "the commit will not be completed until all preceding commands which it affects have been completed." The tx.rollback description adds a few more words in its related statement, saying "notification of completion for all such commands will be issued before the issuance of the completion for the rollback".
>
> Robbie Gemmell wrote:
> * I'll leave the *broker* code for people familiar with it, obviously ;)
Agreed, I can modify this fix slightly to get the broker to do the right thing - complete the commit only when the entire TX is complete. Finally it makes sense!
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23096/#review51165
-----------------------------------------------------------
On Aug. 20, 2014, 9:35 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23096/
> -----------------------------------------------------------
>
> (Updated Aug. 20, 2014, 9:35 p.m.)
>
>
> Review request for qpid, Gordon Sim, rajith attapattu, and Robbie Gemmell.
>
>
> Bugs: QPID-5855
> https://issues.apache.org/jira/browse/QPID-5855
>
>
> Repository: qpid
>
>
> Description
> -------
>
> The problem: the java client sets the sync flag on tx.commit and then waits for
> completion of the entire transaction. With HA enabled, both transfer and commit
> are asynchronously completed and it is possible for the commit to
> complete *before* the preceeding transfer. In that case the C++ broker sends
> completions immediately for the commit (because it has the sync flag), but there
> is a hole for the incomplete transfer. The transfer does *not* have the sync
> flag so when it completes the broker does not immediately send a completion and
> we hang.
>
> Its not completely clear which of the broker or the client is strictly correct
> in this case, but it seems prudent to fix it on the broker. The fix: when a
> command with the sync flag completes, if there are any "holes" (earlier commands
> not yet complete) then the broker records the current command ID as if it were
> an execution.sync. Thus when the holes are filled the broker will send the
> completions immediately.
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/src/qpid/broker/SessionState.h 1619236
> trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1619236
>
> Diff: https://reviews.apache.org/r/23096/diff/
>
>
> Testing
> -------
>
> Passes reproducer and ctest
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request 23096: QPID-5855: JAVA Client Can not recieve
message
with qpid ha cluster "Session exception occured while trying to commit"
Posted by Robbie Gemmell <ro...@gmail.com>.
> On Aug. 21, 2014, 11:13 a.m., Robbie Gemmell wrote:
> > Looking at the client code (I'll leave the code for people familiar with it), I can see where it (o.a.q.transport.Session#complete) would be aware of the gap and make the sync() method hold awaiting it being filled, and I think what you describe sounds like it should work in that as the completion for the transfer is issued before the timeout period elapses then the client should be happy and return from the commit() call.
> >
> > That said, I think the 0-10 spec does says that what the client is doing is ok and what the broker is [going to be] doing [still] isn't: for tx.commit it has the statement that "the commit will not be completed until all preceding commands which it affects have been completed." The tx.rollback description adds a few more words in its related statement, saying "notification of completion for all such commands will be issued before the issuance of the completion for the rollback".
* I'll leave the *broker* code for people familiar with it, obviously ;)
- Robbie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23096/#review51165
-----------------------------------------------------------
On Aug. 20, 2014, 9:35 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23096/
> -----------------------------------------------------------
>
> (Updated Aug. 20, 2014, 9:35 p.m.)
>
>
> Review request for qpid, Gordon Sim, rajith attapattu, and Robbie Gemmell.
>
>
> Bugs: QPID-5855
> https://issues.apache.org/jira/browse/QPID-5855
>
>
> Repository: qpid
>
>
> Description
> -------
>
> The problem: the java client sets the sync flag on tx.commit and then waits for
> completion of the entire transaction. With HA enabled, both transfer and commit
> are asynchronously completed and it is possible for the commit to
> complete *before* the preceeding transfer. In that case the C++ broker sends
> completions immediately for the commit (because it has the sync flag), but there
> is a hole for the incomplete transfer. The transfer does *not* have the sync
> flag so when it completes the broker does not immediately send a completion and
> we hang.
>
> Its not completely clear which of the broker or the client is strictly correct
> in this case, but it seems prudent to fix it on the broker. The fix: when a
> command with the sync flag completes, if there are any "holes" (earlier commands
> not yet complete) then the broker records the current command ID as if it were
> an execution.sync. Thus when the holes are filled the broker will send the
> completions immediately.
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/src/qpid/broker/SessionState.h 1619236
> trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1619236
>
> Diff: https://reviews.apache.org/r/23096/diff/
>
>
> Testing
> -------
>
> Passes reproducer and ctest
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request 23096: QPID-5855: JAVA Client Can not recieve
message
with qpid ha cluster "Session exception occured while trying to commit"
Posted by Robbie Gemmell <ro...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23096/#review51165
-----------------------------------------------------------
Looking at the client code (I'll leave the code for people familiar with it), I can see where it (o.a.q.transport.Session#complete) would be aware of the gap and make the sync() method hold awaiting it being filled, and I think what you describe sounds like it should work in that as the completion for the transfer is issued before the timeout period elapses then the client should be happy and return from the commit() call.
That said, I think the 0-10 spec does says that what the client is doing is ok and what the broker is [going to be] doing [still] isn't: for tx.commit it has the statement that "the commit will not be completed until all preceding commands which it affects have been completed." The tx.rollback description adds a few more words in its related statement, saying "notification of completion for all such commands will be issued before the issuance of the completion for the rollback".
- Robbie Gemmell
On Aug. 20, 2014, 9:35 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23096/
> -----------------------------------------------------------
>
> (Updated Aug. 20, 2014, 9:35 p.m.)
>
>
> Review request for qpid, Gordon Sim, rajith attapattu, and Robbie Gemmell.
>
>
> Bugs: QPID-5855
> https://issues.apache.org/jira/browse/QPID-5855
>
>
> Repository: qpid
>
>
> Description
> -------
>
> The problem: the java client sets the sync flag on tx.commit and then waits for
> completion of the entire transaction. With HA enabled, both transfer and commit
> are asynchronously completed and it is possible for the commit to
> complete *before* the preceeding transfer. In that case the C++ broker sends
> completions immediately for the commit (because it has the sync flag), but there
> is a hole for the incomplete transfer. The transfer does *not* have the sync
> flag so when it completes the broker does not immediately send a completion and
> we hang.
>
> Its not completely clear which of the broker or the client is strictly correct
> in this case, but it seems prudent to fix it on the broker. The fix: when a
> command with the sync flag completes, if there are any "holes" (earlier commands
> not yet complete) then the broker records the current command ID as if it were
> an execution.sync. Thus when the holes are filled the broker will send the
> completions immediately.
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/src/qpid/broker/SessionState.h 1619236
> trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1619236
>
> Diff: https://reviews.apache.org/r/23096/diff/
>
>
> Testing
> -------
>
> Passes reproducer and ctest
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request 23096: QPID-5855: JAVA Client Can not recieve
message
with qpid ha cluster "Session exception occured while trying to commit"
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23096/#review51270
-----------------------------------------------------------
Ship it!
Looks good to me; nice work!
- Gordon Sim
On Aug. 21, 2014, 10:56 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23096/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2014, 10:56 p.m.)
>
>
> Review request for qpid, Gordon Sim, rajith attapattu, and Robbie Gemmell.
>
>
> Bugs: QPID-5855
> https://issues.apache.org/jira/browse/QPID-5855
>
>
> Repository: qpid
>
>
> Description
> -------
>
> The problem: the java client sets the sync flag on tx.commit and then waits for
> completion of the entire transaction. According to the 0-10 spec, this is
> correct, the commit (or rollback) will not complete until all of the
> transactional commands have completed. However the C++ broker was sometimes
> completing a commit *before* one of the the corresponding enqueues. It issued
> the completions up to the commit (because the commit is makred sync) but there
> is a "hole" for the incomplete enqueue. The enqueue is not marked sync so when
> this hole is filled no completion is sent and the client hangs.
>
> Fix: make tx.commit a "sync point", that is it behaves like execution.sync and
> is not completed till all preceeding commands are complete. Note tx.rollback
> does not need modification as it is never completed asynchronously.
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/src/qpid/broker/AsyncCommandCallback.h 1619584
> trunk/qpid/cpp/src/qpid/broker/AsyncCommandCallback.cpp 1619584
> trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1619584
> trunk/qpid/cpp/src/qpid/broker/SessionContext.h 1619584
> trunk/qpid/cpp/src/qpid/broker/SessionState.h 1619584
> trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1619584
> trunk/qpid/cpp/src/tests/AsyncCompletion.cpp 1619584
>
> Diff: https://reviews.apache.org/r/23096/diff/
>
>
> Testing
> -------
>
> Passes reproducer and ctest
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request 23096: QPID-5855: JAVA Client Can not recieve
message
with qpid ha cluster "Session exception occured while trying to commit"
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23096/
-----------------------------------------------------------
(Updated Aug. 21, 2014, 10:56 p.m.)
Review request for qpid, Gordon Sim, rajith attapattu, and Robbie Gemmell.
Changes
-------
Small modification of previous fix to make tx.commit a "sync point" so that it does not complete util the async commit operations are complete *and* all previous commands are complete (like execution.sync) Thanks Robbie!
Bugs: QPID-5855
https://issues.apache.org/jira/browse/QPID-5855
Repository: qpid
Description (updated)
-------
The problem: the java client sets the sync flag on tx.commit and then waits for
completion of the entire transaction. According to the 0-10 spec, this is
correct, the commit (or rollback) will not complete until all of the
transactional commands have completed. However the C++ broker was sometimes
completing a commit *before* one of the the corresponding enqueues. It issued
the completions up to the commit (because the commit is makred sync) but there
is a "hole" for the incomplete enqueue. The enqueue is not marked sync so when
this hole is filled no completion is sent and the client hangs.
Fix: make tx.commit a "sync point", that is it behaves like execution.sync and
is not completed till all preceeding commands are complete. Note tx.rollback
does not need modification as it is never completed asynchronously.
Diffs (updated)
-----
trunk/qpid/cpp/src/qpid/broker/AsyncCommandCallback.h 1619584
trunk/qpid/cpp/src/qpid/broker/AsyncCommandCallback.cpp 1619584
trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1619584
trunk/qpid/cpp/src/qpid/broker/SessionContext.h 1619584
trunk/qpid/cpp/src/qpid/broker/SessionState.h 1619584
trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1619584
trunk/qpid/cpp/src/tests/AsyncCompletion.cpp 1619584
Diff: https://reviews.apache.org/r/23096/diff/
Testing
-------
Passes reproducer and ctest
Thanks,
Alan Conway