Hi Jacques, if you could help to remove all code that is specific to Geronimo/WASCE that would be great in my opinion; some of the code I have seen is really a kind of tweak and if it is not finished it would be better to clean everything and then, when someone will really have to work with Geronimo again, and if the issues you have faced are still there in the new versions, then we could see if we can improve the framework without requiring a specific handling/tweak for Geronimo. As regards the appserver component... in my opinion it could go to specialpurpose or to attic (all or a part of it).
Regards, Jacopo On Jul 14, 2012, at 11:49 AM, Jacques Le Roux wrote: > 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 >> >>
