You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Darren Shepherd <da...@gmail.com> on 2013/10/16 18:59:00 UTC
[MERGE] txn-refactor
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
Re: [MERGE] txn-refactor
Posted by Darren Shepherd <da...@gmail.com>.
Hmm, what did Alex post a while back? Can't say I really follow what
you are saying.
Darren
On Tue, Oct 22, 2013 at 12:59 AM, Hugo Trippaers <tr...@gmail.com> wrote:
> +1
>
> Side note, you're doing some automated code cleanup as well. Pretty nice, but did you sync the cleanup settings with the ones Alex posted a while back? We wouldn't want conflicting autocleanup settings battling for supremacy.
>
> Sent from my iPhone
>
>> On 22 okt. 2013, at 06:21, Darren Shepherd <da...@gmail.com> wrote:
>>
>> I plan to merge this wed morning unless I or others find issues.
>>
>> Darren
>>
>>>> On Oct 17, 2013, at 3:05 PM, Darren Shepherd <da...@gmail.com> wrote:
>>>>
>>>> On Thu, Oct 17, 2013 at 12:59 PM, Hugo Trippaers <tr...@gmail.com> wrote:
>>>> 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.
>>>
>>> Good idea.
>>>
>>>>
>>>> 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 ;-)
>>>
>>> I wrote some tests, I'll commit them in a bit.
>>>
>>>> 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.
>>>
>>> I had no clue that build-with-branch jenkins jobs existed! That is
>>> very useful. How do I get an account to kick off jobs? I do feel
>>> stupid now looking at the build error. I had some maven projects
>>> disabled, so I forgot to update about 10 different projects. I'll fix
>>> those real quick and commit.
>>>
>>> Darren
Re: [MERGE] txn-refactor
Posted by Hugo Trippaers <tr...@gmail.com>.
+1
Side note, you're doing some automated code cleanup as well. Pretty nice, but did you sync the cleanup settings with the ones Alex posted a while back? We wouldn't want conflicting autocleanup settings battling for supremacy.
Sent from my iPhone
> On 22 okt. 2013, at 06:21, Darren Shepherd <da...@gmail.com> wrote:
>
> I plan to merge this wed morning unless I or others find issues.
>
> Darren
>
>>> On Oct 17, 2013, at 3:05 PM, Darren Shepherd <da...@gmail.com> wrote:
>>>
>>> On Thu, Oct 17, 2013 at 12:59 PM, Hugo Trippaers <tr...@gmail.com> wrote:
>>> 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.
>>
>> Good idea.
>>
>>>
>>> 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 ;-)
>>
>> I wrote some tests, I'll commit them in a bit.
>>
>>> 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.
>>
>> I had no clue that build-with-branch jenkins jobs existed! That is
>> very useful. How do I get an account to kick off jobs? I do feel
>> stupid now looking at the build error. I had some maven projects
>> disabled, so I forgot to update about 10 different projects. I'll fix
>> those real quick and commit.
>>
>> Darren
Re: [MERGE] txn-refactor
Posted by Darren Shepherd <da...@gmail.com>.
I plan to merge this wed morning unless I or others find issues.
Darren
> On Oct 17, 2013, at 3:05 PM, Darren Shepherd <da...@gmail.com> wrote:
>
>> On Thu, Oct 17, 2013 at 12:59 PM, Hugo Trippaers <tr...@gmail.com> wrote:
>> 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.
>
> Good idea.
>
>>
>> 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 ;-)
>
> I wrote some tests, I'll commit them in a bit.
>
>> 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.
>
> I had no clue that build-with-branch jenkins jobs existed! That is
> very useful. How do I get an account to kick off jobs? I do feel
> stupid now looking at the build error. I had some maven projects
> disabled, so I forgot to update about 10 different projects. I'll fix
> those real quick and commit.
>
> Darren
Re: [MERGE] txn-refactor
Posted by Darren Shepherd <da...@gmail.com>.
On Thu, Oct 17, 2013 at 12:59 PM, Hugo Trippaers <tr...@gmail.com> wrote:
> 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.
Good idea.
>
> 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 ;-)
>
I wrote some tests, I'll commit them in a bit.
> 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.
I had no clue that build-with-branch jenkins jobs existed! That is
very useful. How do I get an account to kick off jobs? I do feel
stupid now looking at the build error. I had some maven projects
disabled, so I forgot to update about 10 different projects. I'll fix
those real quick and commit.
Darren
Re: [MERGE] txn-refactor
Posted by Hugo Trippaers <tr...@gmail.com>.
Sent from my iPhone
> On 17 okt. 2013, at 20:12, Darren Shepherd <da...@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
Re: [MERGE] txn-refactor
Posted by Darren Shepherd <da...@gmail.com>.
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
Re: [MERGE] txn-refactor
Posted by Hugo Trippaers <hu...@trippaers.nl>.
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 <hu...@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 <da...@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
>
Re: [MERGE] txn-refactor
Posted by Hugo Trippaers <hu...@trippaers.nl>.
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 <da...@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
Re: [MERGE] txn-refactor
Posted by Sebastien Goasguen <ru...@gmail.com>.
On Oct 16, 2013, at 9:08 PM, Darren Shepherd <da...@gmail.com> wrote:
> How do these merge emails work? Do I just wait and certain amount of time and if nobody says no I just do it? I've tested the branch as much as I'd like, so just waiting for input.
>
I think there is a 72 hours window, but we might want to wait one week to get this one merged ?
> Darren
>
>> On Oct 16, 2013, at 9:59 AM, Darren Shepherd <da...@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
Re: [MERGE] txn-refactor
Posted by Sebastien Goasguen <ru...@gmail.com>.
On Oct 16, 2013, at 9:08 PM, Darren Shepherd <da...@gmail.com> wrote:
> How do these merge emails work? Do I just wait and certain amount of time and if nobody says no I just do it? I've tested the branch as much as I'd like, so just waiting for input.
>
I think there is a 72 hours window, but we might want to wait one week to get this one merged ?
> Darren
>
>> On Oct 16, 2013, at 9:59 AM, Darren Shepherd <da...@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
Re: [MERGE] txn-refactor
Posted by Darren Shepherd <da...@gmail.com>.
How do these merge emails work? Do I just wait and certain amount of time and if nobody says no I just do it? I've tested the branch as much as I'd like, so just waiting for input.
Darren
> On Oct 16, 2013, at 9:59 AM, Darren Shepherd <da...@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