Mailing-List: contact dev-help@cloudstack.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@cloudstack.apache.org
Received-SPF: pass (nike.apache.org: domain of daan.hoogland@gmail.com
 designates 209.85.223.182 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CE7AE69E.5690B%kelven.yang@citrix.com>
References: <CE7AE223.2F2D9%chiradeep.vittal@citrix.com>
 <CE7AE69E.5690B%kelven.yang@citrix.com>
From: Daan Hoogland <daan.hoogland@gmail.com>
Date: Wed, 9 Oct 2013 20:56:44 +0200
Message-ID: 
 <CAGQtxvas0FvVfhwCui+tR9-zDZE-Cm9NZpo-bG72+OzR3LF2VA@mail.gmail.com>
Subject: Re: [DISCUSS] Transaction Hell
To: dev <dev@cloudstack.apache.org>
Content-Type: multipart/alternative; boundary=20cf303ea4b879b9b504e8537373

--20cf303ea4b879b9b504e8537373
Content-Type: text/plain; charset=ISO-8859-1

Darren,

Happy to hear your view on Spring.

+1 for option B


On Wed, Oct 9, 2013 at 8:06 PM, Kelven Yang <kelven.yang@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" <Chiradeep.Vittal@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" <darren.s.shepherd@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
> >><pedro.r.marques@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
> >>>
> >
>
>

--20cf303ea4b879b9b504e8537373--