Anil Gangolli wrote:
I didn't get a chance to do a careful code review, and I probably won't
over the course of the next couple of days, but I did look over it.
It is much cleaner than the last version I saw. All of the
transaction-related code is now very well isolated in the
HibernatePersistenceStrategy. The manager implementation code isn't
mucking with transaction management at all. Very nice.
This looks good to me. Nice work Allen.
The only thing I'd suggest to review carefully is to make sure that the
paths get to executing the rollback in all non-success cases.
I think you want to look at the following issue in particular. It looks
like rollback is only getting called in the
HibernatePersistenceStrategy.release() method, and it looks like this is
only called after a failure during the explicit flush(). I believe this
is not sufficient. A query after some modifications to session objects
might result in what is in effect an implicit flush. This is discussed
in the Hibernate docs. What this means is that even in the absence of
an explicit flush() you want to make sure that the rollback() is called
if the transaction wasn't committed. This probably means that we need
at least a release()/rollback() call in a filter that is called whenever
the transaction hasn't been committed.
You are correct that a rollback only happens on a call to
strategy.release(), however strategy.release() is called from 2 very
important places.
1. If there is any kind of exception generated from a call to
Roller.flush() then it triggers a release.
2. The PersistenceSessionFilter calls Roller.release() at the end of
every response cycle. So this triggers an explicit rollback at the end
of every response.
Do we need more than that?
All other places in the code that happen outside the normal
request/response cycle need to call Roller.release() themselves, so like
in the scheduled tasks.
One thing that I am uncertain about though is what the dispatch level
needs to be for the PersistenceSessionFilter. We currently have it set
to REQUEST and FORWARD but I wasn't sure if that is needed. I suppose
that when a request is dispatched to a different jsp/servlet then it is
on a new thread, so it can't use the previous Hibernate Session correct?
That is kind of a bummer and I suppose it could cause some issues with
detached objects in the view as well. But that isn't any different than
what we have had all along, so I suppose it's okay.
-- Allen
In Tomcat, I think Apache DBCP's behavior will save us in the sense that
it will always do a rollback on the connection just before returning it
to the pool, but we probably won't want to rely on this behavior in
general on other server's DataSource / pool implementations.
If we address this issue, I think the approach is fine. Unfortunately I
won't be able to do a more careful review in your desired timeframe.
Someone should though.
Overall, looks very good.
--a.
----- Original Message ----- From: "Anil Gangolli" <[EMAIL PROTECTED]>
To: <[email protected]>
Sent: Monday, April 17, 2006 11:32 PM
Subject: Re: new backend ready for trunk?
I'd like a little bit more time to understand the changes; I don't
anticipate any major objections.
--a.
----- Original Message ----- From: "Allen Gilliland"
<[EMAIL PROTECTED]>
To: <[email protected]>
Sent: Monday, April 17, 2006 3:45 PM
Subject: new backend ready for trunk?
i'd like to move forward with merging the new backend changes back
into the trunk sometime in the next couple days, so anyone who's
interested enough, can you take a look pretty soon and holler if you
see anything you think looks wrong or broken.
-- Allen
