Hi Jacopo,
If it's only that part (all related to GeronimoMultiOfbizInstances) which worries you then no problem for me to revert it (I will
put a note into Attic though).
It was a try (not in requirements I had, but made sense) but I never really got a chance to finish and nobody was interested it
seems.
https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=7045153#Geronimo&IBMWebsphereApplicationServerCommunityEdition-Multiinstances
We could even put all Geronimo/WASCE in Attic
I think nobody is really using it nowadays. Anyway a mention and link to Attic
from the wiki page would do it.
BTW this list of commit should help since removing
framework\appserver\templates\wasce2 will not be enough
http://svn.apache.org/viewvc?view=revision&revision=643173
http://svn.apache.org/viewvc?view=revision&revision=646349
http://svn.apache.org/viewvc?view=revision&revision=652177
I can do it myself if you want
BTW I wonder if we should keep all appserver implementations in trunk? Some begin to be really old. I must say that they are much
simple, the challenge in the requirements was to deploy automatically (r646349)
Jacques
From: "Jacopo Cappellato" <jacopo.cappell...@hotwaxmedia.com>
On Jun 25, 2012, at 7:12 PM, Jacques Le Roux wrote:
From: "Jacopo Cappellato" <jacopo.cappell...@hotwaxmedia.com>
Hi all,
this is part of my review of framework code to make it cleaner. I see a series
of questionable changes in this commit, do you
agree?
Jacques, before I start my review please let me know if you want to do a first
pass on it.
I can certainly have a fresh look, depends on timing though...
What did you saw at 1st glance which makes you want to improve, is it related
to GeronimoMultiOfbizInstances?
Jacques
Mostly all the code in the commit seems questionable and I would like to see it
reverted or completely refactored.
In particular, the changes to ContextFilter are awful:
String GeronimoMultiOfbizInstances = (String)
config.getServletContext().getAttribute("GeronimoMultiOfbizInstances");
if (UtilValidate.isNotEmpty(GeronimoMultiOfbizInstances)) {
String ofbizHome = System.getProperty("ofbiz.home");
if (GeronimoMultiOfbizInstances.equalsIgnoreCase("true") &&
UtilValidate.isEmpty(ofbizHome)) {
ofbizHome = System.getProperty("ofbiz.home"); // This is only
used in case of Geronimo or WASCE using OFBiz
multi-instances. It allows to retrieve ofbiz.home value set in JVM env
System.out.println("Set OFBIZ_HOME to - " + ofbizHome);
System.setProperty("ofbiz.home", ofbizHome);
}
}
In just a few lines added to the class I see:
* bad variable name ("GeronimoMultiOfbizInstances")
* two if blocks that should be one
* a bad practice for logging
* all the code seems like a bad tweak
* the code doesn't make any sense to me... to the point that I may be
misunderstanding it: why do you get the property and then
set it? how this is going to fix the problem? what was the problem?
Why did you commit code like this? Did you really think it was good for OFBiz?
Regards,
Jacopo
