Anil, thanks for taking a look. I have some comments inline ...
Anil Gangolli wrote:
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?)
I can change it to catch a greater scope of exceptions, but I believe
Hibernate wraps all of its exceptions in some version of the
HibernateException, so I imagine this should be fine.
(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.
All XXXManager methods are committed before ending. Some methods in the
HibernatePersistenceStrategy class do not commit for the reason you
mentioned, so that they can be used multiple times before committing.
There are some methods of the HibernatePersistenceStrategy which do
commit, but I think they are clearly marked as xxxAndCommit().
So I'm not really sure where the confusion is coming from. In general,
the assumption moving forward is that any call to XXXManager.method()
results in a commit, assuming it is a write operation.
(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?
This sounds more like a Hibernate issue than an architecture issue to
me. The fact is that this issue will exist in any implementation
because thread1 could have just committed some changes a split second
after thread2 read those same objects for use. Obviously we don't want
to be getting LazyInitializationExceptions, but again, that is a
Hibernate issue, not an architecture issue.
Hibernate demands that all operations happen within a transaction, so if
you peek at the code you will see that all XXXManager methods begin with
a call to session.beginTransaction().
(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.
I am still not sure I believe this, but I am happy to do it if we really
need to.
-- Allen
--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
