Mailing-List: contact dev-help@cloudstack.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@cloudstack.apache.org
Received-SPF: pass (athena.apache.org: domain of trippie@gmail.com designates
 209.85.215.169 as permitted sender)
Subject: Re: [MERGE] txn-refactor
References: 
 <CAMCEDC0xohkCTd105uQkKC6aUiyMRQ9g1jg7KT09U3ndtEvb0A@mail.gmail.com>
 <2AD0D256-8418-4E6E-B5DE-FA456D18EC06@GMAIL.com>
 <84B0AECD-80EE-480E-AAA6-E3E769430B0C@GMAIL.com> <52602889.2080801@gmail.com>
From: Hugo Trippaers <trippie@gmail.com>
Content-Type: text/plain;
	charset=us-ascii
In-Reply-To: <52602889.2080801@gmail.com>
Message-Id: <2473D6CF-0FB9-4E1C-A9CA-C87691874A3C@gmail.com>
Date: Thu, 17 Oct 2013 21:59:54 +0200
To: "dev@cloudstack.apache.org" <dev@cloudstack.apache.org>
Content-Transfer-Encoding: quoted-printable
Mime-Version: 1.0 (1.0)


Sent from my iPhone

> On 17 okt. 2013, at 20:12, Darren Shepherd <darren.s.shepherd@gmail.com> w=
rote:
>=20
>> On 10/17/2013 01:53 AM, Hugo Trippaers wrote:
>> Hey Darren,
>>=20
>> Looking through the code it looks like this more an API change than an ac=
tual redesign of the transaction code? I like the resulting code a lot bette=
r than the existing way of doing it. As far as i can see you wrapped the exi=
sting TransactionLegacy way of doing it (txn.start / txn.commit) inside of t=
he new Transaction functions execute and executeWithException. So if i under=
stand it correctly, nothing changed in how transactions are actually handled=
, except that the code can now be easily changed to use spring TX.
>>=20
> Yes the purpose of this is to introduce a new API that will be compatible w=
ith 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!
>=20
> But know I do need to make this change because I found that people were in=
consistently using the old API and it was causing problems with the spring m=
odularization branch.  In the spring modularization branch I've moved the AO=
P logic for the transactions from custom ACS AOP to Spring AOP which has a s=
lightly different semantics.
>=20
> Also note I found tons of bugs in the transaction handling while doing thi=
s.  Having the anonymous class style has made error handling more consistent=
.  But, there a tons of places that people are catching Throwable/Exception w=
here they shouldn't.  That's a different discussion, I need to put together s=
ome thoughts on more consistent error handling in ACS.
>=20
>> 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 anno=
tate the TransactionLegacy class with @Deprecated so we can easily identify w=
hat needs to be done?
>=20
> I updated all of the management non-DAO code.  AWS, Usage and DAOs are sti=
ll using the old API.  I didn't touch DAOs because its not worth it in my mi=
nd.  DAOs should move to declarative transaction managment which will mean b=
asically 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 declarati=
ve transactions go and change all the DAOs again.

Sounds like a solid approach. Works for me.
>=20
> I don't know if I want to actually put @Deprecated on TransactionLegacy ye=
t because there are things you can do with it that you can't do in the new A=
PI.  Namely, prepared statements and switching databases.

Maybe just mark start() as deprecated then. Would at least put a marker for a=
nybody writing new code that they should think again about using it.

>=20
>>=20
>> The new code is not covered by any new unit test yet? I couldn't check th=
e cobertura result yet as there are some build and test failures. Can you ha=
ve a look at this build : http://jenkins.buildacloud.org/job/cloudstack-mave=
n-build-with-branch-parameter/3/.  You can kick off that job yourself from j=
enkins if you like to do the full test. Didn't do the noredist build test ye=
t as the normal build failed.
>>=20
>=20
> I can write a unit test that tests the new code.  I started by altering th=
e 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 manua=
l testing.

A unit test would be really nice to have for this piece of code. Especially n=
ow we know there will be changes is this area for some time to come. The DB l=
ayer is at the core of CloudStack so a test is a real requirement here. I kn=
ow there is a bunch of stuff disabled, we decided long ago to fix those test=
s 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 app=
ears broken. One test failure and a compile error as far as I can tell.

>=20
> Darren

Cheers,

Hugo=