Proposed enhancement: discard() on error
Mark Mandel
This is the last thing on my list of things to do before doing leaving
the code base stable for the 0.6.1 release, but I wanted to run it
past the group.
Essentially, I was thinking about error handling - and figured that I
would wrap each of the public methods that Transfer has, like save()
and new() and delete() etc in try, catch block, and if an error
occurred during the processing, I would discard() the current object,
and rethrow the error.
I figured I would do this, as it would remove some of the chance
(there is always a chance) that there is dirty data in the cache if
something happens to go wrong, thus causing further issues.
I wanted to run this past the list, as I didn't want to end up
building something that assumed it was smarter than the developer
using it.
Can anyone think of any scenarios in which this would be a bad thing?
Obviously there would be a control to ensure that a discard operation
that threw an error didn't send the system into an infinite loop.
Just wanted to get your thoughts before I did any more work.
Otherwise, assuming no one finds anything in the current code base for
the next week or so, I'll be looking at doing the 0.6.1 release
towards the end of next week, or early the week after.
Look forward to hearing your thoughts.
Mark
Sean Corfield
> I figured I would do this, as it would remove some of the chance
> (there is always a chance) that there is dirty data in the cache if
> something happens to go wrong, thus causing further issues.
My first reaction was "Yeah, great idea!" then I wondered about
clustered environments... If you get an exception, you need to discard
on all instances in the cluster since it would be dirty. This is a
problem we already ran into at Adobe, running Transfer in a cluster.
We have machinery that allows us to discard across a cluster on any
changes (using our own custom machinery) but it sounds like we'd need
to do this on any exception as well? Not something I'd considered
before.
Because of that, in many ways I'd rather you did *not* discard() dirty
data silently on an exception because if there really *is* dirty data
- and I've not seen the scenario in real-world usage yet - then it
would help debug it quicker if you trip over the dirty data locally
(as opposed to only having an out-of-sync clustered cache...
--
Sean A Corfield -- (904) 302-SEAN
An Architect's View -- http://corfield.org/
"If you're not annoying somebody, you're not really alive."
-- Margaret Atwood
Mark Mandel
Hopefully this makes sense, as I'm thinking it out as I type.
For a clustered environment, I would guess that if something went
wrong currently, (I dunno, someone changes a database field in the DB
without a Transfer definition being updated.. or something), then the
local version would be out of sync, and the rest of any clustered
versions would be fine.
With my new proposal, if something went wrong, the local version would
still be okay, as it's been discarded, and the clustered versions
would be fine, as if something goes wrong, the data is not saved.
Now that I really think about it, I can't see a case when something
went wrong, and the clustered data could end up being dirty, simply
because if something goes wrong, no data should be commited to the
database.
This isn't to push my idea, just trying to work out a case in which
you could actually cause dirty data on the cluster.
Mark
Sean Corfield
> With my new proposal, if something went wrong, the local version would
> still be okay, as it's been discarded, and the clustered versions
> would be fine, as if something goes wrong, the data is not saved.
Hmm, I'm fairly persuaded... So you're saying that if a save() fails,
this would guarantee that the local cache was clean (the object would
be removed) and the database state would be unchanged? If so, then I'd
support this change.
Mark Mandel
There is a CFC that is hooked into the onAfterDelete event (for
example), with a code error in it, like a divide by zero error.
The object goes to delete, the delete is committed to the database,
and the afterDeleteEvent fires, causing an error - which then discards
the object, but I'm figuring will also not allow you to update your
clustered environment?
A couple of thoughts on that:
1) You would hope that would get picked up in testing, but sometimes
things do not
2) I could wrap the try/catch blocks to not do work on 'after' events,
thus ensuring that they won't discard() valid data (which is actually
a good idea).... in fact, the more i think about it, I may just avoid
the try/catch around event actions at all.
So that leaves me still leaning towards the enhancement, but a more
finely tuned one.
Mark
On 1/2/07, Sean Corfield <seanco...@gmail.com> wrote:
>
Sean Corfield
> There is a CFC that is hooked into the onAfterDelete event (for
> example), with a code error in it, like a divide by zero error.
Ooh, an interesting use case...
> The object goes to delete, the delete is committed to the database,
> and the afterDeleteEvent fires, causing an error - which then discards
> the object, but I'm figuring will also not allow you to update your
> clustered environment?
Well, our expectation would be that a *successful* delete() would need
an update to the clustered cache... interesting that there are use
cases where even an "unsuccessful" delete() would need an update to
the clustered cache... ugh!
Yeah, I think it's fair to say that this change doesn't make my world
any worse...
Mark Mandel
I think I'm going to leave this one out for now, since I think I'll
end up ripping out all the code in the long run.
For a future release, I'm thinking of building something that looks like;
<cfset transaction = getTransfer().getTransaction()>
<cftry>
<cfset transaction.begin()>
<cfset getTranser().save(a)>
<cfset getTranser().save(b)>
<cfset getTranser().save(c)>
<cfset transaction.commit()>
<cfcatch type="any">
<cfset transaction.rollback()>
</cfcatch>
</cftry>
This would not only manage your database transaction, but also if the
save on 'b' failing, then both 'a' and 'b' would be discarded to also
rollback the cache state.
This wouldn't be required, but would just take place over doing calls
like <getTransfer().save(a, false)> to tell Transfer to not use
internal Transactions.
Regards,
Mark
On 1/2/07, Sean Corfield <seanco...@gmail.com> wrote:
>