Darren, Thank you for the great investigative work. As for the assert, I think that if no one else at least Jenkins should run with asserts on, so we can see if a commit breaks something.
As for the framework, I think your proposed solution sound good. I am not a big fan of spring. It does some things very good but it tries to do to much. If you say it is a good framework for transaction management I have to trust you as I didn't use that for more then five years. I would certainly not let you in the pit as sole owner of the change though as I have been in a short struggle with what you are trying to solve as well. I would like to hear from the originators of the present transaction management though. regards, Daan On Wed, Oct 9, 2013 at 11:38 AM, Frankie Onuonga <fran...@angani.co> wrote: > Hi Darren, > Greetings from Nairobi. > > First, let me start by saying that you for the email . > I personally think the options were explained very well. > > Now on a serious note, I think that it would be good practice to ensure that > we go with a standardized method of doing things. > I therefore support the option to go with a framework. > It should make management of code easier. > Future development should also be much better. > > Kind Regards, > > > Sent from my Windows Phone > ________________________________ > From: Darren Shepherd<mailto:darren.s.sheph...@gmail.com> > Sent: 10/9/2013 11:46 AM > To: dev@cloudstack.apache.org<mailto:dev@cloudstack.apache.org> > Subject: [DISCUSS] Transaction Hell > > Okay, please read this all, this is important... I want you all to > know that its personally important to me to attempt to get rid of ACS > custom stuff and introduce patterns, frameworks, libraries, etc that I > feel are more consistent with modern Java development and are > understood by a wider audience. This is one of the basic reasons I > started the spring-modularization branch. I just want to be able to > leverage Spring in a sane way. The current implementation in ACS is > backwards and broken and abuses Spring to the point that leveraging > Spring isn't really all that possible. > > So while I did the Spring work, I also started laying the ground work > to get rid of the ACS custom transaction management. The custom DAO > framework and the corresponding transaction management has been a huge > barrier to me extending ACS in the past. When you look at how you are > supposed to access the database, it's all very custom and what I feel > isn't really all that straight forward. I was debugging an issue > today and figured out there is a huge bug in what I've done and that > has lead me down this rabbit hole of what the correct solution is. > Additionally ACS custom transaction mgmt is done in a way that > basically breaks Spring too. > > At some point on the mailing list there was a small discussion about > removing the @DB interceptor. The @DB interceptor does txn.open() and > txn.close() around a method. If a method forgets to commit or > rollback the txn, txn.close() will rollback the transaction for the > method. So the general idea of the change was to instead move that > logic to the bottom of the call stack. The assumption being that the > @DB code was just an additional check to ensure the programmer didn't > forget something and we could instead just do that once at the bottom > of the stack. Oh how wrong I was. > > The problem is that developers have relied on the @DB interceptor to > handle rollback for them. So you see the following code quite a bit > > txn.start() > ... > txn.commit() > > And there is no sign of a rollback anywhere. So the rollback will > happen if some exception is thrown. By moving the @DB logic to the > bottom of stack what happens is the transaction is not rolled back > when the developer thought it would and madness ensues. So that > change was bad. So what to do.... Here's my totally bias description > of solutions: > > Option A or "Custom Forever!": Go back to custom ACS AOP and the @DB. > This is what one would think is the simplest and safest solution. > We'll it ain't really. Here's the killer problem, besides that fact > that it makes me feel very sad inside, the current rollback behavior > is broken in certain spots in ACS. While investigating possible > solutions I started looking at all the places that do programmatic txn > management. It's important to realize that the txn framework only > works properly if the method in which you do txn.start() has @DB on > it. There is a java assert in currentTxn() that attempts to make sure > that @DB is there. But.... nobody runs with asserts on. So there are > places in ACS where transactions are started and no @DB is there, but > it happens to work because some method in the stack has @DB. So to > properly go back to option A we really need to fix all places that > don't have @DB, plus make sure people always run with asserts on. And > then give up making the ACS world a better place and just do things > how we always have... > > Option B or "Progress is Good": The current transaction management > approach (especially rollback) doesn't match how the majority of > frameworks out there work. This option is to change the Transaction > class API to be more consistent with standard frameworks out there. I > propose the following APIs (if you know Spring TX mgmt, this will look > familiar) > > 1) remove start(), commit(), rollback() - The easiest way to ensure we > up date everything properly is to break the API and fix everything > that is broken (about 433 places) > 2) Add execute(TransactionCallback) where TransactionCallback has one > method doInTransaction(). For somebody to run a transaction you would > need to do > > txn.execute(new TransactionCallback() { > Object doInTransaction() { > // do stuff > } > }) > 3) add "Object startTransaction()," commit(Object), and > rollback(Object) - These methods are for callers who really really > want to do thing programmatically. To run a transaction you would do > > Object status = txn.startTransaction() > try { > //.. do stuff > txn.commit(status) > } catch (Exception e) { > txn.rollback(status) > } > > I'm perfectly willing to go and change all the code for this. It will > just take a couple hours or so. Option B is purposely almost exactly > like Spring PlatformTransactionManager. The reason being if we switch > all the code to this style, we can later drop the implementation of > Transaction and move to 100% fully Spring TX managed. > > Just as a final point, every custom approach or framework we have adds > a barrier to people extending ACS and additionally puts more burden on > the ACS community as that is more code we have to support. If > somebody today wants to know how DB transaction propagation works, > there's zero documentation on it and probably 2 people who know how it > works. If we use a standard framework then somebody can just refer to > that frameworks documentation, community, or stackexchange. > > So either option we can do and I'm opening this up to the community to > decide. If we go with Option A, a small portion of me will die > inside, but so be it. > > Darren
