Hi Darren, Thank you for explain this issue in huge detail.
Could I respond with some questions? 1. If @DB declarations are not detected, because Java 'assert' is not turned on, can we use a different mechanism to check for @DB? 2. What standard framework do you recommend using for DB transactions? Is there a guide available? 3. Going back to 'assert'. Since they're not being used, should we get rid of all instances of Java 'assert' in the code base? Or should we turn on 'assert' in non-release builds? DL > -----Original Message----- > From: Darren Shepherd [mailto:darren.s.sheph...@gmail.com] > Sent: 09 October 2013 09:45 > To: 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
