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.
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
