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.
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? 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. In short there are some things to fix before we can merge this. Cheers, Hugo On Oct 17, 2013, at 9:04 AM, Hugo Trippaers <h...@trippaers.nl> wrote: > Hey Darren, > > I'm having a look at the branch. Takes some time so i will get back to you > when i have something. > > Did you run the bvt test suite on this branch already? > > Cheers, > > Hugo > > On Oct 16, 2013, at 6:59 PM, Darren Shepherd <darren.s.sheph...@gmail.com> > wrote: > >> I need as many people as possible to review this branch. I'm still >> testing it out, but I wanted to get as many eyes on it as possible. >> This is a huge cross cutting change. This branch is the changes to >> use a new Transaction API that will be consistent Spring TX's style so >> that we can eventually move to it. You can get a bit more context >> from >> https://cwiki.apache.org/confluence/display/CLOUDSTACK/Database+Transactions >> >> Having spent so much time looking at the transaction management in >> ACS, I'm well convinced we need to adopt Spring TX as soon as we can. >> I've found just too many bugs. It will be a painful transitiion, as >> you can see from this branch. It will also be very tricky, but I'll >> figure it out. >> >> If you are reviewing this branch, use a diff tool that ignores >> whitespace. Also if you don't know about "git difftool -d", you >> should use that. Just know, its was 10x more tedious and painful for >> me to make this change then it is for you to review it :) >> >> Darren >
