You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by Jacopo Cappellato <ja...@hotwaxmedia.com> on 2012/07/08 19:41:55 UTC
Re: svn commit: r646349 - in /ofbiz/trunk/framework: appserver/ appserver/src/org/ofbiz/appservers/ appserver/templates/wasce2/ base/src/base/org/ofbiz/base/util/string/ webapp/src/org/ofbiz/webapp/control/ webtools/webapp/webtools/WEB-INF/
On Jun 25, 2012, at 7:12 PM, Jacques Le Roux wrote:
> From: "Jacopo Cappellato" <ja...@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
Re: svn commit: r646349 - in /ofbiz/trunk/framework: appserver/ appserver/src/org/ofbiz/appservers/ appserver/templates/wasce2/ base/src/base/org/ofbiz/base/util/string/ webapp/src/org/ofbiz/webapp/control/ webtools/webapp/webtools/WEB-INF/
Posted by Jacques Le Roux <ja...@les7arts.com>.
It's actually finished and has been used by a company during 2 years. Initially I did it like the other appservers has been done
precedently.
But I then got a requirement to automatically deploy. This works began at http://svn.apache.org/viewvc?view=revision&revision=646349
and was completed, see
https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=7045153#Geronimo&IBMWebsphereApplicationServerCommunityEdition-Setupanddeploy(orredeploy)
The only part which was not completed is the 2n point
"2. Instances are differents or mixed (some instances may be the same)" at the link I gave below.
>> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=7045153#Geronimo&IBMWebsphereApplicationServerCommunityEdition-Multiinstances
where it's explained.
Note that it's rare to have a complete documentation in wiki of a such feature (a bit exotic in OFBiz world).
>From comment I got from users and even consultants, with it's aging UI, documentation is what OFBiz misses more...
Anyway, as I said, I think nobody use it anymore. The company which payed for it has been sold and restructured since. So it's no pb
for me
to put in Attic, from where it can be resurrected at any moment (mayber with some pain later though, since part of code used whiil
change)
Then this must be done carefully to allow its possible use later. Though it seems also that Geronimo did not have a big momentum
lately, so I don't expect much about that...
For the appserver component, we would need a larger consensus. If ever people are interested to simply give their opinions :/
Else, then Attic, people will have to help themself more if they are not supporting the team enough! Most of the time they come,
pick what they need and go.
Jacques
From: "Jacopo Cappellato" <ja...@hotwaxmedia.com>
> 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" <ja...@hotwaxmedia.com>
>>> On Jun 25, 2012, at 7:12 PM, Jacques Le Roux wrote:
>>>
>>>> From: "Jacopo Cappellato" <ja...@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
>>>
>>>
>
>
Re: svn commit: r646349 - in /ofbiz/trunk/framework: appserver/ appserver/src/org/ofbiz/appservers/ appserver/templates/wasce2/ base/src/base/org/ofbiz/base/util/string/ webapp/src/org/ofbiz/webapp/control/ webtools/webapp/webtools/WEB-INF/
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
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" <ja...@hotwaxmedia.com>
>> On Jun 25, 2012, at 7:12 PM, Jacques Le Roux wrote:
>>
>>> From: "Jacopo Cappellato" <ja...@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
>>
>>
Re: svn commit: r646349 - in /ofbiz/trunk/framework: appserver/ appserver/src/org/ofbiz/appservers/ appserver/templates/wasce2/ base/src/base/org/ofbiz/base/util/string/ webapp/src/org/ofbiz/webapp/control/ webtools/webapp/webtools/WEB-INF/
Posted by Jacques Le Roux <ja...@les7arts.com>.
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" <ja...@hotwaxmedia.com>
> On Jun 25, 2012, at 7:12 PM, Jacques Le Roux wrote:
>
>> From: "Jacopo Cappellato" <ja...@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
>
>