geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dain Sundstrom <dsundst...@gluecode.com>
Subject Re: Review - svn commit: r126264
Date Wed, 26 Jan 2005 07:19:37 GMT
Finally got some free time to respond again :)

On Jan 24, 2005, at 7:28 PM, Mark wrote:

> Thanks for the review.  I will go through your comments one-by-one  
> over the next few days.
>
> A few things to keep in mind:
>
> 1. The rmi/iiop code is fairly solid (with my fingers crossed).  I  
> hope to get in a bunch of test cases that following the Geronimo  
> design.

Excellent!

> 2. The stub/skeleton generation is farily *dirty* it.  You can  
> convience it to work, code generation style/speed were not my primary  
> objective.  The rmi-iiop was.  So I fully anticipate this to change in  
> the near future.

Yep.  The Geronimo deployment code is similar; not fast or that  
beautiful, but works.

> 3. The original code was stand alone, hence there are a few other  
> interop.* packages.  Once I learn more about Geronimo, I anticipate a  
> much neater/tighter integration.  Of course, I think I will have lots  
> of help from Alan.  :-)

I just wanted to point out some of the stuff I noticed.  I figure the  
neat/tight integration will take long time (months) as all of us get  
accustomed to new integrated code base.

> 4. I don't mind making the code or style changes to conform tothe  
> Apache project.  The current style is a required habit for other  
> projects that I work on.  :-)
>
> I've added a few quick answers below see MD>>
>
> ( I only made it 1/2 way through this email... )
>
> Expect that I will be submitting lots more changes to Alan over the  
> comming days.

Great... the rest of my comments are inline (I sniped out the stuff  
where I don't have any other comments... darn thing is just too long :)

> Dain Sundstrom wrote:

>>> Added:   
>>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/  
>>> CosNaming/iiop_stubs/NamingContext_Stub.1.txt
>>> Url:   
>>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/ 
>>> java/ org/apache/geronimo/interop/CosNaming/iiop_stubs/  
>>> NamingContext_Stub.1.txt?view=auto&rev=126264
>>>
>>> Added:   
>>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/  
>>> CosNaming/iiop_stubs/NamingContext_Stub.java
>>> Url:   
>>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/ 
>>> java/ org/apache/geronimo/interop/CosNaming/iiop_stubs/  
>>> NamingContext_Stub.java?view=auto&rev=126264
>>
>>
>> Are these generated stubs based on the IDL above?  Normally, we don't  
>>  check in generated code, but I bet these almost never change.
>
>
> MD>> The name service stubs/skels are generated.  Right now, due to a  
> couple of issues, I have hand modified them and simply checked them  
> in.  This will need to change.  All other generated files go under  
> interop/target/src

I see. You many want to put a big warning at the top saying "don't  
bother messing with this is generated and going away".

>>> Added:   
>>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/  
>>> InteropGBean.java
>>> Url:   
>>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/ 
>>> java/  
>>> org/apache/geronimo/interop/InteropGBean.java?view=auto&rev=126264
>>> ===================================================================== 
>>> == =======
>>> --- (empty file)
>>> +++   
>>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/  
>>> InteropGBean.java    Sun Jan 23 23:33:10 2005
>>>
>>> +/**
>>> + * A GBean that provides an example interop
>>> + *
>>> + * @version $Rev: $ $Date: $
>>> + */
>>> +public class InteropGBean implements GBeanLifecycle {
>>
>>
>> Is this test code?  If it is an example we want to ship with the   
>> server, maybe we should put it in an example package or rename it   
>> ExampleInteropGBean.
>
>
> MD>> This GBean was the interop/corba container loader.  maybe a  
> better name is in order.

I think you lost me.  What is it?  The reason I thought is was test  
code was because it had an echo method that looks like test code I  
normally write.

> I haven't had a chance to flesh out the details for the Geronimo  
> integration.  I'd like to see the interop container be loaded by the  
> InteropGBean.  The InteropGBean could be a seperate plan or as part of  
> the j2ee-server plan.  For the J2EE 1.4 configuration, the interop  
> container would be enabled, but if a customer didn't care about the  
> interop container they could remove the plan from the server  
> configuration.

I personally would like to see less plans in Geronimo.  I personally  
find all the plans confusing, but others disagree with me :)

>>> +    /*
>>> +     * Return the skeleton implemention for the remote interface.    
>>> This interface has the
>>> +     * invoke method to handle the rmi/iiop messages.
>>> +     */
>>> +    public RemoteInterface getRemoteInterface() {
>>> +        if (_ri == null) {
>>> +            synchronized (this) {
>>> +                String riName = _remoteInterfaceName + "_Skeleton";
>>> +                _remoteInterfaceClass = loadClass(riName);
>>> +
>>> +                try {
>>> +                    _ri = (RemoteInterface)   
>>> _remoteInterfaceClass.newInstance();
>>> +                } catch (InstantiationException e) {
>>> +                    e.printStackTrace();  //To change body of catch  
>>>  statement use File | Settings | File Templates.
>>> +                } catch (IllegalAccessException e) {
>>> +                    e.printStackTrace();  //To change body of catch  
>>>  statement use File | Settings | File Templates.
>>> +                }
>>> +            }
>>> +        }
>>> +
>>> +        return _ri;
>>> +    }
>>
>>
>> This is double check locking.  The whole method needs to be   
>> synchronized.
>
> MD>> The double check code is wrong and I need to change it.    Does  
> the method require sync?  Maybe.  I recently came accross a strategy  
> for multiprocessor locking that uses a transient int as a memory  
> barrier.  Probably best if I don't attempt to explain it.  We can  
> optimize our locking.  The template that I came accross is as follows:
>
>    private volatile boolean _evaluated = false;
>
>    private Object _value;
>
>    /**
>     ** Sub-classes should override this method to get the object's  
> value
>     ** when it is first needed.
>     **/
>    public abstract Object evaluate();
>
>    public final Object getValue()
>    {
>        if (_volatileIsEffectiveMemoryBarrier)
>        {
>            if (! _evaluated)
>            {
>                synchronized (this)
>                {
>                    if (! _evaluated)
>                    {
>                        _value = evaluate();
>                        _evaluated = true;
>                    }
>                }
>            }
>        }
>        else
>        {
>            synchronized (this)
>            {
>                if (! _evaluated)
>                {
>                    _value = evaluate();
>                    _evaluated = true;
>                }
>            }
>        }
>        return _value;
>    }

Jeremy and I tried a structure like this when we were building locking  
code for geronimo.  The problem we found was evaluating a volatile is  
like 10 (maybe 100) times slower then entering a synchronized block.   
It seems that Sun has worked a ton on optimizing synchronized blocked  
and virtually no work on volatiles.  Regardless, I'd go for the easier  
to read simple synchronize code unless it's on the critical path (and  
then I'd do some testing :)

>> Again there is double check locking.  Also, if this is called in the   
>> critical path, I suggest we use CGLIB instead of reflection for   
>> construction as it is way faster.
>
> MD>>  I can look into CGLIB.

No rush on that one... it's for the polish phase.

>>> Added:   
>>> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/  
>>> client/InitialContextFactory.java
>>> Url:   
>>> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/ 
>>> java/ org/apache/geronimo/interop/client/InitialContextFactory.java?  
>>> view=auto&rev=126264
>>> ===================================================================== 
>>> == =======
>>> +public class InitialContextFactory
>>> +        implements javax.naming.spi.InitialContextFactory {
>>> +    private HashMap _startMap = new HashMap();
>>> +
>>> +    public Context getInitialContext(java.util.Hashtable env)  
>>> throws  NamingException {
>>> +        return   
>>> org.apache.geronimo.interop.rmi.iiop.client.ClientNamingContext.getIn 
>>> st ance(env);
>>> +    }
>>> +}
>>
>>
>> Is the _startMap variable used anywhere?
>
> MD>> I'd like to remove any naming in the interop and just use the  
> naming provided in Geronimo.

I just recently re-read the code in geronimo and the code is almost  
identical.  I think the only difference is the implementation in  
geronimo-naming supports compound name (java:comp/env) lookup from  
within a context.


Mime
View raw message