On Wed, 2006-03-01 at 22:00, Anil Gangolli wrote: > OK. -1. I'm against dropping them until we understand the pattern that > is being proposed to replace them.
The pattern would be to encapsulate things in logical operations in the manager classes. When you do things like weblog.delete() or user.delete() or register a weblog, those things are multi step operations to the persistence layer, but they are a single operation logically. This is how I have always done things in the past and I have never needed direct control over the transactional state of persistence operations before. > > It does sound like we need to clean things up. In my mind this basically proves my point that we don't need control over persistence transactions in the presentation layer. The fact is that currently those transaction methods are not doing anything to provide transactions like they suggest. The way the application is running now is without transaction support, and it seems to be doing fine like that. > > Here are some things to keep in mind regarding the questions posed in > your message. > > In general we need to demarcate transactions around the entire unit of > work. The persistence layer generally does not know the unit of work > boundaries. Each call to the persistence layer is not its own > transaction. It is part of a transaction, the boundaries of which > generally need to span multiple calls from the app layer and also calls > *between* managers in the persistence layer. I agree that we need to provide transactions for multi step persistence operations. However, I believe that our Manager classes provide a bridge between logical operations like weblog.delete() or createWeblog() and the actual persistence operations which are needed to carry out those logical operations. The exact implementation of how createWeblog() works to persist the data is meant to be masked by the Manager classes specifically so that the presentation layer doesn't have to deal with it. > > Ideally there should be very few places in the app where explicit > transaction demarcation is needed. If we really have that many calls, > it probably is a sign that it isn't being done right. Typically the > demarcation should be very early on request stacks in generic places > (filters, async task execution, etc.). This part I disagree with. My opinion is that each time a db connection is opened and closed that represents a "transaction" with the database. We may open and close our db connection many times in a given request/response cycle in order to fetch data and/or save it. You can't just lump the entire request/response cycle into a single transaction. > > Regarding existing transaction behavior, there are a couple of things to > keep in mind: (a) generally there is an implicit transaction associated > with all work on the JDBC connection between commits, (b) Roller code > may in places be relying on automatic rollback on return to the pool, > which strictly speaking it shouldn't. a. fine, but that transaction logic should be hidden from the application and masked by our persistence layer classes. the whole point of architecting the persistence layer interfaces is to keep persistence specific logic out of the rest of the code. b. that's just bad coding. > > I don't think we need to refer to Hibernate Transactions explicitly. > That doesn't mean we don't use transactions now. well, i think that our Hibernate persistence implementation should use Hibernate Transactions explicitly. I still claim that our current code does not support transactions. -- Allen > > --a. > > > > Allen Gilliland wrote: > > On Wed, 2006-03-01 at 08:55, Anil Gangolli wrote: > > > >> We definitely need transactions with our coding pattern. We will get > >> inconsistent data states without them if the server goes down or a > >> request hits an exception after partially updating. > >> > > > > That's fine, we can have transactions, but does the transaction logic need > > to be controlled from outside of our persistence layer? > > > > I would expect that calling WeblogManager.createWeblog() would perform a > > number of writes which are wrapped in a transaction, but the caller of that > > method doesn't need to know about that. All it needs to know is if the > > operation as a whole was successful. > > > > I will admit that I haven't yet taken the time to truly analyze all the > > places in which we are doing persistence operations that require full > > transaction support, but even without that info I am asking the basic > > question of whether or not we need to expose persistence specific methods > > like begin(), commit(), and rollback() into presentation layer code. In my > > perfect world the details of how a persitence operation is handled is black > > boxed from the point of view of the presentation code. > > > > > >> We are using transactions, or were as of about 1.2. I'm not sure why > >> you say we aren't using them. I fixed some bugs related to this in 1.0 > >> or so. It does require that your db and db configuration support > >> transactions. > >> > > > > well, I am looking at our Hibernate implementation and we never use the > > Transaction class of hibernate, so that is one indication. another > > indication is that we aren't using the Roller.rollback() function, so > > whatever transactions we are trying to do we aren't rolling them back if > > they fail. > > > > > >> Only a few calls are necessary, so the number of calls is not an > >> indication of lack of use; the transaction demarcation calls should be > >> early in the entry points of requests into the system or in tasks > >> executed asynchronously in their own threads. These are supposed to > >> associate a transaction with the thread. Everything else gets the > >> current transaction on the thread. It's a standard pattern. > >> > > > > well, a few calls multiplied times the number of places where the > > transaction needs to be started and commited/rolled back. i consider all > > communication with the persistence layer to be a "transaction". > > > > Please note that I am not suggesting that we abandon the use of any > > transaction support. I am only trying to question whether we need to > > expose persistence transaction methods outside of our actual persistence > > layer. I fully agree that behind the scenes of the persistence layer > > transactions should be supported. > > > > -- Allen > > > > > > > >> --a. > >> > >> > >> > >> > >> Allen Gilliland wrote: > >> > >>> I have been going through some of the Roller backend and looking at some > >>> parts of our Hibernate implementation and I have gotten myself to > >>> wondering if we really need the transaction methods begin() and commit() > >>> which are part of the Roller and PersistenceStrategy interfaces. > >>> > >>> I think I understand where the idea for these methods came from and they > >>> make some sense, but after inspecting the code a bit it seems like we > >>> definitely don't actually use them. > >>> > >>> In the current code I only see this ... > >>> > >>> Roller.begin() - 6 uses > >>> Roller.begin(user) - 4 uses > >>> Roller.commit() - 7 uses > >>> Roller.rollback() - 0 uses > >>> PersistenceStrategy.begin() - 4 uses, just from RollerImpl classes > >>> PersistenceStrategy.commit() - 4 uses, mainly RollerImpl classes > >>> > >>> I also noticed that in our Hibernate implementation we aren't actually > >>> using Transactions anyways, so even if these methods were in full use we > >>> wouldn't have been taking advantage of them. > >>> > >>> So, the deal is that if we want to use these transaction methods then we > >>> need to make sure we are using them *everywhere*, otherwise they are > >>> doing us no good and only confusing things. Alternatively I would > >>> propose that we drop these methods and let implementors of a persistence > >>> strategy handle the details of transactions within their own > >>> implementations. > >>> > >>> My vote is to drop these methods. I can see the potential value in > >>> having them (if they were working properly), but my opinion is that we > >>> aren't really doing any persistence transactions complicated enough to > >>> really warrant the additional complexity and overhead of having them. > >>> We have to go one way or the other, so we either remove them or add them > >>> in everywhere. > >>> > >>> Who else has an opinion? > >>> > >>> -- Allen > >>> > >>> > >>> > >>> > >>> > > > > > > >
