I just did a quick review of the weblog manager implementation and
the weblogentry action. The code looks good to me, but there are test
failures in the BookmarksManager, RefererManager and PlanetManager.
I'd like those to be fixed or accounted for before merge.
- Dave
On Apr 18, 2006, at 10:27 AM, 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.
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
