cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Hugo Trippaers <trip...@gmail.com>
Subject Re: [MERGE] txn-refactor
Date Thu, 17 Oct 2013 19:59:54 GMT


Sent from my iPhone

> On 17 okt. 2013, at 20:12, Darren Shepherd <darren.s.shepherd@gmail.com> wrote:
> 
>> On 10/17/2013 01:53 AM, Hugo Trippaers wrote:
>> Hey Darren,
>> 
>> Looking through the code it looks like this more an API change than an actual redesign
of the transaction code? I like the resulting code a lot better than the existing way of doing
it. As far as i can see you wrapped the existing TransactionLegacy way of doing it (txn.start
/ txn.commit) inside of the new Transaction functions execute and executeWithException. So
if i understand it correctly, nothing changed in how transactions are actually handled, except
that the code can now be easily changed to use spring TX.
>> 
> Yes the purpose of this is to introduce a new API that will be compatible with Spring
TX.  The internals are all still custom ACS transaction.  This is why I'm comfortable with
this change, because nothing changed too much.

Nice, that also makes it a lot easier to review the changes!
> 
> But know I do need to make this change because I found that people were inconsistently
using the old API and it was causing problems with the spring modularization branch.  In the
spring modularization branch I've moved the AOP logic for the transactions from custom ACS
AOP to Spring AOP which has a slightly different semantics.
> 
> Also note I found tons of bugs in the transaction handling while doing this.  Having
the anonymous class style has made error handling more consistent.  But, there a tons of places
that people are catching Throwable/Exception where they shouldn't.  That's a different discussion,
I need to put together some thoughts on more consistent error handling in ACS.
> 
>> Also you made that changes to a couple of classes to use the new api, but the majority
of the classes still need to be done. It might be nice to annotate the TransactionLegacy class
with @Deprecated so we can easily identify what needs to be done?
> 
> I updated all of the management non-DAO code.  AWS, Usage and DAOs are still using the
old API.  I didn't touch DAOs because its not worth it in my mind.  DAOs should move to declarative
transaction managment which will mean basically the whole method will be under a transactions.
 So I didn't want to convert the code to the anonymous class style, and then when I do declarative
transactions go and change all the DAOs again.

Sounds like a solid approach. Works for me.
> 
> I don't know if I want to actually put @Deprecated on TransactionLegacy yet because there
are things you can do with it that you can't do in the new API.  Namely, prepared statements
and switching databases.

Maybe just mark start() as deprecated then. Would at least put a marker for anybody writing
new code that they should think again about using it.

> 
>> 
>> The new code is not covered by any new unit test yet? I couldn't check the cobertura
result yet as there are some build and test failures. Can you have a look at this build :
http://jenkins.buildacloud.org/job/cloudstack-maven-build-with-branch-parameter/3/.  You can
kick off that job yourself from jenkins if you like to do the full test. Didn't do the noredist
build test yet as the normal build failed.
>> 
> 
> I can write a unit test that tests the new code.  I started by altering the existing
transaction unit test, but then later figured out they don't work and are not being ran as
part of the build.  So I just did a bunch of manual testing.

A unit test would be really nice to have for this piece of code. Especially now we know there
will be changes is this area for some time to come. The DB layer is at the core of CloudStack
so a test is a real requirement here. I know there is a bunch of stuff disabled, we decided
long ago to fix those tests when we would touch that bit of code, and you just hit the jackpot
;-)

Did you have a look at the build link? The current build for your branch appears broken. One
test failure and a compile error as far as I can tell.

> 
> Darren

Cheers,

Hugo
Mime
View raw message