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
