Inlining again. Sorry.
----- Original Message -----
From: "Allen Gilliland" <[EMAIL PROTECTED]>
To: <[email protected]>
Sent: Monday, April 10, 2006 9:28 AM
Subject: Re: Roller persistence transactions, do we need begin(), commit(),
rollback()?
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.
The concern is that other calls made, not just Hibernate work, in the "...do
some work" sections can throw different runtime exceptions and errors
between the two points.
(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.
OK. I must have missed this pattern.
(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().
No it's not a Hibernate issue. However, if all of our operations forward to
views which go back to the manager methods as you indicate to repopulate a
session, then we're probably fine.
(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
