Sent from my iPhone
> On 17 okt. 2013, at 20:12, Darren Shepherd <darren.s.sheph...@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
