geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jules Gosnell <ju...@coredevelopers.net>
Subject Re: -1 on checkin of 368344 was Re: [wadi-dev] Clustering: WADI/Geronimo integrations.
Date Thu, 12 Jan 2006 17:19:48 GMT
see inline - I ommitted one further technical benefit of my approach:

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.
>
>> 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.
>
>> 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.
>
> I have also invited you to work on removing this dep from WADI.
>
>> 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.
>
> 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.
>
>> 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 ?
>
>> 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.
>
>> 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 unifies two separate approaches into a single, more manageable 
> approach, without sacrifice.
> - 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.
> - 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.
> - it is small.

- it coordinates the use of the <distributable/> tag in the web.xml with 
the selection of a clustered, as opposed to non-clustered session 
manager - a further useful enhancement.

Jules

>
> On the non-technical side of things:
>
> - preceding this change, possible solutions were discussed on relevant 
> dev lists at length.
> - 3 Geronimo/WADI committers were involved in and agreed on the final 
> minutiae of the change.
> - by fixing, simplifying and unifying the WADI/Geronimo integration, 
> this changes brings significant benefit to Geronimo.
>
> 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.
>
> 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, dev@wadi.codehaus.org).
>>>>
>>>> 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
>>>>
>>>>     
>>>
>>>   
>>
>
>


-- 
"Open Source is a self-assembling organism. You dangle a piece of
string into a super-saturated solution and a whole operating-system
crystallises out around it."

/**********************************
 * Jules Gosnell
 * Partner
 * Core Developers Network (Europe)
 *
 *    www.coredevelopers.net
 *
 * Open Source Training & Support.
 **********************************/


Mime
View raw message