cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Darren Shepherd <darren.s.sheph...@gmail.com>
Subject Re: [MERGE] txn-refactor
Date Thu, 17 Oct 2013 18:12:25 GMT
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.

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.

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.

>
> 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.

Darren

Mime
View raw message