Jeff Genender wrote:
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.
I don't see how you can. The threads are clearly available to anyone who
wants to browse the archives:
geronimo-dev/wadi-dev - "Re: geronimo-web.xml, container-config,
container-specific namespaces"
wadi-dev: "geronimo integrations...", "WADI configuration..."
I think the area that I was working on was pretty well broadcast and
that you could have mentioned that this was an opportune moment to unify
not only the two disparate approaches to WADI, but also the alternative
Tomcat clustering components.
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.
I repeat. WADI configuration is moving along a discussed and agreed
course towards per-container as opposed to per-app configuration. WADI
currently requires this dep (although it should not be hard to remove it
and replace it with another DB as I have clearly indicated). The fact
that it is a container-dep should put this point to bed.
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.
see my previous comments about per-app vs per-container config/deps.
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.
why not just fix it at codehaus - you have commit - or send me a patch
and I will do it.
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.
No, you didn't Jeff - otherwise you would have understood that this was
only a default value and could be trivially overriden in the container's
plan. Behaviour for non-distributable webapps has NOT been changed.
Behaviour for distributable webapps has been usefully defaulted. Please
see the code extracts below.
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
But, Jeff, before my change, your TomcatWebAppContext had a compile-time
reference to a WADIGBean type, which was imported and used in its
constructor's signature.
from my changes :
-import org.apache.geronimo.tomcat.cluster.WADIGBean;
...
public TomcatWebAppContext(
ClassLoader classLoader,
String objectName,
@@ -133,7 +135,6 @@
ObjectRetriever tomcatRealm,
ValveGBean tomcatValveChain,
CatalinaClusterGBean cluster,
- WADIGBean manager,
boolean crossContext,
Map webServices,
J2EEServer server,
@@ -161,6 +162,8 @@
}
@@ -458,7 +461,6 @@
infoBuilder.addReference("TomcatRealm", ObjectRetriever.class);
infoBuilder.addReference("TomcatValveChain", ValveGBean.class);
infoBuilder.addReference("Cluster", CatalinaClusterGBean.class,
CatalinaClusterGBean.J2EE_TYPE);
- infoBuilder.addReference("Manager", WADIGBean.class);
infoBuilder.addAttribute("crossContext", boolean.class, true);
infoBuilder.addAttribute("webServices", Map.class, true);
infoBuilder.addReference("J2EEServer", J2EEServer.class);
@@ -488,7 +490,6 @@
This is 'hard-coding'
My change only mentions WADI in two places - once in Jetty and once in TC :
+ private String
_distributableSessionManager="org.codehaus.wadi.tomcat55.TomcatManager";;
+ private String distributableSessionManager =
"org.codehaus.wadi.jetty5.JettyManager";
Neither places any compile time dependency on WADI. All deps are runtime
and easily overriden by the setting of attributes in your plan :
+ <attribute
name="distributableSessionManager">org.codehaus.wadi.tomcat55.TomcatManager</attribute>
and
+ <attribute
name="distributableSessionManager">org.codehaus.wadi.jetty5.JettyManager</attribute>
any session manager class may be selected. This is 'soft-coding a
sensible default'. If you wish to change the default, you have only to
do so.
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 was broken - The Jetty integration did not work. You claimed to have
a working TC integration, but failed to deliver it. The integrations
were mutually incompatible as discussed in the 'geronimo-web.xml' thread
on geronimo-dev. A single unified approach is required for both
containers and discussed in this thread. We simply got on and
implemented it. I regret that our discussion did not bring to light your
requirement for a further unification with alternative TC clustering and
will work on resolving this.
- 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
as stated, our changes do not impose the choice of any particular
manager on the app/container. This is plainly visible to anyone who
looks at the code. Why, therefore, do you continue to say that this is
not the case ?
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.
I am happy to look at alternatives. I wish that our discussion before
the changes had brought your requirements to light.
- 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.
This was mentioned in the discussion thread,
When it came to implementation, I looked at the existing hard-coded use
of a WADIGBean, on which no attributes were published or configured.
Knowing that it mapped to a WADI component on which no attributes would
ever be added (because by its very nature it requires a void ctor and
all configuration is done by modification of components below it via an
LWC), we decided to simplify it to simply instantiate a class.
If this raises an issue with alternative TC clustering mechanisms, then
as indicated, I am open to discussion about it.
- 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.
I still do not think that you have put a decent technical case for the
changes rejection.
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.
Jeff, look back at the various threads that I have mentioned in this
thread. All were on the subject of integrating WADI with Geronimo,
resolving issues around the geronimo-web.xml and per-container vs
per-app issues. These were discussed openly and publicly. Of course,
further discussion about the change occurred as we made it. Any change
involves a number of smaller and faster iterations as it is actually
made. If we opened everyone of these up to public discussion we would
never move forward. Ultimately you have to make a call and get coding.
I don't think my saying anything else here is going to help matters.
In conclusion I think that you have raised only one issue of
significance - how this change may interact with other clustering
solutions available to TC.
You want the whole change rolled back and started again from scratch. I
think we should do the following :
Ascertain whether the change actually breaks your alternative clustering
mechanisms - I have already put this question to you and not received a
response.
If no breakage has occurred, then I see no reason for the change to be
rejected.
If breakage has occurred, then you should let me know what I have
broken, so I can fix it.
If the approach that we have taken is sufficiently flexible that it can
express current requirements with a small fix, then it goes in and we
have a discussion about further iterations on this subject. Meanwhile,
Geronimo-1.0.1 goes out with both WADI and alternative TC clustering
strategies available to it - everyone is happy. Later on, we find a way
to unify all three.
If we cannot trivially fix the change to remove breakage, then I am
happy to reconsider and refactor the change.
Why this seems to be such an enormous and contentious issue, I really
don't understand.
Jules
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
([email protected], [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
--
"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.
**********************************/
