You make a lot of good points. Comments inline ... On Thu, 2006-03-09 at 13:44, David M Johnson wrote: > I'm going to respond to your points in a different order than you > posed them, because I want to make one thing clear up-front. > > > On Mar 9, 2006, at 3:06 PM, Allen Gilliland wrote: > > 3. finally, because of the issues that i saw from item #2 i wanted > > to suggest what i believe is far more simplified approach than what > > we have now. that approach is quite simply to push all the > > transaction demarcation code back behind the XXXManager > > implementations and let the various persistence classes take > > control over that logic. this pulls a reasonable amount of code > > outside of the web layer and allows our servlets and struts actions > > to do persistence operations in a more abstracted fashion. i.e. > > rather than creating 6 objects and saving them in a struts action > > as part of new weblog creation, instead we have a method called > > createWeblog() which takes in all the required data and handles the > > details of how to persist that data. > > If you are really going to move all application and persistence logic > from the Struts Actions and Servlets and completely remove the need > for the front-end to ever call fine-grained methods like > retrieveObject(), removeObject(), saveObject() then I am am FOR this > effort and would like to participate. However, this is a large job > and will mean sweeping change across the system. I'm not comfortable > with this happening without more up-front design and I'm not > comfortable with it happening in the trunk -- plus, I'm more than > willing to do the branch management work.
cool. i didn't mean to suggest that i just wanted to do this without anyone looking at it, so i am more than happy to pull together a more detailed proposal for this and to do the work in a branch until it's ready. > > > Allen Gilliland wrote: > > Doing an exhaustive explanation of every piece of code that would > > move is pretty tough. anything that truly requires a transaction > > would be moved into a logical method in a manager class. > > I don't think it would be asking to much to see what the new manager > interfaces will look like. To design the new interfaces you'll have > to review the Actions and Servlets (comment, trackback, MetaWeblog, > Atom protocol, etc.) and how they all use transactions. You need to > do that any way. Doing it up-front is important because it 1) should > reveal any problems that you'll might encounter along the way, 2) > will make it easier to determine level of effort for this work and 3) > will make it easier to divide up the work of implementing and testing > the new methods. you are correct, it isn't asking too much. i'll work on elaborating on the proposal before we commit to anything. in particular i'll focus on the Manager interfaces and how i see them changing. > > > > 1. the first and foremost issue is performance. i have spent a > > considerable amount of time over the recent months evaluating where > > i believe Roller is having performance bottlenecks and trying to > > sort them out as best i can. after looking more closely at > > Hibernate as it is used by Roller i have seen some very troubling > > performance issues. for example, we are generating more traffic > > talking to the db than we are serving requests and the Hibernate L2 > > cache is of essentially no help to us right now. this is a huge > > problem for large sites and it has to be fixed. > > Those are definitely important issues and I think it's very important > that we understand their root cause. I don't think that simply moving > transactions around alone is necessarily a solution for those problems. this is true. i actually didn't mean to suggest that this was a real cause for the refactoring, more that it was the beginning of my inspiration. at the end of the day the caching issue is an issue of how we query for things and transaction support is more about how we store things, so they are separate issues. somewhat related though, since they both happen in the persistence layer. > > > > 2. while doing my debugging on the performance issues i noticed > > that the current Roller transaction demarcation code really isn't > > implemented completely. calls to the begin() methods are used > > somewhat sporatically, and most important, calls to rollback() are > > non-existent. i don't know how we can claim true transaction > > support if we aren't doing rollbacks. we also are not using > > Hibernates Transaction wrappers either, which is highly recommended > > in the Hibernate docs. this leads me to believe that we need a bit > > of an overhaul on the general backend code structure. > > I don't think that is a reason for this specific refactoring either, > I believe begin(), commit() and rollback() are called systematically > and the pattern we are using is fundamentally sound. If we were using > a database that supported that, we could test this. you are right, there is nothing wrong with the pattern. my observeration was more along the lines of 1) that we haven't actually implemented things to actually make use of the pattern we have now and 2) that i think we can simplify our current pattern to make things easier on ourselves. i'm not sure what you mean by a database that supports it. mysql innodb databases support transactions AFAIK. and i'm sure that oracle does as well, which some people appear to be using. > > I really want to make sure we get transactions right. Personally, I'd > like to turn on transaction support on all Roller systems that I'm > involved with and drop databases that can't handle that. i agree, i want to get it right as well. maybe more appropriately, we *need* to get it right. I don't think the underlying database is really our problem though. i think we solve the problem assuming the database supports transactions and if it doesn't that's someone elses problem. > > > > i believe this approach will work fine for Roller and since i am > > already planning to dig into the backend code to resolve issues #1 > > and #2 i thought i would suggest making some simplifications (#3) > > at the same time. > > > > at the end of the day this may be a case where either method will > > work and it's just a matter of choosing one. we can either upgrade/ > > fix our current transaction demarcation code or migrate to the > > approach i proposed which pushes the transaction logic behind the > > manager classes. there may not be a "correct" approach. > > I think moving the transactions behind the interface wall is > basically a good approach and probably the right approach, but I've > never developed a web app that was able to achieve that goal (I'm not > trying to say it is unattainable). I've always used the open session > in view, each request is a transaction approach. You gain a lot of > flexibility by being able to do CRUD operations straight from the > presentation layer and it's hard to give that up. i feel like we are still a little disconnected here. my approach will not alter anything about our ability to use the open session in view pattern. the only difference i see is the difference between these 2 code blocks ... old way: begin(); website.store(); commit(); new way: WeblogManager wmgr = roller.getWeblogManager(); wmgr.saveWebsite(website); that's it. if saving a website requires multiple steps then that will be handled behind the scenes by the implementing weblog manager and if something goes wrong at any point during that operation then an exception is thrown and a rollback happens. the only difference is that outside of the persistence layer you are no longer required to deal with calling transaction specific methods. this is how i have always done things and i have never had a problem with it. but i will fully admit that this only works for reasonably simple transactions and if our transactions really are *that* complex then maybe it won't work. i'll work on flushing out the proposal further with more details about what manager methods will be added/changed, but i still feel that this should be possible in Roller. -- Allen > > - Dave >
