Hi Allen:
I took a quick, not very thorough look. A couple of comments/questions:
(1) In the manager implementations, there are some method bodies now that
have the following pattern:
try {
this.strategy.getSession().beginTransaction();
... do some work
this.strategy.getSession().getTransaction().commit();
} catch (HibernateException ex) {
try {
this.strategy.getSession().getTransaction().rollback();
} catch (HibernateException ex) {
log.error("Error during rollback", he);
}
strategy.release();
throw new RollerException(ex);
}
The first catch probably needs to be much more general, catching either
Exception or Throwable. Perhaps the second too, or put the release in
a finally at that scope. This form of exception handling recurs often
enough to make it an additional method (on the Hibernate strategy?)
(2) Some methods commit, but some do not; they use
beginTransaction() (which does have the appropriate conditional semantics)
but do not commit. This seems to be intentional and intended to facilitate
composition of calls within one transaction for those methods. However, I
coudn't see an easy way to tell which methods are of which type without
looking at the method bodies. This could be handled in the JavaDoc comments
or perhaps better by some naming conventions.
(3) Concern/question: With this scoping of commit() we might see increased
likelihood of weird behavior in views immediately after commits on some
databases. In particular (a) in some databases they might not yet reflect
committed changes of the transaction just committed in other transactions
and (b) we might hit LazyInitializationExceptions . For some background on
the latter issue see the Hibernate reference manual Section 11.1 and 11.1.1,
and http://www.hibernate.org/43.html, particularly the FAQ 'Can I commit the
transaction before rendering the view?' The latter problem could be
addressed by ensuring we create a second transaction for the view before any
objects in the session get referenced by the view code; maybe this is what
you are already doing, but it wasn't clear this is happening. Is it already
dealt with?
(4) Impertinent comment: The approach of moving the demarcation up to a
Filter (rather than down to the manager code) still seems cleaner and more
conventional to me.
--a.
----- Original Message -----
From: "Allen Gilliland" <[EMAIL PROTECTED]>
To: <[email protected]>
Sent: Saturday, April 08, 2006 6:26 PM
Subject: Re: Roller persistence transactions, do we need begin(), commit(),
rollback()?
Ok, first set of changes have been committed for anyone who wants to take
a look.
Most of the re-architecting has been done now and I have begun cleaning up
the things that got broken in the process. So far I have cleaned up
pretty much all of the basic stuff, including operations on users, roles,
websites, permissions, and website templates.
The next things I'll be cleaning up will be pings, weblog contents
(entries, categories, bookmarks, comments, referers), and then the planet
manager.
-- Allen
