geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Bohn <joe.b...@earthlink.net>
Subject Re: svn commit: r712326 - in /geronimo/server/trunk: framework/modules/geronimo-kernel/src/main/java/org/apache/geronimo/kernel/classloader/ framework/modules/geronimo-kernel/src/main/java/org/apache/geronimo/kernel/config/ framework/modules/geronimo-kerne...
Date Wed, 19 Nov 2008 19:49:56 GMT
Gianny Damour wrote:
> 
> On 19/11/2008, at 5:19 AM, Joe Bohn wrote:
> 
>> Joe Bohn wrote:
>>> Just a heads up that I *think* there are still some issues with this 
>>> change.
>>> It appears that the hiddenResource processing from the 
>>> MultiParentClassloader was removed.
>>
>> Correction ... it was not removed but rather changed and relocated.  
>> I'm still looking to understand why this is causing a problem.
> 
> Hi,
> 
> Indeed, I changed the implementation to properly encapsulate class 
> loading rules. The new implementation is way cleaner this way; when my 
> frustration coming from reported issues will reduce, I may use this 
> better encapsulation to add import and export class loading rules.

I agree that what you added for the ClassLoadingRules is cleaner. 
However, I think it might be the ChildrenConfigurationClassLoader and 
it's replacement of MultiParentClassLoader in the Configuration that 
might be causing us problems.  The testsuite failures that I mentioned 
are failing as well as nearly all of the jstl tck tests since this 
change.  Perhaps it is working as designed, but it seems strange to me 
that as we process the "parent" classloaders they are all of type 
"ChildrenConfigurationClassLoader" when they used to be 
"MultiParentClassLoaders".

BTW, I've verified that these tests were not failing prior to this 
change and begin to fail with just the change a few other necessary 
changes (the reverting of the xsd changes and fixing the dependency 
history files).

> 
>>
>> I think this has resulted in some
>>> testsuite failures involving tld processing.  There are failures in 
>>> the web-testsuite/test-2.1-jsps and web-testsuite/test-myfaces 
>>> tests.  I'm looking at what would be necessary to add in the 
>>> hiddenResource logic again hoping that will resolve the issue.
>>> This is a really nice feature and it is great to have the capability. 
>>> However could you please run the testsuite in the future to avoid 
>>> problems like this (especially when introducing fundamental changes 
>>> like this)?
> 
> If this is indeed a bug related to this change, then let's ensure that 
> we have a proper unit test to prevent regression. Let's also ensure that 
> this test is collocated with the proper component to improve cohesion. I 
> have been observing various changes, many of them do not have proper 
> unit tests and this causes problems further down the path as other 
> developers can not safely change code.

I'm fine with that, but I'm sure there are probably a number of more 
obscure situations that aren't covered by the unit tests ... they can 
only do so much.  That's one of the reasons for the testsuite.

> 
> Regarding private-classes, the current Geronimo <-> OpenEJB DD coupling 
> is unfortunate. Does the OpenEJB deployer needs to know about the 
> Geronimo environment? If it does not need to know about it, then let's 
> strip from the DD the environment element and pass to OpenEJB the 
> stripped version. This will reduce a little bit coupling. Another 
> approach is to transform the Geronimo DD into an OpenEJB supported DD 
> when it is handed over to OpenEJB (in this case, we simply remove the 
> private-classes). The creation of a canonical DD representation between 
> Geronimo and OpenEJB will reduce coupling.
> 
> I let you re-revert the private-classes change.

I can't unrevert these changes since I'm not a committer on OpenEJB ... 
but perhaps we should wait on that anyway until we resolve the JSTL 
problems.

> 
> Thanks,
> Gianny
> 
> 
>>> BTW, I'd personally like to see the plan changes re-introduced for 
>>> private-classes if it turns out that we need an OpenEJB release 
>>> anyway (and at this point in time I think that is the case).  I think 
>>> users are  more accustomed to using declarative plans for this type 
>>> of thing at the moment and would find this helpful.
>>> Thanks,
>>> Joe

removed content of original change from this discussion thread.

Mime
View raw message