I am sorry Jules, I am -1 on the change and I stand firm on that. My comments below...however, we need open discussion on the G lists as well as consensus agreement to what you are trying to do in Geronimo. Lets try to open up the discussion for everyone to view and discuss.
Jules Gosnell wrote: > Jeff Genender wrote: > >> Hi Jules. >> >> A few comments. First, you made changes without discussing them on the >> dev lists. >> >> >> > There has been lots of discussion over the past week or two on both > geronimo-dev and wadi-dev - I took this, along with my own findings when > I looked at the code, and further offline discussion with Jan and Greg > as we were making the changes, into account. > > As I have clearly stated, if you don't like the way it has been done, it > is only an iteration towards a final solution and you are welcome to > contribute codewise. It is a major step forwards as it unifies the way > that this is done between both containers, and further allows the use of > clustering solutions other than WADI. I am sorry, but I don't agree. > >> As per the discussions in the past, both Aaron and David Jencks, as well >> as I threw in our .02 on how to integrate the clustering. I would >> appreciate you discuss code ideas and changes >> > I was involved in the recent threads (I started them and stoked them) > and did discuss these issues. Please check the archive. To say that I > was not open to discussion is not a tenable position. > >> that have such a drastic >> impact on the Geronimo code base. > Drastic ? It extends Geronimo to be able to run the WADI demo webapp, > with no impact whatsoever on any non-distributable webapp. With web app dependencies ion the container. Yes, that is drastic. > >> Here are the issues with your check in: >> >> 1) I explained before for Jetty, and obviously now I need to do it for >> Tomcat, a -1 on Axion as a dependency. There should not be any web >> application dependencies injected at the container level. This means >> there is a severe architectural issue with WADI when we are injecting >> these dependencies into the container. >> >> > It is no longer an app dep - it is a container dep. The decision to use > WADI is now made by the container, and as stated in my mail to the list, > the config which determines this will soon move from the app to the > container. > Then why is Axion necessary? Its only use is at the web application level by my perusal of the WADI code...and testing classes at core. > I have also invited you to work on removing this dep from WADI. > When we have fully moved to incubator, I would be glad to work on this code. >> 2) You hard coded in org.codehaus.wadi.tomcat55.TomcatManager as the >> distributablesession manager in the TomcatContainer. Hardcoding a >> pluggable session engine is very bad, and defeats the pluggability of a >> configuration that we requested. >> >> > Jeff, please take the time to read, run and understand the code before > judging it. Jules, I did. I looked at the code and was not happy with how you integrated it and it completely defied the input you got from 3 other Geronimo committers. Lets start over and open up the discussion on the Geronimo lists so the G folks can get their input. I would be more than happy to start this discussion. > > As stated in my mail, a sensible default distributable session manager > is hardcoded. This is overridable in the tomcat or jetty plan. This is a > pretty standard way of doing things and means that any session manager, > not just WADI may now be selected. This is a great step forward over the > previous version where an important method signature included the > WADIGBean type, which restricted distributable webapps to WADI and not > other possible alternatives. I don't agree. There should be no hard coding at all. I also don't like the > >> 3) You placed log.info() in the code, and Aaron worked pretty hard to >> clean those up. >> >> > I shall downgrade the level - apologies to Aaron - as I stated, this > code is only an iteration towards a finished product. > >> 4) Your integration of setting the manager (no matter what) is a direct >> clash with the >> >> > with the..... what ? With the Manager. > >> Jules, I am giving a complete -1 of checkin of 368344. These are all >> for technical reasons. Please back out these changes, and bring this >> discussion to the Geronimo lists as this needs some significant >> discussion for implementation. I would appreciate that you please >> involve the Apache way and open discussions on the lists before doing >> this sort of thing in the future. >> >> > Of the three reasons that you have given 2 are completely mistaken and > one is trivial - in my book, insufficient technical argument for the > rejection of a significant enhancement. I am sorry , I don't agree. > >> Again, I will CC the G lists to make this clear, that I would like this >> change backed out. >> >> > In conclusion my change should remain for the following technical reasons. > > - it fixes something that was broken It wasn't broken at all. However, we can remove what you believe was broken. > - it unifies two separate approaches into a single, more manageable > approach, without sacrifice. The way it was done I am in disagreement with. Lets discuss all of these issues. > - it moves us in an agreed direction (from per-app to per-container > based configuration) > - it is simpler than what it replaces - it frees us from requirements > for an extra GBean and divergent Jetty and Tomcat geronimo-web.xml schemas. The reasons for the Gbean is the pluggability of the Manager. Not everyone wants to use WADI and may have their own Manager they want to use. The GBean leveraged both...so don't be so quick to remove it. However, we can discuss the best way to do this. > - it is more flexible than the code that it replaces - it allows > selection of ANY session manager, NOT JUST WADI, as was previously the > case. I don't agree. Where can I set the Manager properties specific to something else? The way you have it, you cannot. With the Gbean, I could. > - it is small. Small or large, its not the right way, IMHO. Jules, -1 on the change. I am rejecting it based on technical reasons. Lets get a plan together on the G lists and get clear consensus of Clustering plugs into Geronimo appropriately. > > On the non-technical side of things: > > - preceding this change, possible solutions were discussed on relevant > dev lists at length. No Jules, you did not propose exactly what you wanted to do. You simply said you will be discussing things with Jan and Greg at your house. The next communication from you was a major change without getting input on your ideas. Sorry Jules, but I think we disagree vehemently here. > - 3 Geronimo/WADI committers were involved in and agreed on the final > minutiae of the change. AFAICT, I never saw this happen...and I surely would not have agreed with the entire approach you took. Please show emails to this regard. > - by fixing, simplifying and unifying the WADI/Geronimo integration, > this changes brings significant benefit to Geronimo. I don't agree. I am not happy with the approach. > > If there are aspects of the change that you do not like, then we should > simply work together, on top of the change, to resolve these issues. No Jules, lets start with a clean base and move ahead on agreement. My -1 stands. I have backed out the change you made. i also have an open -1 on the Axion for Jetty. Please remove that. Jeff > > By backing out the change, you break something that is fixed and remove > all the beneficial code that you did not have issues with. If there are > small issues, such as the level of a log message, then we should simply > fix it and continue in a forward direction. > > regards, > > > Jules > > >> Jeff >> >> >> Jules Gosnell wrote: >> >> >>> Here is a list of outstanding issues associated with this work: >>> >>> - ActiveMQ's shutdown hook seems to trigger when Geronimo is shutdown, >>> removing AMQ before WADI - WADI doesn't like this. I have added a >>> property to the node.sh script which suppresses this behaviour. I will >>> document it in the Getting Started doc. >>> >>> - There 'may' be issues with nodes finding each other, when a Geronimo >>> node is introduced into a WADI cluster - investigating. >>> >>> - Jeff - you should look over the changes and make sure that they do not >>> impact on any other TC fn-ality. They were done with Emacs, so the >>> formatting may be offensive. Please feel free to make them your own and >>> bring any issues back to the list. The WADIGBean, is no longer used, so >>> you may want to remove this from the repo. >>> >>> - Jan and Jeff - since this config is now done on the container bean and >>> not in the geronimo-web.xml, you may no longer need to implement your >>> own geronimo-web.xml schemas (I haven't looked very closely at TC). You >>> may want to consider this and perhaps lose them. >>> >>> - In order to get the same webapp to work in all containers >>> (tomcat5[05], jetty[56], geronimo-[tomcat/jetty], jboss-tomcat), I had >>> to move deps back to Geronimo container-level. These include Axion, >>> which I know will upset Jeff. As I have stated before, WADI's dependence >>> on Axion is easily removed. If Jeff or anyone wants to look at replacing >>> it with Derby, it is fine with me, as long as they do some testing and >>> confirm that having created a session on a single node and restarted it, >>> the session survives (if the DB is still running). This needs to be >>> tested on all supported containers. Axion was used because it is an >>> in-VM DB (so imposes no further integration dependencies on the Getting >>> Started stuff and is useful for unit-testing) and was in use by Geronimo >>> at the time. So I suggest that any replacement needs to also be able to >>> run in-vm aswell. As we go further and move WADI's actual configuration >>> from the app to the container-level, these issues will disappear and >>> WADI will be able to be hooked to whatever persistance mechanism is >>> shipped in Geronimo by default. >>> >>> - Jan & Jeff , you may want to consider pushing some of this session >>> manager selection code up into a shared GeronimoWebContainer abstraction >>> so that you don't both end up maintaining similar but diverging code... >>> >>> - I may have overlooked a couple of issues. If I come across them, I >>> shall post them. >>> >>> Further work on Geronimo integration : >>> >>> - more testing >>> - make a new WADI release and update geronimo-trunk to use it >>> - look at applying diffs to a G1.0 tree and producing a binary patch for >>> 1.0 distros. >>> - update website and release it >>> >>> >>> Jules >>> >>> >>> >>> Jules Gosnell wrote: >>> >>> >>>> Guys, >>>> >>>> Jan and I have just refactored the Geronimo Jetty and Tomcat >>>> integrations to take the same approach to the installation of a 3rd >>>> party session manager, to ease the integration of WADI. This is now >>>> checked in on Geronimo's trunk. >>>> >>>> Each top level web container GBean now supports a pair of attributes - >>>> LocalSessionManager and DistributableSessionManager. These may be used >>>> to override the container's choice of SessionManager for webapps with >>>> and without the <distributable/> tag present in the WEB-INF/web.xml, >>>> respectively. >>>> >>>> The attributes expect to be given a classname, if required, this class >>>> will be loaded and instantiated. The resulting instance will be used >>>> as the session manager. If not provided, the container will use a >>>> sensible default. Currently Jetty and TC are set up to use their own >>>> default session managers in the local case and the correct WADI >>>> session manager in the distributable case. >>>> >>>> This means that the same WADI-enabled webapp, with its plan held >>>> internally (WEB-INF/geronimo-web.xml) may now be hot-deployed on >>>> either a Jetty or a Tomcat based Geronimo, without changes :-) >>>> >>>> I will post specific WADI issues to the WADI dev lists >>>> (wadi-dev@incubator.apache.org, [EMAIL PROTECTED]). >>>> >>>> This shouldn't be seen as a final position on the subject - there is >>>> still much to talk about, but is a useful interim step, that allows us >>>> to have something working whilst we figure out how to go forward. >>>> >>>> Enjoy, >>>> >>>> >>>> Jules >>>> >>>> >>> > >
