I know that there are a couple tests still failing on the
RefererManagerTest which I had planned to tidy up today, but the
BookmarkManager and PlanetManager tests work fine for me.
I think the PlanetManager test was probably failing because I had added
a couple lines for proxy configuration that had gotten checked into svn
on accident. I just removed them, so try again now.
Also, not that this really matters now, but it was kind of hard to guage
what to expect from some of the tests because if I run the
"test-business" target from the current trunk I pretty much get all
errors. So I wasn't sure if some of the tests were already failing
without my help.
-- Allen
David M Johnson wrote:
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
