You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Pavel Moravec <pm...@redhat.com> on 2014/01/31 17:16:35 UTC
Review Request 17592: QPID-5531 [C++ broker] Set timeout for every DTX
transaction
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17592/
-----------------------------------------------------------
Review request for qpid, Chug Rolke, Cliff Jansen, Kim van der Riet, and Steve Huston.
Bugs: https://issues.apache.org/jira/browse/QPID-5531
https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/QPID-5531
Repository: qpid
Description
-------
If a rogue external Transaction Manager forgets to commit/rollback a prepared DTX, Tpl store keeps an orphaned enqueue record. To prevent it, every DTX transaction should have a default timeout after that the broker automatically aborts the transaction.
QPID-5531 adds broker option dtx-default-timeout for that.
My concerns for review:
- is 3600 seconds as default value proper? Isn't it too high?
- ms-sql and/or ms-clfs store part of the patch (recoverTransaction method) has not been even compiled
Diffs
-----
/trunk/qpid/cpp/src/qpid/broker/Broker.h 1562753
/trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1562753
/trunk/qpid/cpp/src/qpid/broker/DtxManager.h 1562753
/trunk/qpid/cpp/src/qpid/broker/DtxManager.cpp 1562753
/trunk/qpid/cpp/src/qpid/broker/RecoveryManager.h 1562753
/trunk/qpid/cpp/src/qpid/broker/RecoveryManagerImpl.h 1562753
/trunk/qpid/cpp/src/qpid/broker/RecoveryManagerImpl.cpp 1562753
/trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1562753
/trunk/qpid/cpp/src/qpid/legacystore/MessageStoreImpl.cpp 1562753
/trunk/qpid/cpp/src/qpid/linearstore/MessageStoreImpl.cpp 1562753
/trunk/qpid/cpp/src/qpid/store/ms-clfs/MSSqlClfsProvider.cpp 1562753
/trunk/qpid/cpp/src/qpid/store/ms-sql/MSSqlProvider.cpp 1562753
Diff: https://reviews.apache.org/r/17592/diff/
Testing
-------
Thanks,
Pavel Moravec
Re: Review Request 17592: QPID-5531 [C++ broker] Set timeout for every DTX
transaction
Posted by Pavel Moravec <pm...@redhat.com>.
> On Jan. 31, 2014, 4:36 p.m., Gordon Sim wrote:
> > /trunk/qpid/cpp/src/qpid/broker/DtxManager.h, line 54
> > <https://reviews.apache.org/r/17592/diff/1/?file=457723#file457723line54>
> >
> > Rather than passing the default timeout through on each call, why not pass it in to the constructor of DtxManager, and then simply use a member variable from createWork()?
Thanks for idea. Patch to follow..
- Pavel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17592/#review33331
-----------------------------------------------------------
On Feb. 1, 2014, 11:30 a.m., Pavel Moravec wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17592/
> -----------------------------------------------------------
>
> (Updated Feb. 1, 2014, 11:30 a.m.)
>
>
> Review request for qpid, Chug Rolke, Cliff Jansen, Kim van der Riet, and Steve Huston.
>
>
> Bugs: https://issues.apache.org/jira/browse/QPID-5531
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/QPID-5531
>
>
> Repository: qpid
>
>
> Description
> -------
>
> If a rogue external Transaction Manager forgets to commit/rollback a prepared DTX, Tpl store keeps an orphaned enqueue record. To prevent it, every DTX transaction should have a default timeout after that the broker automatically aborts the transaction.
>
> QPID-5531 adds broker option dtx-default-timeout for that.
>
> My concerns for review:
> - is 3600 seconds as default value proper? Isn't it too high?
> - ms-sql and/or ms-clfs store part of the patch (recoverTransaction method) has not been even compiled
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/Broker.h 1563386
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1563386
> /trunk/qpid/cpp/src/qpid/broker/DtxManager.h 1563386
> /trunk/qpid/cpp/src/qpid/broker/DtxManager.cpp 1563386
> /trunk/qpid/cpp/src/tests/legacystore/OrderingTest.cpp 1563386
> /trunk/qpid/cpp/src/tests/legacystore/SimpleTest.cpp 1563386
> /trunk/qpid/cpp/src/tests/legacystore/TransactionalTest.cpp 1563386
> /trunk/qpid/cpp/src/tests/legacystore/TwoPhaseCommitTest.cpp 1563386
>
> Diff: https://reviews.apache.org/r/17592/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Pavel Moravec
>
>
Re: Review Request 17592: QPID-5531 [C++ broker] Set timeout for every DTX
transaction
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17592/#review33331
-----------------------------------------------------------
/trunk/qpid/cpp/src/qpid/broker/DtxManager.h
<https://reviews.apache.org/r/17592/#comment62771>
Rather than passing the default timeout through on each call, why not pass it in to the constructor of DtxManager, and then simply use a member variable from createWork()?
- Gordon Sim
On Jan. 31, 2014, 4:16 p.m., Pavel Moravec wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17592/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2014, 4:16 p.m.)
>
>
> Review request for qpid, Chug Rolke, Cliff Jansen, Kim van der Riet, and Steve Huston.
>
>
> Bugs: https://issues.apache.org/jira/browse/QPID-5531
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/QPID-5531
>
>
> Repository: qpid
>
>
> Description
> -------
>
> If a rogue external Transaction Manager forgets to commit/rollback a prepared DTX, Tpl store keeps an orphaned enqueue record. To prevent it, every DTX transaction should have a default timeout after that the broker automatically aborts the transaction.
>
> QPID-5531 adds broker option dtx-default-timeout for that.
>
> My concerns for review:
> - is 3600 seconds as default value proper? Isn't it too high?
> - ms-sql and/or ms-clfs store part of the patch (recoverTransaction method) has not been even compiled
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/Broker.h 1562753
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1562753
> /trunk/qpid/cpp/src/qpid/broker/DtxManager.h 1562753
> /trunk/qpid/cpp/src/qpid/broker/DtxManager.cpp 1562753
> /trunk/qpid/cpp/src/qpid/broker/RecoveryManager.h 1562753
> /trunk/qpid/cpp/src/qpid/broker/RecoveryManagerImpl.h 1562753
> /trunk/qpid/cpp/src/qpid/broker/RecoveryManagerImpl.cpp 1562753
> /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1562753
> /trunk/qpid/cpp/src/qpid/legacystore/MessageStoreImpl.cpp 1562753
> /trunk/qpid/cpp/src/qpid/linearstore/MessageStoreImpl.cpp 1562753
> /trunk/qpid/cpp/src/qpid/store/ms-clfs/MSSqlClfsProvider.cpp 1562753
> /trunk/qpid/cpp/src/qpid/store/ms-sql/MSSqlProvider.cpp 1562753
>
> Diff: https://reviews.apache.org/r/17592/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Pavel Moravec
>
>
Re: Review Request 17592: QPID-5531 [C++ broker] Set timeout for every DTX
transaction
Posted by Cliff Jansen <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17592/#review33415
-----------------------------------------------------------
This builds fine on Windows. By inspection the code changes look correct to me. Sadly, I was not able to get ctest to cooperate on my system to run simplest tests, let alone the CLFS-specific tests. However, I believe the patch is useful and does no harm on the Windows side.
- Cliff Jansen
On Feb. 1, 2014, 2:47 p.m., Pavel Moravec wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17592/
> -----------------------------------------------------------
>
> (Updated Feb. 1, 2014, 2:47 p.m.)
>
>
> Review request for qpid, Chug Rolke, Cliff Jansen, Kim van der Riet, and Steve Huston.
>
>
> Bugs: QPID-5531
> https://issues.apache.org/jira/browse/QPID-5531
>
>
> Repository: qpid
>
>
> Description
> -------
>
> If a rogue external Transaction Manager forgets to commit/rollback a prepared DTX, Tpl store keeps an orphaned enqueue record. To prevent it, every DTX transaction should have a default timeout after that the broker automatically aborts the transaction.
>
> QPID-5531 adds broker option dtx-default-timeout for that.
>
> My concerns for review:
> - is 3600 seconds as default value proper? Isn't it too high?
> - ms-sql and/or ms-clfs store part of the patch (recoverTransaction method) has not been even compiled
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/Broker.h 1563386
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1563386
> /trunk/qpid/cpp/src/qpid/broker/DtxManager.h 1563386
> /trunk/qpid/cpp/src/qpid/broker/DtxManager.cpp 1563386
> /trunk/qpid/cpp/src/tests/legacystore/OrderingTest.cpp 1563386
> /trunk/qpid/cpp/src/tests/legacystore/SimpleTest.cpp 1563386
> /trunk/qpid/cpp/src/tests/legacystore/TransactionalTest.cpp 1563386
> /trunk/qpid/cpp/src/tests/legacystore/TwoPhaseCommitTest.cpp 1563386
>
> Diff: https://reviews.apache.org/r/17592/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Pavel Moravec
>
>
Re: Review Request 17592: QPID-5531 [C++ broker] Set timeout for every DTX
transaction
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17592/#review33686
-----------------------------------------------------------
Ship it!
I think 60 seconds may be a bot short for the default timeout, but its configurable and in the absence of concrete data to base the choice on its fine as is.
Nice work, Pavel!
- Gordon Sim
On Feb. 5, 2014, 8:36 a.m., Pavel Moravec wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17592/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2014, 8:36 a.m.)
>
>
> Review request for qpid, Chug Rolke, Cliff Jansen, Kim van der Riet, and Steve Huston.
>
>
> Bugs: QPID-5531
> https://issues.apache.org/jira/browse/QPID-5531
>
>
> Repository: qpid
>
>
> Description
> -------
>
> If a rogue external Transaction Manager forgets to commit/rollback a prepared DTX, Tpl store keeps an orphaned enqueue record. To prevent it, every DTX transaction should have a default timeout after that the broker automatically aborts the transaction.
>
> QPID-5531 adds broker option dtx-default-timeout for that.
>
> My concerns for review:
> - is 3600 seconds as default value proper? Isn't it too high?
> - ms-sql and/or ms-clfs store part of the patch (recoverTransaction method) has not been even compiled
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/Broker.h 1563872
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1563872
> /trunk/qpid/cpp/src/qpid/broker/DtxManager.h 1563872
> /trunk/qpid/cpp/src/qpid/broker/DtxManager.cpp 1563872
> /trunk/qpid/cpp/src/qpid/broker/SessionAdapter.cpp 1563872
> /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/dtx.py 1563872
>
> Diff: https://reviews.apache.org/r/17592/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Pavel Moravec
>
>
Re: Review Request 17592: QPID-5531 [C++ broker] Set timeout for every DTX
transaction
Posted by Pavel Moravec <pm...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17592/
-----------------------------------------------------------
(Updated Feb. 5, 2014, 8:36 a.m.)
Review request for qpid, Chug Rolke, Cliff Jansen, Kim van der Riet, and Steve Huston.
Changes
-------
Comments implemented.
Re "The default maximum allowed timeout is 3600 seconds." - qpidd --help already prints out default values, e.g.:
--dtx-max-timeout SECONDS (3600) Maximum allowed
timeout for DTX
transaction. A value
of zero disables
maximum timeout
limit checks and
allows arbitrarily
large timeout
settings.
Bugs: QPID-5531
https://issues.apache.org/jira/browse/QPID-5531
Repository: qpid
Description
-------
If a rogue external Transaction Manager forgets to commit/rollback a prepared DTX, Tpl store keeps an orphaned enqueue record. To prevent it, every DTX transaction should have a default timeout after that the broker automatically aborts the transaction.
QPID-5531 adds broker option dtx-default-timeout for that.
My concerns for review:
- is 3600 seconds as default value proper? Isn't it too high?
- ms-sql and/or ms-clfs store part of the patch (recoverTransaction method) has not been even compiled
Diffs (updated)
-----
/trunk/qpid/cpp/src/qpid/broker/Broker.h 1563872
/trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1563872
/trunk/qpid/cpp/src/qpid/broker/DtxManager.h 1563872
/trunk/qpid/cpp/src/qpid/broker/DtxManager.cpp 1563872
/trunk/qpid/cpp/src/qpid/broker/SessionAdapter.cpp 1563872
/trunk/qpid/tests/src/py/qpid_tests/broker_0_10/dtx.py 1563872
Diff: https://reviews.apache.org/r/17592/diff/
Testing
-------
Thanks,
Pavel Moravec
Re: Review Request 17592: QPID-5531 [C++ broker] Set timeout for every DTX
transaction
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17592/#review33613
-----------------------------------------------------------
/trunk/qpid/cpp/src/qpid/broker/DtxManager.h
<https://reviews.apache.org/r/17592/#comment63149>
This doesn't look right. Though I believe C++11 allows you to delegate to another constructor, I don't think this is the right syntax. In any case I think we want something that works for older compilers.
Why not just keep one constructor but add a default value for the second parameter:
DtxManager(sys::Timer&, uint32_t _dtxDefaultTimeout=0);
- Gordon Sim
On Feb. 4, 2014, 2:15 p.m., Pavel Moravec wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17592/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2014, 2:15 p.m.)
>
>
> Review request for qpid, Chug Rolke, Cliff Jansen, Kim van der Riet, and Steve Huston.
>
>
> Bugs: QPID-5531
> https://issues.apache.org/jira/browse/QPID-5531
>
>
> Repository: qpid
>
>
> Description
> -------
>
> If a rogue external Transaction Manager forgets to commit/rollback a prepared DTX, Tpl store keeps an orphaned enqueue record. To prevent it, every DTX transaction should have a default timeout after that the broker automatically aborts the transaction.
>
> QPID-5531 adds broker option dtx-default-timeout for that.
>
> My concerns for review:
> - is 3600 seconds as default value proper? Isn't it too high?
> - ms-sql and/or ms-clfs store part of the patch (recoverTransaction method) has not been even compiled
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/Broker.h 1563872
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1563872
> /trunk/qpid/cpp/src/qpid/broker/DtxManager.h 1563872
> /trunk/qpid/cpp/src/qpid/broker/DtxManager.cpp 1563872
> /trunk/qpid/cpp/src/qpid/broker/SessionAdapter.cpp 1563872
> /trunk/qpid/cpp/src/tests/qpid-txtest.cpp 1563872
> /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/dtx.py 1563872
>
> Diff: https://reviews.apache.org/r/17592/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Pavel Moravec
>
>
Re: Review Request 17592: QPID-5531 [C++ broker] Set timeout for every DTX
transaction
Posted by Chug Rolke <cr...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17592/#review33617
-----------------------------------------------------------
/trunk/qpid/cpp/src/qpid/broker/Broker.cpp
<https://reviews.apache.org/r/17592/#comment63153>
For dtx-max-timeout, add text to the effect: "A value of zero disables maximum timeout limit checks and allows arbitrarily large timeout settings. The default maximum allowed timeout is 3600 seconds."
- Chug Rolke
On Feb. 4, 2014, 2:15 p.m., Pavel Moravec wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17592/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2014, 2:15 p.m.)
>
>
> Review request for qpid, Chug Rolke, Cliff Jansen, Kim van der Riet, and Steve Huston.
>
>
> Bugs: QPID-5531
> https://issues.apache.org/jira/browse/QPID-5531
>
>
> Repository: qpid
>
>
> Description
> -------
>
> If a rogue external Transaction Manager forgets to commit/rollback a prepared DTX, Tpl store keeps an orphaned enqueue record. To prevent it, every DTX transaction should have a default timeout after that the broker automatically aborts the transaction.
>
> QPID-5531 adds broker option dtx-default-timeout for that.
>
> My concerns for review:
> - is 3600 seconds as default value proper? Isn't it too high?
> - ms-sql and/or ms-clfs store part of the patch (recoverTransaction method) has not been even compiled
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/Broker.h 1563872
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1563872
> /trunk/qpid/cpp/src/qpid/broker/DtxManager.h 1563872
> /trunk/qpid/cpp/src/qpid/broker/DtxManager.cpp 1563872
> /trunk/qpid/cpp/src/qpid/broker/SessionAdapter.cpp 1563872
> /trunk/qpid/cpp/src/tests/qpid-txtest.cpp 1563872
> /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/dtx.py 1563872
>
> Diff: https://reviews.apache.org/r/17592/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Pavel Moravec
>
>
Re: Review Request 17592: QPID-5531 [C++ broker] Set timeout for every DTX
transaction
Posted by Pavel Moravec <pm...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17592/
-----------------------------------------------------------
(Updated Feb. 4, 2014, 2:15 p.m.)
Review request for qpid, Chug Rolke, Cliff Jansen, Kim van der Riet, and Steve Huston.
Changes
-------
New version of patch, newly fixing automated test failure and adding another test there.
(Note, two auto-tests fail but independently on this patch: legacystore_OrderingTest and legacystore_TwoPhaseCommit, due to valgrind issues)
Bugs: QPID-5531
https://issues.apache.org/jira/browse/QPID-5531
Repository: qpid
Description
-------
If a rogue external Transaction Manager forgets to commit/rollback a prepared DTX, Tpl store keeps an orphaned enqueue record. To prevent it, every DTX transaction should have a default timeout after that the broker automatically aborts the transaction.
QPID-5531 adds broker option dtx-default-timeout for that.
My concerns for review:
- is 3600 seconds as default value proper? Isn't it too high?
- ms-sql and/or ms-clfs store part of the patch (recoverTransaction method) has not been even compiled
Diffs (updated)
-----
/trunk/qpid/cpp/src/qpid/broker/Broker.h 1563872
/trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1563872
/trunk/qpid/cpp/src/qpid/broker/DtxManager.h 1563872
/trunk/qpid/cpp/src/qpid/broker/DtxManager.cpp 1563872
/trunk/qpid/cpp/src/qpid/broker/SessionAdapter.cpp 1563872
/trunk/qpid/cpp/src/tests/qpid-txtest.cpp 1563872
/trunk/qpid/tests/src/py/qpid_tests/broker_0_10/dtx.py 1563872
Diff: https://reviews.apache.org/r/17592/diff/
Testing
-------
Thanks,
Pavel Moravec
Re: Review Request 17592: QPID-5531 [C++ broker] Set timeout for every DTX
transaction
Posted by Pavel Moravec <pm...@redhat.com>.
> On Feb. 4, 2014, 12:14 a.m., Steve Huston wrote:
> > /trunk/qpid/cpp/src/qpid/broker/DtxManager.h, line 53
> > <https://reviews.apache.org/r/17592/diff/3/?file=463341#file463341line53>
> >
> > Would it maybe be better to apply the dtxDefaultTimeout here in order to close the DOS attack vector by default?
DtxManager contructor is called from several places where:
- DtxManager(timer) is called only from automated tests (like ./src/tests/legacystore/TwoPhaseCommitTest.cpp or ./src/tests/legacystore/TransactionalTest.cpp)
- in any other case, DtxManager(timer, timeout) is called
So whenever DtxManager is called due to a real client, it is done with timeout parameter as well.
(anyway, some automated tests failed so I will provide next patch version soon)
- Pavel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17592/#review33536
-----------------------------------------------------------
On Feb. 3, 2014, 4:39 p.m., Pavel Moravec wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17592/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2014, 4:39 p.m.)
>
>
> Review request for qpid, Chug Rolke, Cliff Jansen, Kim van der Riet, and Steve Huston.
>
>
> Bugs: QPID-5531
> https://issues.apache.org/jira/browse/QPID-5531
>
>
> Repository: qpid
>
>
> Description
> -------
>
> If a rogue external Transaction Manager forgets to commit/rollback a prepared DTX, Tpl store keeps an orphaned enqueue record. To prevent it, every DTX transaction should have a default timeout after that the broker automatically aborts the transaction.
>
> QPID-5531 adds broker option dtx-default-timeout for that.
>
> My concerns for review:
> - is 3600 seconds as default value proper? Isn't it too high?
> - ms-sql and/or ms-clfs store part of the patch (recoverTransaction method) has not been even compiled
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/Broker.h 1563872
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1563872
> /trunk/qpid/cpp/src/qpid/broker/DtxManager.h 1563872
> /trunk/qpid/cpp/src/qpid/broker/DtxManager.cpp 1563872
> /trunk/qpid/cpp/src/qpid/broker/SessionAdapter.cpp 1563872
>
> Diff: https://reviews.apache.org/r/17592/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Pavel Moravec
>
>
Re: Review Request 17592: QPID-5531 [C++ broker] Set timeout for every DTX
transaction
Posted by Steve Huston <sh...@riverace.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17592/#review33536
-----------------------------------------------------------
/trunk/qpid/cpp/src/qpid/broker/DtxManager.h
<https://reviews.apache.org/r/17592/#comment63059>
Would it maybe be better to apply the dtxDefaultTimeout here in order to close the DOS attack vector by default?
- Steve Huston
On Feb. 3, 2014, 4:39 p.m., Pavel Moravec wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17592/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2014, 4:39 p.m.)
>
>
> Review request for qpid, Chug Rolke, Cliff Jansen, Kim van der Riet, and Steve Huston.
>
>
> Bugs: QPID-5531
> https://issues.apache.org/jira/browse/QPID-5531
>
>
> Repository: qpid
>
>
> Description
> -------
>
> If a rogue external Transaction Manager forgets to commit/rollback a prepared DTX, Tpl store keeps an orphaned enqueue record. To prevent it, every DTX transaction should have a default timeout after that the broker automatically aborts the transaction.
>
> QPID-5531 adds broker option dtx-default-timeout for that.
>
> My concerns for review:
> - is 3600 seconds as default value proper? Isn't it too high?
> - ms-sql and/or ms-clfs store part of the patch (recoverTransaction method) has not been even compiled
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/Broker.h 1563872
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1563872
> /trunk/qpid/cpp/src/qpid/broker/DtxManager.h 1563872
> /trunk/qpid/cpp/src/qpid/broker/DtxManager.cpp 1563872
> /trunk/qpid/cpp/src/qpid/broker/SessionAdapter.cpp 1563872
>
> Diff: https://reviews.apache.org/r/17592/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Pavel Moravec
>
>
Re: Review Request 17592: QPID-5531 [C++ broker] Set timeout for every DTX
transaction
Posted by Pavel Moravec <pm...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17592/
-----------------------------------------------------------
(Updated Feb. 3, 2014, 4:39 p.m.)
Review request for qpid, Chug Rolke, Cliff Jansen, Kim van der Riet, and Steve Huston.
Changes
-------
New patch version provided, incorporating Gordon's comment & --dtx-max-timeout option per https://issues.apache.org/jira/browse/QPID-5531?focusedCommentId=13889327&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13889327
Bugs: QPID-5531
https://issues.apache.org/jira/browse/QPID-5531
Repository: qpid
Description
-------
If a rogue external Transaction Manager forgets to commit/rollback a prepared DTX, Tpl store keeps an orphaned enqueue record. To prevent it, every DTX transaction should have a default timeout after that the broker automatically aborts the transaction.
QPID-5531 adds broker option dtx-default-timeout for that.
My concerns for review:
- is 3600 seconds as default value proper? Isn't it too high?
- ms-sql and/or ms-clfs store part of the patch (recoverTransaction method) has not been even compiled
Diffs (updated)
-----
/trunk/qpid/cpp/src/qpid/broker/Broker.h 1563872
/trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1563872
/trunk/qpid/cpp/src/qpid/broker/DtxManager.h 1563872
/trunk/qpid/cpp/src/qpid/broker/DtxManager.cpp 1563872
/trunk/qpid/cpp/src/qpid/broker/SessionAdapter.cpp 1563872
Diff: https://reviews.apache.org/r/17592/diff/
Testing
-------
Thanks,
Pavel Moravec
Re: Review Request 17592: QPID-5531 [C++ broker] Set timeout for every DTX
transaction
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17592/#review33416
-----------------------------------------------------------
Ship it!
/trunk/qpid/cpp/src/qpid/broker/DtxManager.h
<https://reviews.apache.org/r/17592/#comment62863>
If you gave this a default value of 0 and added a one line check for that (meaning no timeout) that would (a) mean you didn't need to change the tests and (b) would allow users to turn of the default timeout completely if desired (not that I can think of any good reason for doing so).
However this is perfectly acceptable as is also.
- Gordon Sim
On Feb. 1, 2014, 2:47 p.m., Pavel Moravec wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17592/
> -----------------------------------------------------------
>
> (Updated Feb. 1, 2014, 2:47 p.m.)
>
>
> Review request for qpid, Chug Rolke, Cliff Jansen, Kim van der Riet, and Steve Huston.
>
>
> Bugs: QPID-5531
> https://issues.apache.org/jira/browse/QPID-5531
>
>
> Repository: qpid
>
>
> Description
> -------
>
> If a rogue external Transaction Manager forgets to commit/rollback a prepared DTX, Tpl store keeps an orphaned enqueue record. To prevent it, every DTX transaction should have a default timeout after that the broker automatically aborts the transaction.
>
> QPID-5531 adds broker option dtx-default-timeout for that.
>
> My concerns for review:
> - is 3600 seconds as default value proper? Isn't it too high?
> - ms-sql and/or ms-clfs store part of the patch (recoverTransaction method) has not been even compiled
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/Broker.h 1563386
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1563386
> /trunk/qpid/cpp/src/qpid/broker/DtxManager.h 1563386
> /trunk/qpid/cpp/src/qpid/broker/DtxManager.cpp 1563386
> /trunk/qpid/cpp/src/tests/legacystore/OrderingTest.cpp 1563386
> /trunk/qpid/cpp/src/tests/legacystore/SimpleTest.cpp 1563386
> /trunk/qpid/cpp/src/tests/legacystore/TransactionalTest.cpp 1563386
> /trunk/qpid/cpp/src/tests/legacystore/TwoPhaseCommitTest.cpp 1563386
>
> Diff: https://reviews.apache.org/r/17592/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Pavel Moravec
>
>
Re: Review Request 17592: QPID-5531 [C++ broker] Set timeout for every DTX
transaction
Posted by Pavel Moravec <pm...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17592/
-----------------------------------------------------------
(Updated Feb. 1, 2014, 2:47 p.m.)
Review request for qpid, Chug Rolke, Cliff Jansen, Kim van der Riet, and Steve Huston.
Bugs: QPID-5531
https://issues.apache.org/jira/browse/QPID-5531
Repository: qpid
Description
-------
If a rogue external Transaction Manager forgets to commit/rollback a prepared DTX, Tpl store keeps an orphaned enqueue record. To prevent it, every DTX transaction should have a default timeout after that the broker automatically aborts the transaction.
QPID-5531 adds broker option dtx-default-timeout for that.
My concerns for review:
- is 3600 seconds as default value proper? Isn't it too high?
- ms-sql and/or ms-clfs store part of the patch (recoverTransaction method) has not been even compiled
Diffs
-----
/trunk/qpid/cpp/src/qpid/broker/Broker.h 1563386
/trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1563386
/trunk/qpid/cpp/src/qpid/broker/DtxManager.h 1563386
/trunk/qpid/cpp/src/qpid/broker/DtxManager.cpp 1563386
/trunk/qpid/cpp/src/tests/legacystore/OrderingTest.cpp 1563386
/trunk/qpid/cpp/src/tests/legacystore/SimpleTest.cpp 1563386
/trunk/qpid/cpp/src/tests/legacystore/TransactionalTest.cpp 1563386
/trunk/qpid/cpp/src/tests/legacystore/TwoPhaseCommitTest.cpp 1563386
Diff: https://reviews.apache.org/r/17592/diff/
Testing
-------
Thanks,
Pavel Moravec
Re: Review Request 17592: QPID-5531 [C++ broker] Set timeout for every DTX
transaction
Posted by Pavel Moravec <pm...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17592/
-----------------------------------------------------------
(Updated Feb. 1, 2014, 11:30 a.m.)
Review request for qpid, Chug Rolke, Cliff Jansen, Kim van der Riet, and Steve Huston.
Changes
-------
New patch version, per Gordon's comment. Windows bits not touched at all.
Bugs: https://issues.apache.org/jira/browse/QPID-5531
https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/QPID-5531
Repository: qpid
Description
-------
If a rogue external Transaction Manager forgets to commit/rollback a prepared DTX, Tpl store keeps an orphaned enqueue record. To prevent it, every DTX transaction should have a default timeout after that the broker automatically aborts the transaction.
QPID-5531 adds broker option dtx-default-timeout for that.
My concerns for review:
- is 3600 seconds as default value proper? Isn't it too high?
- ms-sql and/or ms-clfs store part of the patch (recoverTransaction method) has not been even compiled
Diffs (updated)
-----
/trunk/qpid/cpp/src/qpid/broker/Broker.h 1563386
/trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1563386
/trunk/qpid/cpp/src/qpid/broker/DtxManager.h 1563386
/trunk/qpid/cpp/src/qpid/broker/DtxManager.cpp 1563386
/trunk/qpid/cpp/src/tests/legacystore/OrderingTest.cpp 1563386
/trunk/qpid/cpp/src/tests/legacystore/SimpleTest.cpp 1563386
/trunk/qpid/cpp/src/tests/legacystore/TransactionalTest.cpp 1563386
/trunk/qpid/cpp/src/tests/legacystore/TwoPhaseCommitTest.cpp 1563386
Diff: https://reviews.apache.org/r/17592/diff/
Testing
-------
Thanks,
Pavel Moravec