cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Hugo Trippaers <h...@trippaers.nl>
Subject Re: [MERGE] txn-refactor
Date Thu, 17 Oct 2013 08:53:28 GMT
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 <hugo@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.shepherd@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
> 


Mime
View raw message