[
https://issues.apache.org/jira/browse/QPID-2128?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12790919#action_12790919
]
Cliff Jansen commented on QPID-2128:
------------------------------------
Thank you very much for the detailed review.
> Some comments/questions
> Dtcplugin.cpp
> 188: should host be checked for empty string?
I don't think so. It is only used internally and it would take
contorted programming to pass the reference parameter as a null value.
> 172: "i < len" should be "i < len-1" since the next line is referencing
> dsn[i+1]
That's a bug, will fix.
> 324: the qpid SQL storage provider does not log indoubt transactions,
> so the recover will always get an empty list. If the qpid broker
> crashes after prepare, the transaction is rolled back by SQL. This may
> cause data inconsistency because other RMs may commit that transaction
> already. Need to work with Stephen to figure out if this is an issue
> in the storage provider implementation.
Wow. Triple points for finding that merely from the Windows provider
code. Will fix asap, thanks.
> 454: if the new operator throws due to low memory condition and the
> exception is handled in some other place, there could be deadlock
> because the critical section is not released.
Bug, will fix.
> 454 - 464: if you call open first and add the new resource manager to
> the map when it succeeds, there is no need to erase. Using an auto
> pointer here can make it easy to release the object on failures.
Good point, will rework.
> 461: what exception can be thrown by delete? The destructor of
> ResourceManager is empty.
The destructors of Qpid::Client::Session and Connection are recursively called.
> 490: if close fails, is xa_close going to retried?
I see your point. The XA spec is silent on this and DTC documentation
only refers to XA. Guarding against the possibility will be required.
> Overall the implementation in this file does not follow the qpid style
> very well. I would wrap the global variables and functions (line 91 to
> 190) into a DtcPlugin class to avoid global variables and functions as
> much as possible.
> At least for me, the use of the critical section rmLock is not easy to
> follow. Ideally thread synchronization should be encapsulated into
> classes (i.e. making the class thread-safe). The ResourceManager class
> can hold one critical section to synchronize communication with the
> qpid broker. The DtcPlugin class can use one critical section to
> synchronize access to all resource manager instances. The DtcPlugin
> class can implement the XA interface and the xa_switch entry methods
> (xa_open, xa_close, etc) can just call the DtcPlugin methods. Qpid
> implements a scoped lock/unlock (qpid.sys.ScopedLock) but I am not
> sure if you want to use it in the DTC plugin.
In the trade off of addressing people familiar with COM and MSDTC
versus people familiar with Qpid, I think the former makes the most
sense for code maintainability. The Qpid use is practically trivial
(if obscure), whereas the interaction with DTC is non-obvious and
needs to be bullet proof. I would think there to be mainly scrutiny
of the COM calling and callback interfaces, so sticking with a style
that matches the related documentation and standard examples is
probably preferable.
Using the two tiered lock structure as you describe is a useful
enhancement if the DTC does parallel recovery to RM instances.
> AmqpConnection.cpp
> 110 - 118: this can be refactored into DtxResourceManager. The
> connection should not care what needs to be done for pending
> transactions on connection closing. It should notify the resource
> manager that the connection is closing, like what it does for
> sessions.
Agreed. This is left over cruft from another fix, and effectively a
no-op. Will remove.
> XaTransaction.cpp
> 66: use InterlockedIncrement/InterlockedDecrement so you don't need
> this lock
OK.
> 204: You can set the ref count to 1 in the constructor so you don't
> have to call AddRef() after creating each object.
I think that messes with traditional coding style, at least of the
sample code I've seen. I think it is easier for Qpid contributors to
maintain this if the constructor is "normal" and the AddRef is an
additional (commented) step.
> AmqpSession.cpp
> 109: why not make this a method of XaTransaction?
This is a convenience casting routine, just needed by AmqpSession. I
try to hide native C++ Qpid definitions from the managed .NET
interfaces.
> 322: remove
OK.
> In ConnectionClosed and NotifyClosed, why is it important to flush
> phase 0 (sending dtxEnd)? The broker is going to rollback all pending
> transactions of a session when it is closed. If a dtx involves
> multiple sessions, what is the behavior of closing one session before
> commit?
Unfortunately, there is no way to know when a broker will execute a
dtxEnd other than waiting for the command status completion. The
dtxEnd can precede the prepare on the wire, but be processed
afterwards on the broker. The broker will kill the control session if
it sees out of order on its end. Worse, if the session tears down
from an async close, you have no event to wait for to indicate the
broker has done it's clean-up work.
> DtxResourceManager.cpp
> 220-227: this method is named GetXaTransaction but internally we
> create a new resource manager for that connection. I would refactor
> this method into two methods: GetResourceMananger and then
> GetXaTransaction from that resource manager.
That has the disadvantage that anyone who wants to use the object
directly has to guard against async TmDown with try/catch blocks and
reacquire, or else the singleton-per-broker has to have a split brain
to complete old pending transactions while accepting new ones. The
static method hides all that and is very fast in the "sunny day"
scenarios.
> 233: this method can be combined with the previous one for the
> refactoring mentioned above.
> 214: InternalGetXaTransaction returns nullptr only during TmDown.
> In that case do we need to create the RM?
In the case of a long running application, lazily creating a new RM as
necessary allows for a DTC restart.
> 270: ideally the resource manager should manage the state of all
> transactions in response to events, e.g. connection close. It seems a
> bit weird for a connection to get the pending transactions and perform
> actions on each transaction.
Quite right. Will fix.
> Inputlink.cpp
> 141: does Subscription automatically call cancel() in its destructor
> if the subscription is active?
It appears not, this section should be fixed.
Thanks again for taking such a close look.
> Transaction support for the WCF channel
> ---------------------------------------
>
> Key: QPID-2128
> URL: https://issues.apache.org/jira/browse/QPID-2128
> Project: Qpid
> Issue Type: New Feature
> Components: WCF/C++ Client
> Affects Versions: 0.6
> Environment: Windows
> Reporter: Cliff Jansen
> Attachments: cppbld.patch, qpid2128_1.patch, wcfbld.patch
>
>
> Transaction support is currently lacking from the WCF channel.
> As an initial implementation, full distributed transaction support from
> within .NET is proposed using the dtx AMQP classes and creating a plugin to
> the MSDTC infrastructure to drive recovery.
> Added performance from promotable transaction enlistment is deferred to a
> later date.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:[email protected]
