[DISCUSS] Transaction Hell

[Darren Shepherd <da...@gmail.com>]

Okay, please read this all, this is important...  I want you all to
know that its personally important to me to attempt to get rid of ACS
custom stuff and introduce patterns, frameworks, libraries, etc that I
feel are more consistent with modern Java development and are
understood by a wider audience.  This is one of the basic reasons I
started the spring-modularization branch.  I just want to be able to
leverage Spring in a sane way.  The current implementation in ACS is
backwards and broken and abuses Spring to the point that leveraging
Spring isn't really all that possible.

So while I did the Spring work, I also started laying the ground work
to get rid of the ACS custom transaction management.  The custom DAO
framework and the corresponding transaction management has been a huge
barrier to me extending ACS in the past.  When you look at how you are
supposed to access the database, it's all very custom and what I feel
isn't really all that straight forward.  I was debugging an issue
today and figured out there is a huge bug in what I've done and that
has lead me down this rabbit hole of what the correct solution is.
Additionally ACS custom transaction mgmt is done in a way that
basically breaks Spring too.

At some point on the mailing list there was a small discussion about
removing the @DB interceptor.  The @DB interceptor does txn.open() and
txn.close() around a method.  If a method forgets to commit or
rollback the txn, txn.close() will rollback the transaction for the
method.  So the general idea of the change was to instead move that
logic to the bottom of the call stack.  The assumption being that the
@DB code was just an additional check to ensure the programmer didn't
forget something and we could instead just do that once at the bottom
of the stack.  Oh how wrong I was.

The problem is that developers have relied on the @DB interceptor to
handle rollback for them.  So you see the following code quite a bit

txn.start()
...
txn.commit()

And there is no sign of a rollback anywhere.  So the rollback will
happen if some exception is thrown.  By moving the @DB logic to the
bottom of stack what happens is the transaction is not rolled back
when the developer thought it would and madness ensues.  So that
change was bad.  So what to do....  Here's my totally bias description
of solutions:

Option A or "Custom Forever!":  Go back to custom ACS AOP and the @DB.
 This is what one would think is the simplest and safest solution.
We'll it ain't really.  Here's the killer problem, besides that fact
that it makes me feel very sad inside, the current rollback behavior
is broken in certain spots in ACS.  While investigating possible
solutions I started looking at all the places that do programmatic txn
management.  It's important to realize that the txn framework only
works properly if the method in which you do txn.start() has @DB on
it.  There is a java assert in currentTxn() that attempts to make sure
that @DB is there.  But.... nobody runs with asserts on.  So there are
places in ACS where transactions are started and no @DB is there, but
it happens to work because some method in the stack has @DB.  So to
properly go back to option A we really need to fix all places that
don't have @DB, plus make sure people always run with asserts on.  And
then give up making the ACS world a better place and just do things
how we always have...

Option B or "Progress is Good":  The current transaction management
approach (especially rollback) doesn't match how the majority of
frameworks out there work.  This option is to change the Transaction
class API to be more consistent with standard frameworks out there.  I
propose the following APIs (if you know Spring TX mgmt, this will look
familiar)

1) remove start(), commit(), rollback() - The easiest way to ensure we
up date everything properly is to break the API and fix everything
that is broken (about 433 places)
2) Add execute(TransactionCallback) where TransactionCallback has one
method doInTransaction().  For somebody to run a transaction you would
need to do

txn.execute(new TransactionCallback() {
Object doInTransaction() {
  // do stuff
}
})
3) add "Object startTransaction()," commit(Object), and
rollback(Object) - These methods are for callers who really really
want to do thing programmatically.  To run a transaction you would do

Object status = txn.startTransaction()
try {
  //.. do stuff
  txn.commit(status)
} catch (Exception e) {
  txn.rollback(status)
}

I'm perfectly willing to go and change all the code for this.  It will
just take a couple hours or so.  Option B is purposely almost exactly
like Spring PlatformTransactionManager.  The reason being if we switch
all the code to this style, we can later drop the implementation of
Transaction and move to 100% fully Spring TX managed.

Just as a final point, every custom approach or framework we have adds
a barrier to people extending ACS and additionally puts more burden on
the ACS community as that is more code we have to support.  If
somebody today wants to know how DB transaction propagation works,
there's zero documentation on it and probably 2 people who know how it
works.  If we use a standard framework then somebody can just refer to
that frameworks documentation, community, or stackexchange.

So either option we can do and I'm opening this up to the community to
decide.  If we go with Option A, a small portion of me will die
inside, but so be it.

Darren

Re: [DISCUSS] Transaction Hell

[Mike Tutkowski <mi...@solidfire.com>]

I think Option B is a good move.


On Wed, Oct 9, 2013 at 1:00 PM, Darren Shepherd <darren.s.shepherd@gmail.com
> wrote:

> This is blocking my spring-modularization branch as I coupled changing
> the DB transactions stuff with the spring stuff (maybe not the best
> idea...).  So if everyone is on board with option 2, I'll look to get
> it done by probably Tuesday.  I'll create a branch from master,
> independent of the spring stuff I did.  If all is swell with that,
> merge the DB txn change, and merge the spring changes.
>
> Darren
>
> On Wed, Oct 9, 2013 at 10:38 AM, Chiradeep Vittal
> <Ch...@citrix.com> wrote:
> > +1 to option B (for a lot of the reasons enunciated by Darren).
> > Also, let's get this in right away so that by 1/31/2014 we are confident
> > about the change and fixed any bugs uncovered by the new scheme.
> >
> > On 10/9/13 10:29 AM, "Darren Shepherd" <da...@gmail.com>
> wrote:
> >
> >>Pedro,
> >>
> > >From a high level I think we'd probably agree.  Generally I feel an
> >>IaaS platform is largely a metadata management framework that stores
> >>the "desired" state of the infrastructure and then pro-actively tries
> >>to reconcile the desired state with reality.  So failures should be
> >>recovered from easily as inconsistency will be discovered and
> >>reconciled.  Having sad that, ACS is not at all like that.  It is very
> >>task oriented.  Hopefully I/we/everyone can change that, its a huge
> >>concern of mine.  The general approach in ACS I see is do task X and
> >>hopefully it works.  If it doesn't work, well hopefully we didn't
> >>leave things in an inconsistent state.  If we find it does leave
> >>things in an inconsistent state, write a cleanup thread to fix bad
> >>things in bad states....
> >>
> >>Regarding TX specifically.  This is a huge topic.  I really don't know
> >>where to start.  I have so many complaints with the data access in
> >>ACS.  There's what I'd like to see, but its so far from what it really
> >>is.  Instead I'll address specifically your question.
> >>
> >>I wish we were doing transaction per API, but I don't think that was
> >>ever a consideration.  I do think the sync portion of API commands
> >>should be wrapped in a single transaction.  I really think the
> >>original intention of the Transaction framework was to assist in
> >>cleaning up resources that people always forget to close.  I think
> >>that is mostly it.
> >>
> >>The general guidelines of how I'd like transactions to work would be
> >>
> >>1) Synchronous portions of API commands are wrapped in a single
> >>transaction.  Transaction propagation capability from spring tx can
> >>then handle nesting transaction as more complicated transaction
> >>management may be need in certain places.
> >>
> >>2) Async jobs that run in a background threads should do small fine
> >>grained transaction management.  Ideally no transactions.
> >>Transactions should not be used as a locking mechanism.
> >>
> >>Having said that, there are currently so many technical issues in
> >>getting to that.  For example, with point 1, because IPC/MessageBus
> >>and EventBus were added recently, that makes it difficult to do 1.
> >>The problem is that you can't send a message while a DB tx is open
> >>because the reciever may get the message before the commit.  So
> >>messaging frameworks have to be written in consideration of the
> >>transaction management.  Not saying you need to do complex XA style
> >>transactions, there's simpler ways to do that.  So regarding points 1
> >>and 2 I said.  That's what I'd like to see, but I know its a long road
> >>to that.
> >>
> >>Option B is really about introducing an API that will eventually serve
> >>as a lightweight wrapper around Spring TX.  In the short term, if I do
> >>option B, the implementation of the code will still be the custom ACS
> >>TX mgmt.  So across modules, its sorta kinda works but not really.
> >>But if I do the second step of replacing custom ACS TX impl with
> >>Spring TX, it will follow how Spring TX works.  If we have Sprint TX
> >>we can then leverage the transaction propagation features of it to
> >>more sanely handle transaction nesting.
> >>
> >>I feel I went a bit the weeds with that response, but maybe something
> >>in there made sense.
> >>
> >>Darren
> >>
> >>On Wed, Oct 9, 2013 at 9:31 AM, Pedro Roque Marques
> >><pe...@gmail.com> wrote:
> >>> Darren,
> >>> My assumption when I tried to make sense of the transaction code is
> >>>that the underlying motivation is that the code is trying to create a
> >>>transaction per API call and then allow multiple modules to implement
> >>>that API call...
> >>> i.e. the intent is do use a bit of what i would call a "web-server
> >>>logic"...
> >>>
> >>> 1. API call starts.
> >>> 2. Module X starts transaction...
> >>> 3. Module Y does some other changes in the DB...
> >>> 4. Either the API call completes successfully or not... commit or error
> >>>back to the user.
> >>>
> >>>  I suspect that this was probably the starting point... but it doesn't
> >>>really work as i describe above. Often when the plugin i'm working on
> >>>screws up (or XenServer is misconfigured) one ends up with DB objects in
> >>>inconsistent state.
> >>>
> >>> I suspect that the DB Transaction design needs to include what is the
> >>>methodology for the design of the management server.
> >>>
> >>> In an ideal world, i would say that API calls just check authorization
> >>>and quotas and should store the intent of the management server to reach
> >>>the desired state. State machines that can then deal with transient
> >>>failures should then attempt to move the state of the system to the
> >>>state intended by the user. That however doesn't seem to reflect the
> >>>current state of the management server.
> >>>
> >>> I may be completely wrong... Can you give an example in proposal B of
> >>>how a transaction would span multiple modules of code ?
> >>>
> >>>   Pedro.
> >>>
> >>> On Oct 9, 2013, at 1:44 AM, Darren Shepherd wrote:
> >>>
> >>>> Okay, please read this all, this is important...  I want you all to
> >>>> know that its personally important to me to attempt to get rid of ACS
> >>>> custom stuff and introduce patterns, frameworks, libraries, etc that I
> >>>> feel are more consistent with modern Java development and are
> >>>> understood by a wider audience.  This is one of the basic reasons I
> >>>> started the spring-modularization branch.  I just want to be able to
> >>>> leverage Spring in a sane way.  The current implementation in ACS is
> >>>> backwards and broken and abuses Spring to the point that leveraging
> >>>> Spring isn't really all that possible.
> >>>>
> >>>> So while I did the Spring work, I also started laying the ground work
> >>>> to get rid of the ACS custom transaction management.  The custom DAO
> >>>> framework and the corresponding transaction management has been a huge
> >>>> barrier to me extending ACS in the past.  When you look at how you are
> >>>> supposed to access the database, it's all very custom and what I feel
> >>>> isn't really all that straight forward.  I was debugging an issue
> >>>> today and figured out there is a huge bug in what I've done and that
> >>>> has lead me down this rabbit hole of what the correct solution is.
> >>>> Additionally ACS custom transaction mgmt is done in a way that
> >>>> basically breaks Spring too.
> >>>>
> >>>> At some point on the mailing list there was a small discussion about
> >>>> removing the @DB interceptor.  The @DB interceptor does txn.open() and
> >>>> txn.close() around a method.  If a method forgets to commit or
> >>>> rollback the txn, txn.close() will rollback the transaction for the
> >>>> method.  So the general idea of the change was to instead move that
> >>>> logic to the bottom of the call stack.  The assumption being that the
> >>>> @DB code was just an additional check to ensure the programmer didn't
> >>>> forget something and we could instead just do that once at the bottom
> >>>> of the stack.  Oh how wrong I was.
> >>>>
> >>>> The problem is that developers have relied on the @DB interceptor to
> >>>> handle rollback for them.  So you see the following code quite a bit
> >>>>
> >>>> txn.start()
> >>>> ...
> >>>> txn.commit()
> >>>>
> >>>> And there is no sign of a rollback anywhere.  So the rollback will
> >>>> happen if some exception is thrown.  By moving the @DB logic to the
> >>>> bottom of stack what happens is the transaction is not rolled back
> >>>> when the developer thought it would and madness ensues.  So that
> >>>> change was bad.  So what to do....  Here's my totally bias description
> >>>> of solutions:
> >>>>
> >>>> Option A or "Custom Forever!":  Go back to custom ACS AOP and the @DB.
> >>>> This is what one would think is the simplest and safest solution.
> >>>> We'll it ain't really.  Here's the killer problem, besides that fact
> >>>> that it makes me feel very sad inside, the current rollback behavior
> >>>> is broken in certain spots in ACS.  While investigating possible
> >>>> solutions I started looking at all the places that do programmatic txn
> >>>> management.  It's important to realize that the txn framework only
> >>>> works properly if the method in which you do txn.start() has @DB on
> >>>> it.  There is a java assert in currentTxn() that attempts to make sure
> >>>> that @DB is there.  But.... nobody runs with asserts on.  So there are
> >>>> places in ACS where transactions are started and no @DB is there, but
> >>>> it happens to work because some method in the stack has @DB.  So to
> >>>> properly go back to option A we really need to fix all places that
> >>>> don't have @DB, plus make sure people always run with asserts on.  And
> >>>> then give up making the ACS world a better place and just do things
> >>>> how we always have...
> >>>>
> >>>> Option B or "Progress is Good":  The current transaction management
> >>>> approach (especially rollback) doesn't match how the majority of
> >>>> frameworks out there work.  This option is to change the Transaction
> >>>> class API to be more consistent with standard frameworks out there.  I
> >>>> propose the following APIs (if you know Spring TX mgmt, this will look
> >>>> familiar)
> >>>>
> >>>> 1) remove start(), commit(), rollback() - The easiest way to ensure we
> >>>> up date everything properly is to break the API and fix everything
> >>>> that is broken (about 433 places)
> >>>> 2) Add execute(TransactionCallback) where TransactionCallback has one
> >>>> method doInTransaction().  For somebody to run a transaction you would
> >>>> need to do
> >>>>
> >>>> txn.execute(new TransactionCallback() {
> >>>> Object doInTransaction() {
> >>>>  // do stuff
> >>>> }
> >>>> })
> >>>> 3) add "Object startTransaction()," commit(Object), and
> >>>> rollback(Object) - These methods are for callers who really really
> >>>> want to do thing programmatically.  To run a transaction you would do
> >>>>
> >>>> Object status = txn.startTransaction()
> >>>> try {
> >>>>  //.. do stuff
> >>>>  txn.commit(status)
> >>>> } catch (Exception e) {
> >>>>  txn.rollback(status)
> >>>> }
> >>>>
> >>>> I'm perfectly willing to go and change all the code for this.  It will
> >>>> just take a couple hours or so.  Option B is purposely almost exactly
> >>>> like Spring PlatformTransactionManager.  The reason being if we switch
> >>>> all the code to this style, we can later drop the implementation of
> >>>> Transaction and move to 100% fully Spring TX managed.
> >>>>
> >>>> Just as a final point, every custom approach or framework we have adds
> >>>> a barrier to people extending ACS and additionally puts more burden on
> >>>> the ACS community as that is more code we have to support.  If
> >>>> somebody today wants to know how DB transaction propagation works,
> >>>> there's zero documentation on it and probably 2 people who know how it
> >>>> works.  If we use a standard framework then somebody can just refer to
> >>>> that frameworks documentation, community, or stackexchange.
> >>>>
> >>>> So either option we can do and I'm opening this up to the community to
> >>>> decide.  If we go with Option A, a small portion of me will die
> >>>> inside, but so be it.
> >>>>
> >>>> Darren
> >>>
> >
>



-- 
*Mike Tutkowski*
*Senior CloudStack Developer, SolidFire Inc.*
e: mike.tutkowski@solidfire.com
o: 303.746.7302
Advancing the way the world uses the
cloud<http://solidfire.com/solution/overview/?video=play>
*™*

Re: [DISCUSS] Transaction Hell

[Darren Shepherd <da...@gmail.com>]

This is blocking my spring-modularization branch as I coupled changing
the DB transactions stuff with the spring stuff (maybe not the best
idea...).  So if everyone is on board with option 2, I'll look to get
it done by probably Tuesday.  I'll create a branch from master,
independent of the spring stuff I did.  If all is swell with that,
merge the DB txn change, and merge the spring changes.

Darren

On Wed, Oct 9, 2013 at 10:38 AM, Chiradeep Vittal
<Ch...@citrix.com> wrote:
> +1 to option B (for a lot of the reasons enunciated by Darren).
> Also, let's get this in right away so that by 1/31/2014 we are confident
> about the change and fixed any bugs uncovered by the new scheme.
>
> On 10/9/13 10:29 AM, "Darren Shepherd" <da...@gmail.com> wrote:
>
>>Pedro,
>>
> >From a high level I think we'd probably agree.  Generally I feel an
>>IaaS platform is largely a metadata management framework that stores
>>the "desired" state of the infrastructure and then pro-actively tries
>>to reconcile the desired state with reality.  So failures should be
>>recovered from easily as inconsistency will be discovered and
>>reconciled.  Having sad that, ACS is not at all like that.  It is very
>>task oriented.  Hopefully I/we/everyone can change that, its a huge
>>concern of mine.  The general approach in ACS I see is do task X and
>>hopefully it works.  If it doesn't work, well hopefully we didn't
>>leave things in an inconsistent state.  If we find it does leave
>>things in an inconsistent state, write a cleanup thread to fix bad
>>things in bad states....
>>
>>Regarding TX specifically.  This is a huge topic.  I really don't know
>>where to start.  I have so many complaints with the data access in
>>ACS.  There's what I'd like to see, but its so far from what it really
>>is.  Instead I'll address specifically your question.
>>
>>I wish we were doing transaction per API, but I don't think that was
>>ever a consideration.  I do think the sync portion of API commands
>>should be wrapped in a single transaction.  I really think the
>>original intention of the Transaction framework was to assist in
>>cleaning up resources that people always forget to close.  I think
>>that is mostly it.
>>
>>The general guidelines of how I'd like transactions to work would be
>>
>>1) Synchronous portions of API commands are wrapped in a single
>>transaction.  Transaction propagation capability from spring tx can
>>then handle nesting transaction as more complicated transaction
>>management may be need in certain places.
>>
>>2) Async jobs that run in a background threads should do small fine
>>grained transaction management.  Ideally no transactions.
>>Transactions should not be used as a locking mechanism.
>>
>>Having said that, there are currently so many technical issues in
>>getting to that.  For example, with point 1, because IPC/MessageBus
>>and EventBus were added recently, that makes it difficult to do 1.
>>The problem is that you can't send a message while a DB tx is open
>>because the reciever may get the message before the commit.  So
>>messaging frameworks have to be written in consideration of the
>>transaction management.  Not saying you need to do complex XA style
>>transactions, there's simpler ways to do that.  So regarding points 1
>>and 2 I said.  That's what I'd like to see, but I know its a long road
>>to that.
>>
>>Option B is really about introducing an API that will eventually serve
>>as a lightweight wrapper around Spring TX.  In the short term, if I do
>>option B, the implementation of the code will still be the custom ACS
>>TX mgmt.  So across modules, its sorta kinda works but not really.
>>But if I do the second step of replacing custom ACS TX impl with
>>Spring TX, it will follow how Spring TX works.  If we have Sprint TX
>>we can then leverage the transaction propagation features of it to
>>more sanely handle transaction nesting.
>>
>>I feel I went a bit the weeds with that response, but maybe something
>>in there made sense.
>>
>>Darren
>>
>>On Wed, Oct 9, 2013 at 9:31 AM, Pedro Roque Marques
>><pe...@gmail.com> wrote:
>>> Darren,
>>> My assumption when I tried to make sense of the transaction code is
>>>that the underlying motivation is that the code is trying to create a
>>>transaction per API call and then allow multiple modules to implement
>>>that API call...
>>> i.e. the intent is do use a bit of what i would call a "web-server
>>>logic"...
>>>
>>> 1. API call starts.
>>> 2. Module X starts transaction...
>>> 3. Module Y does some other changes in the DB...
>>> 4. Either the API call completes successfully or not... commit or error
>>>back to the user.
>>>
>>>  I suspect that this was probably the starting point... but it doesn't
>>>really work as i describe above. Often when the plugin i'm working on
>>>screws up (or XenServer is misconfigured) one ends up with DB objects in
>>>inconsistent state.
>>>
>>> I suspect that the DB Transaction design needs to include what is the
>>>methodology for the design of the management server.
>>>
>>> In an ideal world, i would say that API calls just check authorization
>>>and quotas and should store the intent of the management server to reach
>>>the desired state. State machines that can then deal with transient
>>>failures should then attempt to move the state of the system to the
>>>state intended by the user. That however doesn't seem to reflect the
>>>current state of the management server.
>>>
>>> I may be completely wrong... Can you give an example in proposal B of
>>>how a transaction would span multiple modules of code ?
>>>
>>>   Pedro.
>>>
>>> On Oct 9, 2013, at 1:44 AM, Darren Shepherd wrote:
>>>
>>>> Okay, please read this all, this is important...  I want you all to
>>>> know that its personally important to me to attempt to get rid of ACS
>>>> custom stuff and introduce patterns, frameworks, libraries, etc that I
>>>> feel are more consistent with modern Java development and are
>>>> understood by a wider audience.  This is one of the basic reasons I
>>>> started the spring-modularization branch.  I just want to be able to
>>>> leverage Spring in a sane way.  The current implementation in ACS is
>>>> backwards and broken and abuses Spring to the point that leveraging
>>>> Spring isn't really all that possible.
>>>>
>>>> So while I did the Spring work, I also started laying the ground work
>>>> to get rid of the ACS custom transaction management.  The custom DAO
>>>> framework and the corresponding transaction management has been a huge
>>>> barrier to me extending ACS in the past.  When you look at how you are
>>>> supposed to access the database, it's all very custom and what I feel
>>>> isn't really all that straight forward.  I was debugging an issue
>>>> today and figured out there is a huge bug in what I've done and that
>>>> has lead me down this rabbit hole of what the correct solution is.
>>>> Additionally ACS custom transaction mgmt is done in a way that
>>>> basically breaks Spring too.
>>>>
>>>> At some point on the mailing list there was a small discussion about
>>>> removing the @DB interceptor.  The @DB interceptor does txn.open() and
>>>> txn.close() around a method.  If a method forgets to commit or
>>>> rollback the txn, txn.close() will rollback the transaction for the
>>>> method.  So the general idea of the change was to instead move that
>>>> logic to the bottom of the call stack.  The assumption being that the
>>>> @DB code was just an additional check to ensure the programmer didn't
>>>> forget something and we could instead just do that once at the bottom
>>>> of the stack.  Oh how wrong I was.
>>>>
>>>> The problem is that developers have relied on the @DB interceptor to
>>>> handle rollback for them.  So you see the following code quite a bit
>>>>
>>>> txn.start()
>>>> ...
>>>> txn.commit()
>>>>
>>>> And there is no sign of a rollback anywhere.  So the rollback will
>>>> happen if some exception is thrown.  By moving the @DB logic to the
>>>> bottom of stack what happens is the transaction is not rolled back
>>>> when the developer thought it would and madness ensues.  So that
>>>> change was bad.  So what to do....  Here's my totally bias description
>>>> of solutions:
>>>>
>>>> Option A or "Custom Forever!":  Go back to custom ACS AOP and the @DB.
>>>> This is what one would think is the simplest and safest solution.
>>>> We'll it ain't really.  Here's the killer problem, besides that fact
>>>> that it makes me feel very sad inside, the current rollback behavior
>>>> is broken in certain spots in ACS.  While investigating possible
>>>> solutions I started looking at all the places that do programmatic txn
>>>> management.  It's important to realize that the txn framework only
>>>> works properly if the method in which you do txn.start() has @DB on
>>>> it.  There is a java assert in currentTxn() that attempts to make sure
>>>> that @DB is there.  But.... nobody runs with asserts on.  So there are
>>>> places in ACS where transactions are started and no @DB is there, but
>>>> it happens to work because some method in the stack has @DB.  So to
>>>> properly go back to option A we really need to fix all places that
>>>> don't have @DB, plus make sure people always run with asserts on.  And
>>>> then give up making the ACS world a better place and just do things
>>>> how we always have...
>>>>
>>>> Option B or "Progress is Good":  The current transaction management
>>>> approach (especially rollback) doesn't match how the majority of
>>>> frameworks out there work.  This option is to change the Transaction
>>>> class API to be more consistent with standard frameworks out there.  I
>>>> propose the following APIs (if you know Spring TX mgmt, this will look
>>>> familiar)
>>>>
>>>> 1) remove start(), commit(), rollback() - The easiest way to ensure we
>>>> up date everything properly is to break the API and fix everything
>>>> that is broken (about 433 places)
>>>> 2) Add execute(TransactionCallback) where TransactionCallback has one
>>>> method doInTransaction().  For somebody to run a transaction you would
>>>> need to do
>>>>
>>>> txn.execute(new TransactionCallback() {
>>>> Object doInTransaction() {
>>>>  // do stuff
>>>> }
>>>> })
>>>> 3) add "Object startTransaction()," commit(Object), and
>>>> rollback(Object) - These methods are for callers who really really
>>>> want to do thing programmatically.  To run a transaction you would do
>>>>
>>>> Object status = txn.startTransaction()
>>>> try {
>>>>  //.. do stuff
>>>>  txn.commit(status)
>>>> } catch (Exception e) {
>>>>  txn.rollback(status)
>>>> }
>>>>
>>>> I'm perfectly willing to go and change all the code for this.  It will
>>>> just take a couple hours or so.  Option B is purposely almost exactly
>>>> like Spring PlatformTransactionManager.  The reason being if we switch
>>>> all the code to this style, we can later drop the implementation of
>>>> Transaction and move to 100% fully Spring TX managed.
>>>>
>>>> Just as a final point, every custom approach or framework we have adds
>>>> a barrier to people extending ACS and additionally puts more burden on
>>>> the ACS community as that is more code we have to support.  If
>>>> somebody today wants to know how DB transaction propagation works,
>>>> there's zero documentation on it and probably 2 people who know how it
>>>> works.  If we use a standard framework then somebody can just refer to
>>>> that frameworks documentation, community, or stackexchange.
>>>>
>>>> So either option we can do and I'm opening this up to the community to
>>>> decide.  If we go with Option A, a small portion of me will die
>>>> inside, but so be it.
>>>>
>>>> Darren
>>>
>

Re: [DISCUSS] Transaction Hell

[Darren Shepherd <da...@gmail.com>]

Pedro,

The @DB annotation adds a guard.  It doesn't actually start or end a
transaction.  It will check that the transaction should be rolled back
if one was started and not closed and that's about it.  So the only
time transactions are really started is when you see txn.start() in
the code.  So the flow of 1, 2, 3 that you wrote, has almost nothing
to do with the actual transactions in play.

And this further illustrates how confusing this framework is and why I
would like to abolish it.  The @DB annotation should go away.

The example you gave is code that runs in the background as part of an
async job.  What I would propose is that we do not rely on transaction
as much as possible.  Transactions in background code should be as
small as possible.  There is no point in trying to manage failures
with transactions when the external resources we are interacting with
are not transactional.  So backend code should be written mostly as

1) set to transitioning state
2) make external thing so
3) set to done/finished/non-transitioning-state

At any point there is no long running DB transaction.  DB transactions
are only used for atomic commits if your change spans a couple rows
and needs to be atomic.  If there is failure at any point because of
the states we can recover.

The only time I'd like DB transactions to be used is on the
synchronous portion of APIs.  The sync portion of APIs typically just
writes data to the DB and nothing else, it is not supposed to interact
with external resources.  For sync APIs, one big transaction typically
handles 90% of the failure cases.  But I don't think this is fully
possible right now for sync APIs.

Darren

Darren

On Wed, Oct 9, 2013 at 10:58 AM, Pedro Roque Marques
<pe...@gmail.com> wrote:
> Darren,
> I generally agree with you... just trying to point out what could be pitfalls on the way to evolve the system.
>
> On Oct 9, 2013, at 10:29 AM, Darren Shepherd wrote:
>>
>> I wish we were doing transaction per API, but I don't think that was
>> ever a consideration.  I do think the sync portion of API commands
>> should be wrapped in a single transaction.  I really think the
>> original intention of the Transaction framework was to assist in
>> cleaning up resources that people always forget to close.  I think
>> that is mostly it.
>
> My understanding is that for instance when a VM is created you have a call flow that looks a bit like:
>
> 1. UserVmManagerImpl.createVirtualMachine (@DB, persist)
> 2. VirtualMachineManagerImpl.allocate (@DB, persist)
> 3. NetworkOrchestrator.allocate (@DB, persist)
>
> My understanding is that an check in NetworkOrchestrator (e.g. nic parameters not being kosher) is supposed to rollback the transaction and remove the VM in the database...
>
> There are some errors for which this mechanism works OK today.... I believe it would be desirable to have a proposal of how to deal with such an example and then attempt to implement it consistently. Even if it requires the programmer to understand that it needs to explicitly rollback the VM if the underlying layers throw an exception.
>
>   Pedro.

Re: [DISCUSS] Transaction Hell

[Pedro Roque Marques <pe...@gmail.com>]

Darren,
I generally agree with you... just trying to point out what could be pitfalls on the way to evolve the system.

On Oct 9, 2013, at 10:29 AM, Darren Shepherd wrote:
> 
> I wish we were doing transaction per API, but I don't think that was
> ever a consideration.  I do think the sync portion of API commands
> should be wrapped in a single transaction.  I really think the
> original intention of the Transaction framework was to assist in
> cleaning up resources that people always forget to close.  I think
> that is mostly it.

My understanding is that for instance when a VM is created you have a call flow that looks a bit like:

1. UserVmManagerImpl.createVirtualMachine (@DB, persist)
2. VirtualMachineManagerImpl.allocate (@DB, persist)
3. NetworkOrchestrator.allocate (@DB, persist)

My understanding is that an check in NetworkOrchestrator (e.g. nic parameters not being kosher) is supposed to rollback the transaction and remove the VM in the database...

There are some errors for which this mechanism works OK today.... I believe it would be desirable to have a proposal of how to deal with such an example and then attempt to implement it consistently. Even if it requires the programmer to understand that it needs to explicitly rollback the VM if the underlying layers throw an exception.

  Pedro.

Re: [DISCUSS] Transaction Hell

[Daan Hoogland <da...@gmail.com>]

Darren,

Happy to hear your view on Spring.

+1 for option B


On Wed, Oct 9, 2013 at 8:06 PM, Kelven Yang <ke...@citrix.com> wrote:

> +1
>
> Original Transaction class also has many tightly-coupled assumptions about
> the underlying data source, lock master. Developers are usually lost on
> when and where they should use @DB, for nested transactions, it does not
> really work as expected.
>
> Kelven
>
>
> On 10/9/13 10:38 AM, "Chiradeep Vittal" <Ch...@citrix.com>
> wrote:
>
> >+1 to option B (for a lot of the reasons enunciated by Darren).
> >Also, let's get this in right away so that by 1/31/2014 we are confident
> >about the change and fixed any bugs uncovered by the new scheme.
> >
> >On 10/9/13 10:29 AM, "Darren Shepherd" <da...@gmail.com>
> >wrote:
> >
> >>Pedro,
> >>
> >>From a high level I think we'd probably agree.  Generally I feel an
> >>IaaS platform is largely a metadata management framework that stores
> >>the "desired" state of the infrastructure and then pro-actively tries
> >>to reconcile the desired state with reality.  So failures should be
> >>recovered from easily as inconsistency will be discovered and
> >>reconciled.  Having sad that, ACS is not at all like that.  It is very
> >>task oriented.  Hopefully I/we/everyone can change that, its a huge
> >>concern of mine.  The general approach in ACS I see is do task X and
> >>hopefully it works.  If it doesn't work, well hopefully we didn't
> >>leave things in an inconsistent state.  If we find it does leave
> >>things in an inconsistent state, write a cleanup thread to fix bad
> >>things in bad states....
> >>
> >>Regarding TX specifically.  This is a huge topic.  I really don't know
> >>where to start.  I have so many complaints with the data access in
> >>ACS.  There's what I'd like to see, but its so far from what it really
> >>is.  Instead I'll address specifically your question.
> >>
> >>I wish we were doing transaction per API, but I don't think that was
> >>ever a consideration.  I do think the sync portion of API commands
> >>should be wrapped in a single transaction.  I really think the
> >>original intention of the Transaction framework was to assist in
> >>cleaning up resources that people always forget to close.  I think
> >>that is mostly it.
> >>
> >>The general guidelines of how I'd like transactions to work would be
> >>
> >>1) Synchronous portions of API commands are wrapped in a single
> >>transaction.  Transaction propagation capability from spring tx can
> >>then handle nesting transaction as more complicated transaction
> >>management may be need in certain places.
> >>
> >>2) Async jobs that run in a background threads should do small fine
> >>grained transaction management.  Ideally no transactions.
> >>Transactions should not be used as a locking mechanism.
> >>
> >>Having said that, there are currently so many technical issues in
> >>getting to that.  For example, with point 1, because IPC/MessageBus
> >>and EventBus were added recently, that makes it difficult to do 1.
> >>The problem is that you can't send a message while a DB tx is open
> >>because the reciever may get the message before the commit.  So
> >>messaging frameworks have to be written in consideration of the
> >>transaction management.  Not saying you need to do complex XA style
> >>transactions, there's simpler ways to do that.  So regarding points 1
> >>and 2 I said.  That's what I'd like to see, but I know its a long road
> >>to that.
> >>
> >>Option B is really about introducing an API that will eventually serve
> >>as a lightweight wrapper around Spring TX.  In the short term, if I do
> >>option B, the implementation of the code will still be the custom ACS
> >>TX mgmt.  So across modules, its sorta kinda works but not really.
> >>But if I do the second step of replacing custom ACS TX impl with
> >>Spring TX, it will follow how Spring TX works.  If we have Sprint TX
> >>we can then leverage the transaction propagation features of it to
> >>more sanely handle transaction nesting.
> >>
> >>I feel I went a bit the weeds with that response, but maybe something
> >>in there made sense.
> >>
> >>Darren
> >>
> >>On Wed, Oct 9, 2013 at 9:31 AM, Pedro Roque Marques
> >><pe...@gmail.com> wrote:
> >>> Darren,
> >>> My assumption when I tried to make sense of the transaction code is
> >>>that the underlying motivation is that the code is trying to create a
> >>>transaction per API call and then allow multiple modules to implement
> >>>that API call...
> >>> i.e. the intent is do use a bit of what i would call a "web-server
> >>>logic"...
> >>>
> >>> 1. API call starts.
> >>> 2. Module X starts transaction...
> >>> 3. Module Y does some other changes in the DB...
> >>> 4. Either the API call completes successfully or not... commit or error
> >>>back to the user.
> >>>
> >>>  I suspect that this was probably the starting point... but it doesn't
> >>>really work as i describe above. Often when the plugin i'm working on
> >>>screws up (or XenServer is misconfigured) one ends up with DB objects in
> >>>inconsistent state.
> >>>
> >>> I suspect that the DB Transaction design needs to include what is the
> >>>methodology for the design of the management server.
> >>>
> >>> In an ideal world, i would say that API calls just check authorization
> >>>and quotas and should store the intent of the management server to reach
> >>>the desired state. State machines that can then deal with transient
> >>>failures should then attempt to move the state of the system to the
> >>>state intended by the user. That however doesn't seem to reflect the
> >>>current state of the management server.
> >>>
> >>> I may be completely wrong... Can you give an example in proposal B of
> >>>how a transaction would span multiple modules of code ?
> >>>
> >>>   Pedro.
> >>>
> >>> On Oct 9, 2013, at 1:44 AM, Darren Shepherd wrote:
> >>>
> >>>> Okay, please read this all, this is important...  I want you all to
> >>>> know that its personally important to me to attempt to get rid of ACS
> >>>> custom stuff and introduce patterns, frameworks, libraries, etc that I
> >>>> feel are more consistent with modern Java development and are
> >>>> understood by a wider audience.  This is one of the basic reasons I
> >>>> started the spring-modularization branch.  I just want to be able to
> >>>> leverage Spring in a sane way.  The current implementation in ACS is
> >>>> backwards and broken and abuses Spring to the point that leveraging
> >>>> Spring isn't really all that possible.
> >>>>
> >>>> So while I did the Spring work, I also started laying the ground work
> >>>> to get rid of the ACS custom transaction management.  The custom DAO
> >>>> framework and the corresponding transaction management has been a huge
> >>>> barrier to me extending ACS in the past.  When you look at how you are
> >>>> supposed to access the database, it's all very custom and what I feel
> >>>> isn't really all that straight forward.  I was debugging an issue
> >>>> today and figured out there is a huge bug in what I've done and that
> >>>> has lead me down this rabbit hole of what the correct solution is.
> >>>> Additionally ACS custom transaction mgmt is done in a way that
> >>>> basically breaks Spring too.
> >>>>
> >>>> At some point on the mailing list there was a small discussion about
> >>>> removing the @DB interceptor.  The @DB interceptor does txn.open() and
> >>>> txn.close() around a method.  If a method forgets to commit or
> >>>> rollback the txn, txn.close() will rollback the transaction for the
> >>>> method.  So the general idea of the change was to instead move that
> >>>> logic to the bottom of the call stack.  The assumption being that the
> >>>> @DB code was just an additional check to ensure the programmer didn't
> >>>> forget something and we could instead just do that once at the bottom
> >>>> of the stack.  Oh how wrong I was.
> >>>>
> >>>> The problem is that developers have relied on the @DB interceptor to
> >>>> handle rollback for them.  So you see the following code quite a bit
> >>>>
> >>>> txn.start()
> >>>> ...
> >>>> txn.commit()
> >>>>
> >>>> And there is no sign of a rollback anywhere.  So the rollback will
> >>>> happen if some exception is thrown.  By moving the @DB logic to the
> >>>> bottom of stack what happens is the transaction is not rolled back
> >>>> when the developer thought it would and madness ensues.  So that
> >>>> change was bad.  So what to do....  Here's my totally bias description
> >>>> of solutions:
> >>>>
> >>>> Option A or "Custom Forever!":  Go back to custom ACS AOP and the @DB.
> >>>> This is what one would think is the simplest and safest solution.
> >>>> We'll it ain't really.  Here's the killer problem, besides that fact
> >>>> that it makes me feel very sad inside, the current rollback behavior
> >>>> is broken in certain spots in ACS.  While investigating possible
> >>>> solutions I started looking at all the places that do programmatic txn
> >>>> management.  It's important to realize that the txn framework only
> >>>> works properly if the method in which you do txn.start() has @DB on
> >>>> it.  There is a java assert in currentTxn() that attempts to make sure
> >>>> that @DB is there.  But.... nobody runs with asserts on.  So there are
> >>>> places in ACS where transactions are started and no @DB is there, but
> >>>> it happens to work because some method in the stack has @DB.  So to
> >>>> properly go back to option A we really need to fix all places that
> >>>> don't have @DB, plus make sure people always run with asserts on.  And
> >>>> then give up making the ACS world a better place and just do things
> >>>> how we always have...
> >>>>
> >>>> Option B or "Progress is Good":  The current transaction management
> >>>> approach (especially rollback) doesn't match how the majority of
> >>>> frameworks out there work.  This option is to change the Transaction
> >>>> class API to be more consistent with standard frameworks out there.  I
> >>>> propose the following APIs (if you know Spring TX mgmt, this will look
> >>>> familiar)
> >>>>
> >>>> 1) remove start(), commit(), rollback() - The easiest way to ensure we
> >>>> up date everything properly is to break the API and fix everything
> >>>> that is broken (about 433 places)
> >>>> 2) Add execute(TransactionCallback) where TransactionCallback has one
> >>>> method doInTransaction().  For somebody to run a transaction you would
> >>>> need to do
> >>>>
> >>>> txn.execute(new TransactionCallback() {
> >>>> Object doInTransaction() {
> >>>>  // do stuff
> >>>> }
> >>>> })
> >>>> 3) add "Object startTransaction()," commit(Object), and
> >>>> rollback(Object) - These methods are for callers who really really
> >>>> want to do thing programmatically.  To run a transaction you would do
> >>>>
> >>>> Object status = txn.startTransaction()
> >>>> try {
> >>>>  //.. do stuff
> >>>>  txn.commit(status)
> >>>> } catch (Exception e) {
> >>>>  txn.rollback(status)
> >>>> }
> >>>>
> >>>> I'm perfectly willing to go and change all the code for this.  It will
> >>>> just take a couple hours or so.  Option B is purposely almost exactly
> >>>> like Spring PlatformTransactionManager.  The reason being if we switch
> >>>> all the code to this style, we can later drop the implementation of
> >>>> Transaction and move to 100% fully Spring TX managed.
> >>>>
> >>>> Just as a final point, every custom approach or framework we have adds
> >>>> a barrier to people extending ACS and additionally puts more burden on
> >>>> the ACS community as that is more code we have to support.  If
> >>>> somebody today wants to know how DB transaction propagation works,
> >>>> there's zero documentation on it and probably 2 people who know how it
> >>>> works.  If we use a standard framework then somebody can just refer to
> >>>> that frameworks documentation, community, or stackexchange.
> >>>>
> >>>> So either option we can do and I'm opening this up to the community to
> >>>> decide.  If we go with Option A, a small portion of me will die
> >>>> inside, but so be it.
> >>>>
> >>>> Darren
> >>>
> >
>
>

Re: [DISCUSS] Transaction Hell

[Kelven Yang <ke...@citrix.com>]

+1

Original Transaction class also has many tightly-coupled assumptions about
the underlying data source, lock master. Developers are usually lost on
when and where they should use @DB, for nested transactions, it does not
really work as expected.

Kelven


On 10/9/13 10:38 AM, "Chiradeep Vittal" <Ch...@citrix.com>
wrote:

>+1 to option B (for a lot of the reasons enunciated by Darren).
>Also, let's get this in right away so that by 1/31/2014 we are confident
>about the change and fixed any bugs uncovered by the new scheme.
>
>On 10/9/13 10:29 AM, "Darren Shepherd" <da...@gmail.com>
>wrote:
>
>>Pedro,
>>
>>From a high level I think we'd probably agree.  Generally I feel an
>>IaaS platform is largely a metadata management framework that stores
>>the "desired" state of the infrastructure and then pro-actively tries
>>to reconcile the desired state with reality.  So failures should be
>>recovered from easily as inconsistency will be discovered and
>>reconciled.  Having sad that, ACS is not at all like that.  It is very
>>task oriented.  Hopefully I/we/everyone can change that, its a huge
>>concern of mine.  The general approach in ACS I see is do task X and
>>hopefully it works.  If it doesn't work, well hopefully we didn't
>>leave things in an inconsistent state.  If we find it does leave
>>things in an inconsistent state, write a cleanup thread to fix bad
>>things in bad states....
>>
>>Regarding TX specifically.  This is a huge topic.  I really don't know
>>where to start.  I have so many complaints with the data access in
>>ACS.  There's what I'd like to see, but its so far from what it really
>>is.  Instead I'll address specifically your question.
>>
>>I wish we were doing transaction per API, but I don't think that was
>>ever a consideration.  I do think the sync portion of API commands
>>should be wrapped in a single transaction.  I really think the
>>original intention of the Transaction framework was to assist in
>>cleaning up resources that people always forget to close.  I think
>>that is mostly it.
>>
>>The general guidelines of how I'd like transactions to work would be
>>
>>1) Synchronous portions of API commands are wrapped in a single
>>transaction.  Transaction propagation capability from spring tx can
>>then handle nesting transaction as more complicated transaction
>>management may be need in certain places.
>>
>>2) Async jobs that run in a background threads should do small fine
>>grained transaction management.  Ideally no transactions.
>>Transactions should not be used as a locking mechanism.
>>
>>Having said that, there are currently so many technical issues in
>>getting to that.  For example, with point 1, because IPC/MessageBus
>>and EventBus were added recently, that makes it difficult to do 1.
>>The problem is that you can't send a message while a DB tx is open
>>because the reciever may get the message before the commit.  So
>>messaging frameworks have to be written in consideration of the
>>transaction management.  Not saying you need to do complex XA style
>>transactions, there's simpler ways to do that.  So regarding points 1
>>and 2 I said.  That's what I'd like to see, but I know its a long road
>>to that.
>>
>>Option B is really about introducing an API that will eventually serve
>>as a lightweight wrapper around Spring TX.  In the short term, if I do
>>option B, the implementation of the code will still be the custom ACS
>>TX mgmt.  So across modules, its sorta kinda works but not really.
>>But if I do the second step of replacing custom ACS TX impl with
>>Spring TX, it will follow how Spring TX works.  If we have Sprint TX
>>we can then leverage the transaction propagation features of it to
>>more sanely handle transaction nesting.
>>
>>I feel I went a bit the weeds with that response, but maybe something
>>in there made sense.
>>
>>Darren
>>
>>On Wed, Oct 9, 2013 at 9:31 AM, Pedro Roque Marques
>><pe...@gmail.com> wrote:
>>> Darren,
>>> My assumption when I tried to make sense of the transaction code is
>>>that the underlying motivation is that the code is trying to create a
>>>transaction per API call and then allow multiple modules to implement
>>>that API call...
>>> i.e. the intent is do use a bit of what i would call a "web-server
>>>logic"...
>>>
>>> 1. API call starts.
>>> 2. Module X starts transaction...
>>> 3. Module Y does some other changes in the DB...
>>> 4. Either the API call completes successfully or not... commit or error
>>>back to the user.
>>>
>>>  I suspect that this was probably the starting point... but it doesn't
>>>really work as i describe above. Often when the plugin i'm working on
>>>screws up (or XenServer is misconfigured) one ends up with DB objects in
>>>inconsistent state.
>>>
>>> I suspect that the DB Transaction design needs to include what is the
>>>methodology for the design of the management server.
>>>
>>> In an ideal world, i would say that API calls just check authorization
>>>and quotas and should store the intent of the management server to reach
>>>the desired state. State machines that can then deal with transient
>>>failures should then attempt to move the state of the system to the
>>>state intended by the user. That however doesn't seem to reflect the
>>>current state of the management server.
>>>
>>> I may be completely wrong... Can you give an example in proposal B of
>>>how a transaction would span multiple modules of code ?
>>>
>>>   Pedro.
>>>
>>> On Oct 9, 2013, at 1:44 AM, Darren Shepherd wrote:
>>>
>>>> Okay, please read this all, this is important...  I want you all to
>>>> know that its personally important to me to attempt to get rid of ACS
>>>> custom stuff and introduce patterns, frameworks, libraries, etc that I
>>>> feel are more consistent with modern Java development and are
>>>> understood by a wider audience.  This is one of the basic reasons I
>>>> started the spring-modularization branch.  I just want to be able to
>>>> leverage Spring in a sane way.  The current implementation in ACS is
>>>> backwards and broken and abuses Spring to the point that leveraging
>>>> Spring isn't really all that possible.
>>>>
>>>> So while I did the Spring work, I also started laying the ground work
>>>> to get rid of the ACS custom transaction management.  The custom DAO
>>>> framework and the corresponding transaction management has been a huge
>>>> barrier to me extending ACS in the past.  When you look at how you are
>>>> supposed to access the database, it's all very custom and what I feel
>>>> isn't really all that straight forward.  I was debugging an issue
>>>> today and figured out there is a huge bug in what I've done and that
>>>> has lead me down this rabbit hole of what the correct solution is.
>>>> Additionally ACS custom transaction mgmt is done in a way that
>>>> basically breaks Spring too.
>>>>
>>>> At some point on the mailing list there was a small discussion about
>>>> removing the @DB interceptor.  The @DB interceptor does txn.open() and
>>>> txn.close() around a method.  If a method forgets to commit or
>>>> rollback the txn, txn.close() will rollback the transaction for the
>>>> method.  So the general idea of the change was to instead move that
>>>> logic to the bottom of the call stack.  The assumption being that the
>>>> @DB code was just an additional check to ensure the programmer didn't
>>>> forget something and we could instead just do that once at the bottom
>>>> of the stack.  Oh how wrong I was.
>>>>
>>>> The problem is that developers have relied on the @DB interceptor to
>>>> handle rollback for them.  So you see the following code quite a bit
>>>>
>>>> txn.start()
>>>> ...
>>>> txn.commit()
>>>>
>>>> And there is no sign of a rollback anywhere.  So the rollback will
>>>> happen if some exception is thrown.  By moving the @DB logic to the
>>>> bottom of stack what happens is the transaction is not rolled back
>>>> when the developer thought it would and madness ensues.  So that
>>>> change was bad.  So what to do....  Here's my totally bias description
>>>> of solutions:
>>>>
>>>> Option A or "Custom Forever!":  Go back to custom ACS AOP and the @DB.
>>>> This is what one would think is the simplest and safest solution.
>>>> We'll it ain't really.  Here's the killer problem, besides that fact
>>>> that it makes me feel very sad inside, the current rollback behavior
>>>> is broken in certain spots in ACS.  While investigating possible
>>>> solutions I started looking at all the places that do programmatic txn
>>>> management.  It's important to realize that the txn framework only
>>>> works properly if the method in which you do txn.start() has @DB on
>>>> it.  There is a java assert in currentTxn() that attempts to make sure
>>>> that @DB is there.  But.... nobody runs with asserts on.  So there are
>>>> places in ACS where transactions are started and no @DB is there, but
>>>> it happens to work because some method in the stack has @DB.  So to
>>>> properly go back to option A we really need to fix all places that
>>>> don't have @DB, plus make sure people always run with asserts on.  And
>>>> then give up making the ACS world a better place and just do things
>>>> how we always have...
>>>>
>>>> Option B or "Progress is Good":  The current transaction management
>>>> approach (especially rollback) doesn't match how the majority of
>>>> frameworks out there work.  This option is to change the Transaction
>>>> class API to be more consistent with standard frameworks out there.  I
>>>> propose the following APIs (if you know Spring TX mgmt, this will look
>>>> familiar)
>>>>
>>>> 1) remove start(), commit(), rollback() - The easiest way to ensure we
>>>> up date everything properly is to break the API and fix everything
>>>> that is broken (about 433 places)
>>>> 2) Add execute(TransactionCallback) where TransactionCallback has one
>>>> method doInTransaction().  For somebody to run a transaction you would
>>>> need to do
>>>>
>>>> txn.execute(new TransactionCallback() {
>>>> Object doInTransaction() {
>>>>  // do stuff
>>>> }
>>>> })
>>>> 3) add "Object startTransaction()," commit(Object), and
>>>> rollback(Object) - These methods are for callers who really really
>>>> want to do thing programmatically.  To run a transaction you would do
>>>>
>>>> Object status = txn.startTransaction()
>>>> try {
>>>>  //.. do stuff
>>>>  txn.commit(status)
>>>> } catch (Exception e) {
>>>>  txn.rollback(status)
>>>> }
>>>>
>>>> I'm perfectly willing to go and change all the code for this.  It will
>>>> just take a couple hours or so.  Option B is purposely almost exactly
>>>> like Spring PlatformTransactionManager.  The reason being if we switch
>>>> all the code to this style, we can later drop the implementation of
>>>> Transaction and move to 100% fully Spring TX managed.
>>>>
>>>> Just as a final point, every custom approach or framework we have adds
>>>> a barrier to people extending ACS and additionally puts more burden on
>>>> the ACS community as that is more code we have to support.  If
>>>> somebody today wants to know how DB transaction propagation works,
>>>> there's zero documentation on it and probably 2 people who know how it
>>>> works.  If we use a standard framework then somebody can just refer to
>>>> that frameworks documentation, community, or stackexchange.
>>>>
>>>> So either option we can do and I'm opening this up to the community to
>>>> decide.  If we go with Option A, a small portion of me will die
>>>> inside, but so be it.
>>>>
>>>> Darren
>>>
>

Re: [DISCUSS] Transaction Hell

[Chiradeep Vittal <Ch...@citrix.com>]

+1 to option B (for a lot of the reasons enunciated by Darren).
Also, let's get this in right away so that by 1/31/2014 we are confident
about the change and fixed any bugs uncovered by the new scheme.

On 10/9/13 10:29 AM, "Darren Shepherd" <da...@gmail.com> wrote:

>Pedro,
>
>From a high level I think we'd probably agree.  Generally I feel an
>IaaS platform is largely a metadata management framework that stores
>the "desired" state of the infrastructure and then pro-actively tries
>to reconcile the desired state with reality.  So failures should be
>recovered from easily as inconsistency will be discovered and
>reconciled.  Having sad that, ACS is not at all like that.  It is very
>task oriented.  Hopefully I/we/everyone can change that, its a huge
>concern of mine.  The general approach in ACS I see is do task X and
>hopefully it works.  If it doesn't work, well hopefully we didn't
>leave things in an inconsistent state.  If we find it does leave
>things in an inconsistent state, write a cleanup thread to fix bad
>things in bad states....
>
>Regarding TX specifically.  This is a huge topic.  I really don't know
>where to start.  I have so many complaints with the data access in
>ACS.  There's what I'd like to see, but its so far from what it really
>is.  Instead I'll address specifically your question.
>
>I wish we were doing transaction per API, but I don't think that was
>ever a consideration.  I do think the sync portion of API commands
>should be wrapped in a single transaction.  I really think the
>original intention of the Transaction framework was to assist in
>cleaning up resources that people always forget to close.  I think
>that is mostly it.
>
>The general guidelines of how I'd like transactions to work would be
>
>1) Synchronous portions of API commands are wrapped in a single
>transaction.  Transaction propagation capability from spring tx can
>then handle nesting transaction as more complicated transaction
>management may be need in certain places.
>
>2) Async jobs that run in a background threads should do small fine
>grained transaction management.  Ideally no transactions.
>Transactions should not be used as a locking mechanism.
>
>Having said that, there are currently so many technical issues in
>getting to that.  For example, with point 1, because IPC/MessageBus
>and EventBus were added recently, that makes it difficult to do 1.
>The problem is that you can't send a message while a DB tx is open
>because the reciever may get the message before the commit.  So
>messaging frameworks have to be written in consideration of the
>transaction management.  Not saying you need to do complex XA style
>transactions, there's simpler ways to do that.  So regarding points 1
>and 2 I said.  That's what I'd like to see, but I know its a long road
>to that.
>
>Option B is really about introducing an API that will eventually serve
>as a lightweight wrapper around Spring TX.  In the short term, if I do
>option B, the implementation of the code will still be the custom ACS
>TX mgmt.  So across modules, its sorta kinda works but not really.
>But if I do the second step of replacing custom ACS TX impl with
>Spring TX, it will follow how Spring TX works.  If we have Sprint TX
>we can then leverage the transaction propagation features of it to
>more sanely handle transaction nesting.
>
>I feel I went a bit the weeds with that response, but maybe something
>in there made sense.
>
>Darren
>
>On Wed, Oct 9, 2013 at 9:31 AM, Pedro Roque Marques
><pe...@gmail.com> wrote:
>> Darren,
>> My assumption when I tried to make sense of the transaction code is
>>that the underlying motivation is that the code is trying to create a
>>transaction per API call and then allow multiple modules to implement
>>that API call...
>> i.e. the intent is do use a bit of what i would call a "web-server
>>logic"...
>>
>> 1. API call starts.
>> 2. Module X starts transaction...
>> 3. Module Y does some other changes in the DB...
>> 4. Either the API call completes successfully or not... commit or error
>>back to the user.
>>
>>  I suspect that this was probably the starting point... but it doesn't
>>really work as i describe above. Often when the plugin i'm working on
>>screws up (or XenServer is misconfigured) one ends up with DB objects in
>>inconsistent state.
>>
>> I suspect that the DB Transaction design needs to include what is the
>>methodology for the design of the management server.
>>
>> In an ideal world, i would say that API calls just check authorization
>>and quotas and should store the intent of the management server to reach
>>the desired state. State machines that can then deal with transient
>>failures should then attempt to move the state of the system to the
>>state intended by the user. That however doesn't seem to reflect the
>>current state of the management server.
>>
>> I may be completely wrong... Can you give an example in proposal B of
>>how a transaction would span multiple modules of code ?
>>
>>   Pedro.
>>
>> On Oct 9, 2013, at 1:44 AM, Darren Shepherd wrote:
>>
>>> Okay, please read this all, this is important...  I want you all to
>>> know that its personally important to me to attempt to get rid of ACS
>>> custom stuff and introduce patterns, frameworks, libraries, etc that I
>>> feel are more consistent with modern Java development and are
>>> understood by a wider audience.  This is one of the basic reasons I
>>> started the spring-modularization branch.  I just want to be able to
>>> leverage Spring in a sane way.  The current implementation in ACS is
>>> backwards and broken and abuses Spring to the point that leveraging
>>> Spring isn't really all that possible.
>>>
>>> So while I did the Spring work, I also started laying the ground work
>>> to get rid of the ACS custom transaction management.  The custom DAO
>>> framework and the corresponding transaction management has been a huge
>>> barrier to me extending ACS in the past.  When you look at how you are
>>> supposed to access the database, it's all very custom and what I feel
>>> isn't really all that straight forward.  I was debugging an issue
>>> today and figured out there is a huge bug in what I've done and that
>>> has lead me down this rabbit hole of what the correct solution is.
>>> Additionally ACS custom transaction mgmt is done in a way that
>>> basically breaks Spring too.
>>>
>>> At some point on the mailing list there was a small discussion about
>>> removing the @DB interceptor.  The @DB interceptor does txn.open() and
>>> txn.close() around a method.  If a method forgets to commit or
>>> rollback the txn, txn.close() will rollback the transaction for the
>>> method.  So the general idea of the change was to instead move that
>>> logic to the bottom of the call stack.  The assumption being that the
>>> @DB code was just an additional check to ensure the programmer didn't
>>> forget something and we could instead just do that once at the bottom
>>> of the stack.  Oh how wrong I was.
>>>
>>> The problem is that developers have relied on the @DB interceptor to
>>> handle rollback for them.  So you see the following code quite a bit
>>>
>>> txn.start()
>>> ...
>>> txn.commit()
>>>
>>> And there is no sign of a rollback anywhere.  So the rollback will
>>> happen if some exception is thrown.  By moving the @DB logic to the
>>> bottom of stack what happens is the transaction is not rolled back
>>> when the developer thought it would and madness ensues.  So that
>>> change was bad.  So what to do....  Here's my totally bias description
>>> of solutions:
>>>
>>> Option A or "Custom Forever!":  Go back to custom ACS AOP and the @DB.
>>> This is what one would think is the simplest and safest solution.
>>> We'll it ain't really.  Here's the killer problem, besides that fact
>>> that it makes me feel very sad inside, the current rollback behavior
>>> is broken in certain spots in ACS.  While investigating possible
>>> solutions I started looking at all the places that do programmatic txn
>>> management.  It's important to realize that the txn framework only
>>> works properly if the method in which you do txn.start() has @DB on
>>> it.  There is a java assert in currentTxn() that attempts to make sure
>>> that @DB is there.  But.... nobody runs with asserts on.  So there are
>>> places in ACS where transactions are started and no @DB is there, but
>>> it happens to work because some method in the stack has @DB.  So to
>>> properly go back to option A we really need to fix all places that
>>> don't have @DB, plus make sure people always run with asserts on.  And
>>> then give up making the ACS world a better place and just do things
>>> how we always have...
>>>
>>> Option B or "Progress is Good":  The current transaction management
>>> approach (especially rollback) doesn't match how the majority of
>>> frameworks out there work.  This option is to change the Transaction
>>> class API to be more consistent with standard frameworks out there.  I
>>> propose the following APIs (if you know Spring TX mgmt, this will look
>>> familiar)
>>>
>>> 1) remove start(), commit(), rollback() - The easiest way to ensure we
>>> up date everything properly is to break the API and fix everything
>>> that is broken (about 433 places)
>>> 2) Add execute(TransactionCallback) where TransactionCallback has one
>>> method doInTransaction().  For somebody to run a transaction you would
>>> need to do
>>>
>>> txn.execute(new TransactionCallback() {
>>> Object doInTransaction() {
>>>  // do stuff
>>> }
>>> })
>>> 3) add "Object startTransaction()," commit(Object), and
>>> rollback(Object) - These methods are for callers who really really
>>> want to do thing programmatically.  To run a transaction you would do
>>>
>>> Object status = txn.startTransaction()
>>> try {
>>>  //.. do stuff
>>>  txn.commit(status)
>>> } catch (Exception e) {
>>>  txn.rollback(status)
>>> }
>>>
>>> I'm perfectly willing to go and change all the code for this.  It will
>>> just take a couple hours or so.  Option B is purposely almost exactly
>>> like Spring PlatformTransactionManager.  The reason being if we switch
>>> all the code to this style, we can later drop the implementation of
>>> Transaction and move to 100% fully Spring TX managed.
>>>
>>> Just as a final point, every custom approach or framework we have adds
>>> a barrier to people extending ACS and additionally puts more burden on
>>> the ACS community as that is more code we have to support.  If
>>> somebody today wants to know how DB transaction propagation works,
>>> there's zero documentation on it and probably 2 people who know how it
>>> works.  If we use a standard framework then somebody can just refer to
>>> that frameworks documentation, community, or stackexchange.
>>>
>>> So either option we can do and I'm opening this up to the community to
>>> decide.  If we go with Option A, a small portion of me will die
>>> inside, but so be it.
>>>
>>> Darren
>>

Re: [DISCUSS] Transaction Hell

[Darren Shepherd <da...@gmail.com>]

Pedro,

>From a high level I think we'd probably agree.  Generally I feel an
IaaS platform is largely a metadata management framework that stores
the "desired" state of the infrastructure and then pro-actively tries
to reconcile the desired state with reality.  So failures should be
recovered from easily as inconsistency will be discovered and
reconciled.  Having sad that, ACS is not at all like that.  It is very
task oriented.  Hopefully I/we/everyone can change that, its a huge
concern of mine.  The general approach in ACS I see is do task X and
hopefully it works.  If it doesn't work, well hopefully we didn't
leave things in an inconsistent state.  If we find it does leave
things in an inconsistent state, write a cleanup thread to fix bad
things in bad states....

Regarding TX specifically.  This is a huge topic.  I really don't know
where to start.  I have so many complaints with the data access in
ACS.  There's what I'd like to see, but its so far from what it really
is.  Instead I'll address specifically your question.

I wish we were doing transaction per API, but I don't think that was
ever a consideration.  I do think the sync portion of API commands
should be wrapped in a single transaction.  I really think the
original intention of the Transaction framework was to assist in
cleaning up resources that people always forget to close.  I think
that is mostly it.

The general guidelines of how I'd like transactions to work would be

1) Synchronous portions of API commands are wrapped in a single
transaction.  Transaction propagation capability from spring tx can
then handle nesting transaction as more complicated transaction
management may be need in certain places.

2) Async jobs that run in a background threads should do small fine
grained transaction management.  Ideally no transactions.
Transactions should not be used as a locking mechanism.

Having said that, there are currently so many technical issues in
getting to that.  For example, with point 1, because IPC/MessageBus
and EventBus were added recently, that makes it difficult to do 1.
The problem is that you can't send a message while a DB tx is open
because the reciever may get the message before the commit.  So
messaging frameworks have to be written in consideration of the
transaction management.  Not saying you need to do complex XA style
transactions, there's simpler ways to do that.  So regarding points 1
and 2 I said.  That's what I'd like to see, but I know its a long road
to that.

Option B is really about introducing an API that will eventually serve
as a lightweight wrapper around Spring TX.  In the short term, if I do
option B, the implementation of the code will still be the custom ACS
TX mgmt.  So across modules, its sorta kinda works but not really.
But if I do the second step of replacing custom ACS TX impl with
Spring TX, it will follow how Spring TX works.  If we have Sprint TX
we can then leverage the transaction propagation features of it to
more sanely handle transaction nesting.

I feel I went a bit the weeds with that response, but maybe something
in there made sense.

Darren

On Wed, Oct 9, 2013 at 9:31 AM, Pedro Roque Marques
<pe...@gmail.com> wrote:
> Darren,
> My assumption when I tried to make sense of the transaction code is that the underlying motivation is that the code is trying to create a transaction per API call and then allow multiple modules to implement that API call...
> i.e. the intent is do use a bit of what i would call a "web-server logic"...
>
> 1. API call starts.
> 2. Module X starts transaction...
> 3. Module Y does some other changes in the DB...
> 4. Either the API call completes successfully or not... commit or error back to the user.
>
>  I suspect that this was probably the starting point... but it doesn't really work as i describe above. Often when the plugin i'm working on screws up (or XenServer is misconfigured) one ends up with DB objects in inconsistent state.
>
> I suspect that the DB Transaction design needs to include what is the methodology for the design of the management server.
>
> In an ideal world, i would say that API calls just check authorization and quotas and should store the intent of the management server to reach the desired state. State machines that can then deal with transient failures should then attempt to move the state of the system to the state intended by the user. That however doesn't seem to reflect the current state of the management server.
>
> I may be completely wrong... Can you give an example in proposal B of how a transaction would span multiple modules of code ?
>
>   Pedro.
>
> On Oct 9, 2013, at 1:44 AM, Darren Shepherd wrote:
>
>> Okay, please read this all, this is important...  I want you all to
>> know that its personally important to me to attempt to get rid of ACS
>> custom stuff and introduce patterns, frameworks, libraries, etc that I
>> feel are more consistent with modern Java development and are
>> understood by a wider audience.  This is one of the basic reasons I
>> started the spring-modularization branch.  I just want to be able to
>> leverage Spring in a sane way.  The current implementation in ACS is
>> backwards and broken and abuses Spring to the point that leveraging
>> Spring isn't really all that possible.
>>
>> So while I did the Spring work, I also started laying the ground work
>> to get rid of the ACS custom transaction management.  The custom DAO
>> framework and the corresponding transaction management has been a huge
>> barrier to me extending ACS in the past.  When you look at how you are
>> supposed to access the database, it's all very custom and what I feel
>> isn't really all that straight forward.  I was debugging an issue
>> today and figured out there is a huge bug in what I've done and that
>> has lead me down this rabbit hole of what the correct solution is.
>> Additionally ACS custom transaction mgmt is done in a way that
>> basically breaks Spring too.
>>
>> At some point on the mailing list there was a small discussion about
>> removing the @DB interceptor.  The @DB interceptor does txn.open() and
>> txn.close() around a method.  If a method forgets to commit or
>> rollback the txn, txn.close() will rollback the transaction for the
>> method.  So the general idea of the change was to instead move that
>> logic to the bottom of the call stack.  The assumption being that the
>> @DB code was just an additional check to ensure the programmer didn't
>> forget something and we could instead just do that once at the bottom
>> of the stack.  Oh how wrong I was.
>>
>> The problem is that developers have relied on the @DB interceptor to
>> handle rollback for them.  So you see the following code quite a bit
>>
>> txn.start()
>> ...
>> txn.commit()
>>
>> And there is no sign of a rollback anywhere.  So the rollback will
>> happen if some exception is thrown.  By moving the @DB logic to the
>> bottom of stack what happens is the transaction is not rolled back
>> when the developer thought it would and madness ensues.  So that
>> change was bad.  So what to do....  Here's my totally bias description
>> of solutions:
>>
>> Option A or "Custom Forever!":  Go back to custom ACS AOP and the @DB.
>> This is what one would think is the simplest and safest solution.
>> We'll it ain't really.  Here's the killer problem, besides that fact
>> that it makes me feel very sad inside, the current rollback behavior
>> is broken in certain spots in ACS.  While investigating possible
>> solutions I started looking at all the places that do programmatic txn
>> management.  It's important to realize that the txn framework only
>> works properly if the method in which you do txn.start() has @DB on
>> it.  There is a java assert in currentTxn() that attempts to make sure
>> that @DB is there.  But.... nobody runs with asserts on.  So there are
>> places in ACS where transactions are started and no @DB is there, but
>> it happens to work because some method in the stack has @DB.  So to
>> properly go back to option A we really need to fix all places that
>> don't have @DB, plus make sure people always run with asserts on.  And
>> then give up making the ACS world a better place and just do things
>> how we always have...
>>
>> Option B or "Progress is Good":  The current transaction management
>> approach (especially rollback) doesn't match how the majority of
>> frameworks out there work.  This option is to change the Transaction
>> class API to be more consistent with standard frameworks out there.  I
>> propose the following APIs (if you know Spring TX mgmt, this will look
>> familiar)
>>
>> 1) remove start(), commit(), rollback() - The easiest way to ensure we
>> up date everything properly is to break the API and fix everything
>> that is broken (about 433 places)
>> 2) Add execute(TransactionCallback) where TransactionCallback has one
>> method doInTransaction().  For somebody to run a transaction you would
>> need to do
>>
>> txn.execute(new TransactionCallback() {
>> Object doInTransaction() {
>>  // do stuff
>> }
>> })
>> 3) add "Object startTransaction()," commit(Object), and
>> rollback(Object) - These methods are for callers who really really
>> want to do thing programmatically.  To run a transaction you would do
>>
>> Object status = txn.startTransaction()
>> try {
>>  //.. do stuff
>>  txn.commit(status)
>> } catch (Exception e) {
>>  txn.rollback(status)
>> }
>>
>> I'm perfectly willing to go and change all the code for this.  It will
>> just take a couple hours or so.  Option B is purposely almost exactly
>> like Spring PlatformTransactionManager.  The reason being if we switch
>> all the code to this style, we can later drop the implementation of
>> Transaction and move to 100% fully Spring TX managed.
>>
>> Just as a final point, every custom approach or framework we have adds
>> a barrier to people extending ACS and additionally puts more burden on
>> the ACS community as that is more code we have to support.  If
>> somebody today wants to know how DB transaction propagation works,
>> there's zero documentation on it and probably 2 people who know how it
>> works.  If we use a standard framework then somebody can just refer to
>> that frameworks documentation, community, or stackexchange.
>>
>> So either option we can do and I'm opening this up to the community to
>> decide.  If we go with Option A, a small portion of me will die
>> inside, but so be it.
>>
>> Darren
>

Re: [DISCUSS] Transaction Hell

[Pedro Roque Marques <pe...@gmail.com>]

Darren,
My assumption when I tried to make sense of the transaction code is that the underlying motivation is that the code is trying to create a transaction per API call and then allow multiple modules to implement that API call...
i.e. the intent is do use a bit of what i would call a "web-server logic"...

1. API call starts.
2. Module X starts transaction...
3. Module Y does some other changes in the DB...
4. Either the API call completes successfully or not... commit or error back to the user.

 I suspect that this was probably the starting point... but it doesn't really work as i describe above. Often when the plugin i'm working on screws up (or XenServer is misconfigured) one ends up with DB objects in inconsistent state.

I suspect that the DB Transaction design needs to include what is the methodology for the design of the management server.

In an ideal world, i would say that API calls just check authorization and quotas and should store the intent of the management server to reach the desired state. State machines that can then deal with transient failures should then attempt to move the state of the system to the state intended by the user. That however doesn't seem to reflect the current state of the management server.

I may be completely wrong... Can you give an example in proposal B of how a transaction would span multiple modules of code ?

  Pedro.

On Oct 9, 2013, at 1:44 AM, Darren Shepherd wrote:

> Okay, please read this all, this is important...  I want you all to
> know that its personally important to me to attempt to get rid of ACS
> custom stuff and introduce patterns, frameworks, libraries, etc that I
> feel are more consistent with modern Java development and are
> understood by a wider audience.  This is one of the basic reasons I
> started the spring-modularization branch.  I just want to be able to
> leverage Spring in a sane way.  The current implementation in ACS is
> backwards and broken and abuses Spring to the point that leveraging
> Spring isn't really all that possible.
> 
> So while I did the Spring work, I also started laying the ground work
> to get rid of the ACS custom transaction management.  The custom DAO
> framework and the corresponding transaction management has been a huge
> barrier to me extending ACS in the past.  When you look at how you are
> supposed to access the database, it's all very custom and what I feel
> isn't really all that straight forward.  I was debugging an issue
> today and figured out there is a huge bug in what I've done and that
> has lead me down this rabbit hole of what the correct solution is.
> Additionally ACS custom transaction mgmt is done in a way that
> basically breaks Spring too.
> 
> At some point on the mailing list there was a small discussion about
> removing the @DB interceptor.  The @DB interceptor does txn.open() and
> txn.close() around a method.  If a method forgets to commit or
> rollback the txn, txn.close() will rollback the transaction for the
> method.  So the general idea of the change was to instead move that
> logic to the bottom of the call stack.  The assumption being that the
> @DB code was just an additional check to ensure the programmer didn't
> forget something and we could instead just do that once at the bottom
> of the stack.  Oh how wrong I was.
> 
> The problem is that developers have relied on the @DB interceptor to
> handle rollback for them.  So you see the following code quite a bit
> 
> txn.start()
> ...
> txn.commit()
> 
> And there is no sign of a rollback anywhere.  So the rollback will
> happen if some exception is thrown.  By moving the @DB logic to the
> bottom of stack what happens is the transaction is not rolled back
> when the developer thought it would and madness ensues.  So that
> change was bad.  So what to do....  Here's my totally bias description
> of solutions:
> 
> Option A or "Custom Forever!":  Go back to custom ACS AOP and the @DB.
> This is what one would think is the simplest and safest solution.
> We'll it ain't really.  Here's the killer problem, besides that fact
> that it makes me feel very sad inside, the current rollback behavior
> is broken in certain spots in ACS.  While investigating possible
> solutions I started looking at all the places that do programmatic txn
> management.  It's important to realize that the txn framework only
> works properly if the method in which you do txn.start() has @DB on
> it.  There is a java assert in currentTxn() that attempts to make sure
> that @DB is there.  But.... nobody runs with asserts on.  So there are
> places in ACS where transactions are started and no @DB is there, but
> it happens to work because some method in the stack has @DB.  So to
> properly go back to option A we really need to fix all places that
> don't have @DB, plus make sure people always run with asserts on.  And
> then give up making the ACS world a better place and just do things
> how we always have...
> 
> Option B or "Progress is Good":  The current transaction management
> approach (especially rollback) doesn't match how the majority of
> frameworks out there work.  This option is to change the Transaction
> class API to be more consistent with standard frameworks out there.  I
> propose the following APIs (if you know Spring TX mgmt, this will look
> familiar)
> 
> 1) remove start(), commit(), rollback() - The easiest way to ensure we
> up date everything properly is to break the API and fix everything
> that is broken (about 433 places)
> 2) Add execute(TransactionCallback) where TransactionCallback has one
> method doInTransaction().  For somebody to run a transaction you would
> need to do
> 
> txn.execute(new TransactionCallback() {
> Object doInTransaction() {
>  // do stuff
> }
> })
> 3) add "Object startTransaction()," commit(Object), and
> rollback(Object) - These methods are for callers who really really
> want to do thing programmatically.  To run a transaction you would do
> 
> Object status = txn.startTransaction()
> try {
>  //.. do stuff
>  txn.commit(status)
> } catch (Exception e) {
>  txn.rollback(status)
> }
> 
> I'm perfectly willing to go and change all the code for this.  It will
> just take a couple hours or so.  Option B is purposely almost exactly
> like Spring PlatformTransactionManager.  The reason being if we switch
> all the code to this style, we can later drop the implementation of
> Transaction and move to 100% fully Spring TX managed.
> 
> Just as a final point, every custom approach or framework we have adds
> a barrier to people extending ACS and additionally puts more burden on
> the ACS community as that is more code we have to support.  If
> somebody today wants to know how DB transaction propagation works,
> there's zero documentation on it and probably 2 people who know how it
> works.  If we use a standard framework then somebody can just refer to
> that frameworks documentation, community, or stackexchange.
> 
> So either option we can do and I'm opening this up to the community to
> decide.  If we go with Option A, a small portion of me will die
> inside, but so be it.
> 
> Darren

Re: [DISCUSS] Transaction Hell

[Darren Shepherd <da...@gmail.com>]

Some random responses.

Spring is good at two things.  The core IoC container and TX mgmt.
The way I use Spring for IoC is I always ensure there is no dependency
on Spring itself.  This means I don't really care too much about the
Spring core IoC container and would be open to using a different
container, but just for practical reasons its safer to stick with
Spring because of the huge user base and excellent quality.

Even if you don't use Spring IoC, you can still use Spring TX.  I have
never found a better TX mgmt framework than Spring.  It is extremely
comprehensive.  So if your not using container managed transactions
(JEE and EJB nonsense), then I really think Spring TX is the only
option.

Spring MVC, Spring Batch, Spring WS, Spring [Whatever], are mostly
nonsense in my mind and I don't care to use them.  I don't subscribe
to the whole Spring world.  Just IoC (because IoC truly revolutionized
Java) and TX.

Regarding asserts.  I don't care for Java asserts.  Computers are fast
and extra null checks here and there are fine to just have in the
code.  It is not like we are writing a stock trading platform.  I
would prefer we never use asserts.  I hate to find bugs that only
happen when asserts are on or off.  If your not aware, Java asserts
are turned on with a runtime option "-ea."  They aren't a build time
thing like you see in C macro based ASSERTS.

Regarding what standard framework we should use.  At the end of the
day I'm trying to move towards Spring TX.  The best way to use Spring
TX is to use declarative transaction management.  The existing ACS
code base uses a lot of programmatic tx mgmt.  Well, in reality its
100% programmatic.  What I mean by programmatic is that in the code
you do tx.start(), tx.commit().  With declarative tx mgmt you use AOP
to isolate portions of code (like dao methods) to
start/commit/rollback the TX.  I don't think we will move away from
programmatic tx mgmt anytime soon, so if we move to Spring I don't
want us to suddenly start using Spring APIs everywhere.  Instead I
will propose we stay with a light wrapper, which will be the API I
already proposed in Option B.  It won't be 100% Spring, as we still
have a custom light wrapper, but at least at that point, anybody who
knows spring TX (or reads the docs) will understand what is going on.
Also protects us in the future if some hot new framework comes along
we should ideally be able to switch to it.

(Random thing, I've noticed in the code instances where the code is
doing txn.start() and then multiple txn.commit().  The transaction
framework AFAIK wasn't built for this.  The first commit will commit
or not depending on if the tx was nested.  The second commit will
either not commit or will destroy the internal state stack of the
transaction class as it will commit the parent tx.  So, for example, I
don't think AlertDaoImpl.archiveAlert() does at all what the
programmer who wrote it thought.  It's stuff like this that makes me
think we just need to bite the bullet, do the hard work, and clean
this up.)

Darren

Re: [DISCUSS] Transaction Hell

[Daan Hoogland <da...@gmail.com>]

Darren,

Thank you for the great investigative work. As for the assert, I think
that if no one else at least Jenkins should run with asserts on, so we
can see if a commit breaks something.

As for the framework, I think your proposed solution sound good. I am
not a big fan of spring. It does some things very good but it tries to
do to much. If you say it is a good framework for transaction
management I have to trust you as I didn't use that for more then five
years. I would certainly not let you in the pit as sole owner of the
change though as I have been in a short struggle with what you are
trying to solve as well. I would like to hear from the originators of
the present transaction management though.

regards,
Daan

On Wed, Oct 9, 2013 at 11:38 AM, Frankie Onuonga <fr...@angani.co> wrote:
> Hi Darren,
> Greetings from Nairobi.
>
> First, let me start by saying that you for the email .
> I personally think the options were explained very well.
>
> Now on a serious note, I think that it would be good practice to ensure that we go with a standardized method of doing things.
> I therefore support the option to go with a framework.
> It should make management of code easier.
> Future development should also be much better.
>
> Kind Regards,
>
>
> Sent from my Windows Phone
> ________________________________
> From: Darren Shepherd<ma...@gmail.com>
> Sent: 10/9/2013 11:46 AM
> To: dev@cloudstack.apache.org<ma...@cloudstack.apache.org>
> Subject: [DISCUSS] Transaction Hell
>
> Okay, please read this all, this is important...  I want you all to
> know that its personally important to me to attempt to get rid of ACS
> custom stuff and introduce patterns, frameworks, libraries, etc that I
> feel are more consistent with modern Java development and are
> understood by a wider audience.  This is one of the basic reasons I
> started the spring-modularization branch.  I just want to be able to
> leverage Spring in a sane way.  The current implementation in ACS is
> backwards and broken and abuses Spring to the point that leveraging
> Spring isn't really all that possible.
>
> So while I did the Spring work, I also started laying the ground work
> to get rid of the ACS custom transaction management.  The custom DAO
> framework and the corresponding transaction management has been a huge
> barrier to me extending ACS in the past.  When you look at how you are
> supposed to access the database, it's all very custom and what I feel
> isn't really all that straight forward.  I was debugging an issue
> today and figured out there is a huge bug in what I've done and that
> has lead me down this rabbit hole of what the correct solution is.
> Additionally ACS custom transaction mgmt is done in a way that
> basically breaks Spring too.
>
> At some point on the mailing list there was a small discussion about
> removing the @DB interceptor.  The @DB interceptor does txn.open() and
> txn.close() around a method.  If a method forgets to commit or
> rollback the txn, txn.close() will rollback the transaction for the
> method.  So the general idea of the change was to instead move that
> logic to the bottom of the call stack.  The assumption being that the
> @DB code was just an additional check to ensure the programmer didn't
> forget something and we could instead just do that once at the bottom
> of the stack.  Oh how wrong I was.
>
> The problem is that developers have relied on the @DB interceptor to
> handle rollback for them.  So you see the following code quite a bit
>
> txn.start()
> ...
> txn.commit()
>
> And there is no sign of a rollback anywhere.  So the rollback will
> happen if some exception is thrown.  By moving the @DB logic to the
> bottom of stack what happens is the transaction is not rolled back
> when the developer thought it would and madness ensues.  So that
> change was bad.  So what to do....  Here's my totally bias description
> of solutions:
>
> Option A or "Custom Forever!":  Go back to custom ACS AOP and the @DB.
>  This is what one would think is the simplest and safest solution.
> We'll it ain't really.  Here's the killer problem, besides that fact
> that it makes me feel very sad inside, the current rollback behavior
> is broken in certain spots in ACS.  While investigating possible
> solutions I started looking at all the places that do programmatic txn
> management.  It's important to realize that the txn framework only
> works properly if the method in which you do txn.start() has @DB on
> it.  There is a java assert in currentTxn() that attempts to make sure
> that @DB is there.  But.... nobody runs with asserts on.  So there are
> places in ACS where transactions are started and no @DB is there, but
> it happens to work because some method in the stack has @DB.  So to
> properly go back to option A we really need to fix all places that
> don't have @DB, plus make sure people always run with asserts on.  And
> then give up making the ACS world a better place and just do things
> how we always have...
>
> Option B or "Progress is Good":  The current transaction management
> approach (especially rollback) doesn't match how the majority of
> frameworks out there work.  This option is to change the Transaction
> class API to be more consistent with standard frameworks out there.  I
> propose the following APIs (if you know Spring TX mgmt, this will look
> familiar)
>
> 1) remove start(), commit(), rollback() - The easiest way to ensure we
> up date everything properly is to break the API and fix everything
> that is broken (about 433 places)
> 2) Add execute(TransactionCallback) where TransactionCallback has one
> method doInTransaction().  For somebody to run a transaction you would
> need to do
>
> txn.execute(new TransactionCallback() {
> Object doInTransaction() {
>   // do stuff
> }
> })
> 3) add "Object startTransaction()," commit(Object), and
> rollback(Object) - These methods are for callers who really really
> want to do thing programmatically.  To run a transaction you would do
>
> Object status = txn.startTransaction()
> try {
>   //.. do stuff
>   txn.commit(status)
> } catch (Exception e) {
>   txn.rollback(status)
> }
>
> I'm perfectly willing to go and change all the code for this.  It will
> just take a couple hours or so.  Option B is purposely almost exactly
> like Spring PlatformTransactionManager.  The reason being if we switch
> all the code to this style, we can later drop the implementation of
> Transaction and move to 100% fully Spring TX managed.
>
> Just as a final point, every custom approach or framework we have adds
> a barrier to people extending ACS and additionally puts more burden on
> the ACS community as that is more code we have to support.  If
> somebody today wants to know how DB transaction propagation works,
> there's zero documentation on it and probably 2 people who know how it
> works.  If we use a standard framework then somebody can just refer to
> that frameworks documentation, community, or stackexchange.
>
> So either option we can do and I'm opening this up to the community to
> decide.  If we go with Option A, a small portion of me will die
> inside, but so be it.
>
> Darren

RE: [DISCUSS] Transaction Hell

[Frankie Onuonga <fr...@angani.co>]

Hi Darren,
Greetings from Nairobi.

First, let me start by saying that you for the email .
I personally think the options were explained very well.

Now on a serious note, I think that it would be good practice to ensure that we go with a standardized method of doing things.
I therefore support the option to go with a framework.
It should make management of code easier.
Future development should also be much better.

Kind Regards,


Sent from my Windows Phone
________________________________
From: Darren Shepherd<ma...@gmail.com>
Sent: ‎10/‎9/‎2013 11:46 AM
To: dev@cloudstack.apache.org<ma...@cloudstack.apache.org>
Subject: [DISCUSS] Transaction Hell

Okay, please read this all, this is important...  I want you all to
know that its personally important to me to attempt to get rid of ACS
custom stuff and introduce patterns, frameworks, libraries, etc that I
feel are more consistent with modern Java development and are
understood by a wider audience.  This is one of the basic reasons I
started the spring-modularization branch.  I just want to be able to
leverage Spring in a sane way.  The current implementation in ACS is
backwards and broken and abuses Spring to the point that leveraging
Spring isn't really all that possible.

So while I did the Spring work, I also started laying the ground work
to get rid of the ACS custom transaction management.  The custom DAO
framework and the corresponding transaction management has been a huge
barrier to me extending ACS in the past.  When you look at how you are
supposed to access the database, it's all very custom and what I feel
isn't really all that straight forward.  I was debugging an issue
today and figured out there is a huge bug in what I've done and that
has lead me down this rabbit hole of what the correct solution is.
Additionally ACS custom transaction mgmt is done in a way that
basically breaks Spring too.

At some point on the mailing list there was a small discussion about
removing the @DB interceptor.  The @DB interceptor does txn.open() and
txn.close() around a method.  If a method forgets to commit or
rollback the txn, txn.close() will rollback the transaction for the
method.  So the general idea of the change was to instead move that
logic to the bottom of the call stack.  The assumption being that the
@DB code was just an additional check to ensure the programmer didn't
forget something and we could instead just do that once at the bottom
of the stack.  Oh how wrong I was.

The problem is that developers have relied on the @DB interceptor to
handle rollback for them.  So you see the following code quite a bit

txn.start()
...
txn.commit()

And there is no sign of a rollback anywhere.  So the rollback will
happen if some exception is thrown.  By moving the @DB logic to the
bottom of stack what happens is the transaction is not rolled back
when the developer thought it would and madness ensues.  So that
change was bad.  So what to do....  Here's my totally bias description
of solutions:

Option A or "Custom Forever!":  Go back to custom ACS AOP and the @DB.
 This is what one would think is the simplest and safest solution.
We'll it ain't really.  Here's the killer problem, besides that fact
that it makes me feel very sad inside, the current rollback behavior
is broken in certain spots in ACS.  While investigating possible
solutions I started looking at all the places that do programmatic txn
management.  It's important to realize that the txn framework only
works properly if the method in which you do txn.start() has @DB on
it.  There is a java assert in currentTxn() that attempts to make sure
that @DB is there.  But.... nobody runs with asserts on.  So there are
places in ACS where transactions are started and no @DB is there, but
it happens to work because some method in the stack has @DB.  So to
properly go back to option A we really need to fix all places that
don't have @DB, plus make sure people always run with asserts on.  And
then give up making the ACS world a better place and just do things
how we always have...

Option B or "Progress is Good":  The current transaction management
approach (especially rollback) doesn't match how the majority of
frameworks out there work.  This option is to change the Transaction
class API to be more consistent with standard frameworks out there.  I
propose the following APIs (if you know Spring TX mgmt, this will look
familiar)

1) remove start(), commit(), rollback() - The easiest way to ensure we
up date everything properly is to break the API and fix everything
that is broken (about 433 places)
2) Add execute(TransactionCallback) where TransactionCallback has one
method doInTransaction().  For somebody to run a transaction you would
need to do

txn.execute(new TransactionCallback() {
Object doInTransaction() {
  // do stuff
}
})
3) add "Object startTransaction()," commit(Object), and
rollback(Object) - These methods are for callers who really really
want to do thing programmatically.  To run a transaction you would do

Object status = txn.startTransaction()
try {
  //.. do stuff
  txn.commit(status)
} catch (Exception e) {
  txn.rollback(status)
}

I'm perfectly willing to go and change all the code for this.  It will
just take a couple hours or so.  Option B is purposely almost exactly
like Spring PlatformTransactionManager.  The reason being if we switch
all the code to this style, we can later drop the implementation of
Transaction and move to 100% fully Spring TX managed.

Just as a final point, every custom approach or framework we have adds
a barrier to people extending ACS and additionally puts more burden on
the ACS community as that is more code we have to support.  If
somebody today wants to know how DB transaction propagation works,
there's zero documentation on it and probably 2 people who know how it
works.  If we use a standard framework then somebody can just refer to
that frameworks documentation, community, or stackexchange.

So either option we can do and I'm opening this up to the community to
decide.  If we go with Option A, a small portion of me will die
inside, but so be it.

Darren

RE: [DISCUSS] Transaction Hell

[Donal Lafferty <do...@citrix.com>]

Hi Darren,

Thank you for explain this issue in huge detail.

Could I respond with some questions?

1. If @DB declarations are not detected, because Java 'assert' is not turned on, can we use a different mechanism to check for @DB?

2. What standard framework do you recommend using for DB transactions?  Is there a guide available?

3. Going back to 'assert'.  Since they're not being used, should we get rid of all instances of Java 'assert' in the code base?  Or should we turn on 'assert' in non-release builds?


DL


> -----Original Message-----
> From: Darren Shepherd [mailto:darren.s.shepherd@gmail.com]
> Sent: 09 October 2013 09:45
> To: dev@cloudstack.apache.org
> Subject: [DISCUSS] Transaction Hell
> 
> Okay, please read this all, this is important...  I want you all to know that its
> personally important to me to attempt to get rid of ACS custom stuff and
> introduce patterns, frameworks, libraries, etc that I feel are more consistent
> with modern Java development and are understood by a wider audience.
> This is one of the basic reasons I started the spring-modularization branch.  I
> just want to be able to leverage Spring in a sane way.  The current
> implementation in ACS is backwards and broken and abuses Spring to the
> point that leveraging Spring isn't really all that possible.
> 
> So while I did the Spring work, I also started laying the ground work to get rid
> of the ACS custom transaction management.  The custom DAO framework
> and the corresponding transaction management has been a huge barrier to
> me extending ACS in the past.  When you look at how you are supposed to
> access the database, it's all very custom and what I feel isn't really all that
> straight forward.  I was debugging an issue today and figured out there is a
> huge bug in what I've done and that has lead me down this rabbit hole of
> what the correct solution is.
> Additionally ACS custom transaction mgmt is done in a way that basically
> breaks Spring too.
> 
> At some point on the mailing list there was a small discussion about removing
> the @DB interceptor.  The @DB interceptor does txn.open() and
> txn.close() around a method.  If a method forgets to commit or rollback the
> txn, txn.close() will rollback the transaction for the method.  So the general
> idea of the change was to instead move that logic to the bottom of the call
> stack.  The assumption being that the @DB code was just an additional check
> to ensure the programmer didn't forget something and we could instead just
> do that once at the bottom of the stack.  Oh how wrong I was.
> 
> The problem is that developers have relied on the @DB interceptor to handle
> rollback for them.  So you see the following code quite a bit
> 
> txn.start()
> ...
> txn.commit()
> 
> And there is no sign of a rollback anywhere.  So the rollback will happen if
> some exception is thrown.  By moving the @DB logic to the bottom of stack
> what happens is the transaction is not rolled back when the developer
> thought it would and madness ensues.  So that change was bad.  So what to
> do....  Here's my totally bias description of solutions:
> 
> Option A or "Custom Forever!":  Go back to custom ACS AOP and the @DB.
>  This is what one would think is the simplest and safest solution.
> We'll it ain't really.  Here's the killer problem, besides that fact that it makes
> me feel very sad inside, the current rollback behavior is broken in certain
> spots in ACS.  While investigating possible solutions I started looking at all the
> places that do programmatic txn management.  It's important to realize that
> the txn framework only works properly if the method in which you do
> txn.start() has @DB on it.  There is a java assert in currentTxn() that attempts
> to make sure that @DB is there.  But.... nobody runs with asserts on.  So
> there are places in ACS where transactions are started and no @DB is there,
> but it happens to work because some method in the stack has @DB.  So to
> properly go back to option A we really need to fix all places that don't have
> @DB, plus make sure people always run with asserts on.  And then give up
> making the ACS world a better place and just do things how we always
> have...
> 
> Option B or "Progress is Good":  The current transaction management
> approach (especially rollback) doesn't match how the majority of frameworks
> out there work.  This option is to change the Transaction class API to be more
> consistent with standard frameworks out there.  I propose the following APIs
> (if you know Spring TX mgmt, this will look
> familiar)
> 
> 1) remove start(), commit(), rollback() - The easiest way to ensure we up
> date everything properly is to break the API and fix everything that is broken
> (about 433 places)
> 2) Add execute(TransactionCallback) where TransactionCallback has one
> method doInTransaction().  For somebody to run a transaction you would
> need to do
> 
> txn.execute(new TransactionCallback() {
> Object doInTransaction() {
>   // do stuff
> }
> })
> 3) add "Object startTransaction()," commit(Object), and
> rollback(Object) - These methods are for callers who really really want to do
> thing programmatically.  To run a transaction you would do
> 
> Object status = txn.startTransaction()
> try {
>   //.. do stuff
>   txn.commit(status)
> } catch (Exception e) {
>   txn.rollback(status)
> }
> 
> I'm perfectly willing to go and change all the code for this.  It will just take a
> couple hours or so.  Option B is purposely almost exactly like Spring
> PlatformTransactionManager.  The reason being if we switch all the code to
> this style, we can later drop the implementation of Transaction and move to
> 100% fully Spring TX managed.
> 
> Just as a final point, every custom approach or framework we have adds a
> barrier to people extending ACS and additionally puts more burden on the
> ACS community as that is more code we have to support.  If somebody today
> wants to know how DB transaction propagation works, there's zero
> documentation on it and probably 2 people who know how it works.  If we
> use a standard framework then somebody can just refer to that frameworks
> documentation, community, or stackexchange.
> 
> So either option we can do and I'm opening this up to the community to
> decide.  If we go with Option A, a small portion of me will die inside, but so be
> it.
> 
> Darren