geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dain Sundstrom <dsundst...@gluecode.com>
Subject Review - svn commit: r126264
Date Mon, 24 Jan 2005 22:53:39 GMT
Mark,

Wow!  This is very impressive work. I spent some time reading over this  
(well the first half... the thing is huge).  Most of the comments, I  
have are just my curiosity and don't really need to be fixed.  Other  
are notes on differences between this code an geronimo, and I these  
cases I think we should change current geronimo or this new code so we  
don't have multiple ways to do the same thing (BTW, I generally don't  
have a preference on which code we change unless one is broken :)  Then  
there are the few coding convention things, and finally there are a few  
nit picky things :)

Thanks for the great work,

-dain


On Jan 23, 2005, at 11:33 PM, adc@apache.org wrote:

> Added: geronimo/trunk/modules/interop/src/idl/CosNaming.idl
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/idl/ 
> CosNaming.idl?view=auto&rev=126264
>
> Added: geronimo/trunk/modules/interop/src/idl/GIOP.idl
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/idl/ 
> GIOP.idl?view=auto&rev=126264
>
> Added: geronimo/trunk/modules/interop/src/idl/IIOP.idl
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/idl/ 
> IIOP.idl?view=auto&rev=126264
>
> Added: geronimo/trunk/modules/interop/src/idl/IOP.idl
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/idl/ 
> IOP.idl?view=auto&rev=126264
>
> Added:  
> geronimo/trunk/modules/interop/src/idl/org-apache-geronimo-interop- 
> rmi-iiop.idl
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/idl/ 
> org-apache-geronimo-interop-rmi-iiop.idl?view=auto&rev=126264

The idl files need Apache license headers... maybe in the comments that  
are copied into the generated code.

> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> CheckedException.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/CheckedException.java?view=auto&rev=126264
>
> +/**
> + * * A wrapper that allows checked exceptions to be thrown as  
> unchecked.
> + */
> +public class CheckedException extends RuntimeException {
> +    public CheckedException(Throwable cause) {
> +        super(cause);
> +    }
> +}

How is this used?  Normally in Geronimo we have our invocation chains  
(interceptor stacks) throw Throwable, and we just let exceptions flow  
up.  In other cases, we divide exceptions into System and Application.   
We let the System exceptions flow up and the Application exceptions are  
wrapped in the results object since they are treated as a normal return  
for ejbs.  Anyway, I'm just curious.  If it falls into one of the basic  
scenarios we already have a preferred solution for, I'd like to either  
switch the exiting code to follow the new pattern or switch the new  
iiop code to follow the current pattern.

> 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.

> 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.


> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> SystemException.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/SystemException.java?view=auto&rev=126264

Same comment as above for CheckedException.

> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> adapter/Adapter.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/adapter/Adapter.java?view=auto&rev=126264
> ======================================================================= 
> =======
> +public class Adapter {
> +    //
> +    // Public Accessible Properties
> +    //

I don't think we need comments like this... it is self explanatory.


> +    protected String _bindName;
> +    protected String _remoteClassName;
> +    protected String _remoteInterfaceName;
> +    protected Vector _idVector;
> +    protected boolean _shared;
> +    protected ClassLoader _cl;
> +    protected RemoteInterface _ri;

Our naming convention doesn't use lead underscores.  Also we tend not  
to use protected fields.  Maybe there is a subclass or some reason.

> +    //
> +    // Internal Properrties
> +    //
> +
> +    protected Object _sharedObject;
> +    protected HashMap _objects;
> +    protected Class _remoteClassClass;
> +    protected Class _remoteInterfaceClass;
> +
> +    public Adapter() {
> +        _objects = new HashMap();
> +        _idVector = new Vector();
> +    }
> +
> +    /*
> +     * BindName is the name that will be registered with the INS  
> (Inter-operable Name Service)
> +     */
> +    public String getBindName() {
> +        return _bindName;
> +    }
> +
> +    public void setBindName(String bindName) {
> +        _bindName = bindName;
> +    }
> +
> +    /*
> +     * Is this a shared component?  If so this will invoke the  
> getInstance method on
> +     * the component ...
> +     */
> +    public boolean isShared() {
> +        return _shared;
> +    }
> +
> +    public void setShared(boolean shared) {
> +        _shared = shared;
> +    }
> +
> +    /*
> +     * The classloader that will load any dependancies of the adapter  
> or corba skel interfaces.
> +     * Its should be set by the ejb container
> +     */
> +    public ClassLoader getClassLoader() {
> +        return _cl;
> +    }
> +
> +    public void setClassLoader(ClassLoader cl) {
> +        _cl = cl;
> +    }

Can we change these to be constructor injected and make the fields  
final?

> +    /*
> +     * This is the name of the remote class that implements the  
> remote interface.
> +     *
> +     * This is only used if this adapter is going to directly invoke  
> an object.  For the
> +     * EJB Container, the adapter will pass through the method  
> invocations.
> +     */
> +    public String getRemoteClassName() {
> +        return _remoteClassName;
> +    }
> +
> +    public void setRemoteClassName(String rcName) {
> +        _remoteClassName = rcName;
> +    }
> +
> +    /*
> +     * The remote interface name for the remote object.  This will  
> most likely be the name
> +     * of the EJB's RemoteInterface and RemoteHomeInterface
> +     *
> +     * The stub/skel generator will use this interface name.
> +     */
> +    public String getRemoteInterfaceName() {
> +        return _remoteInterfaceName;
> +    }
> +
> +    public void setRemoteInterfaceName(String riName) {
> +        _remoteInterfaceName = riName;
> +    }
> +
> +    /*
> +     * A list of public IDs that the remote object implements:
> +     *
> +     * IDL:....:1.0
> +     * RMI:....:X:Y
> +     */
> +    public Vector getIds() {
> +        return _idVector;
> +    }
> +
> +    public void addId(String id) {
> +        _idVector.add(id);
> +    }
> +
> +    public void removeId(String id) {
> +        _idVector.remove(id);
> +    }
> +
> +    /*
> +     * 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.

> +    /*
> +     * Get an object instance to invoke based on the object key.
> +     *
> +     * The objectKey could probably be passed to the EJB container so  
> that the
> +     * container can directly invoke the ejb object as required.
> +     */
> +    public Object getInstance(byte[] objectKey) {
> +        String key = new String(objectKey);
> +        return getInstance(key);
> +    }
> +
> +    public Object getInstance(String key) {
> +        Object o = _objects.get(key);
> +
> +        if (o == null) {
> +            o = newInstance(key);
> +        }
> +
> +        return o;
> +    }
> +
> +    public Object newInstance(byte[] objectKey) {
> +        String key = new String(objectKey);
> +        return newInstance(key);
> +    }
> +
> +    public Object newInstance(String key) {
> +        Object o = null;
> +
> +        if (_remoteClassClass == null) {
> +            synchronized (this) {
> +                _remoteClassClass = loadClass(_remoteClassName);
> +            }
> +
> +            try {
> +                if (_shared) {
> +                    synchronized (this) {
> +                        Method m =  
> _remoteClassClass.getMethod("getInstance", (Class[]) null);
> +                        o = m.invoke(_remoteClassClass, (Object[])  
> null);
> +
> +                        if (o != null) {
> +                            _objects.put(key, o);
> +                        }
> +                    }
> +                } else {
> +                    o = _remoteClassClass.newInstance();
> +                    _objects.put(key, o);
> +                }
> +            } 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.
> +            } catch (NoSuchMethodException e) {
> +                e.printStackTrace();
> +            } catch (InvocationTargetException e) {
> +                e.printStackTrace();
> +            }
> +        }
> +
> +        return o;
> +    }

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.

> +    /*
> +     * Invoke method from the IIOP Message Handler.  The adapter is  
> bound to the INS name service.
> +     * When an RMI/IIOP message is processed by the server, the  
> message handler will perform a lookup
> +     * on the name service to get the Adapter, then the invocation  
> will be passed to the adapter
> +     * The adapter will obtain the object key and then determine  
> which object instance to pass the
> +     * invocation to.
> +     */

This comment is missing the double star at the top to signify javadoc.

> +    public void invoke(java.lang.String methodName, byte[] objectKey,  
> org.apache.geronimo.interop.rmi.iiop.ObjectInputStream input,  
> org.apache.geronimo.interop.rmi.iiop.ObjectOutputStream output) {
> +        RemoteInterface skeleton = getRemoteInterface();
> +        Object instance = getInstance(objectKey);
> +
> +        if (instance != null) {
> +            skeleton.$invoke(methodName, objectKey, instance, input,  
> output);
> +        } else {
> +            throw new org.omg.CORBA.OBJECT_NOT_EXIST(new  
> String(objectKey));
> +        }
> +    }
> +
> +    /*
> +     * Helper function to load a class.  This uses classloader for  
> the adapter.
> +     */
> +    protected Class loadClass(String name) {
> +        Class c = null;
> +
> +        try {
> +            c = _cl.loadClass(name);
> +        } catch (Exception e) {
> +            e.printStackTrace();
> +        }
> +
> +        return c;
> +    }
> +}

I think we should be at least logging the error or actually letting the  
ClassNotFoundException through.  We also have a ClassLoading class  
which you may want to use as it handles some of the more weird class  
name formats.

> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> adapter/AdapterManager.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/adapter/AdapterManager.java? 
> view=auto&rev=126264
> ======================================================================= 
> =======
> --- (empty file)
> +++  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> adapter/AdapterManager.java	Sun Jan 23 23:33:10 2005
> @@ -0,0 +1,43 @@
> +/**
> + *
> + *  Copyright 2004-2005 The Apache Software Foundation
> + *
> + *  Licensed under the Apache License, Version 2.0 (the "License");
> + *  you may not use this file except in compliance with the License.
> + *  You may obtain a copy of the License at
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + *  Unless required by applicable law or agreed to in writing,  
> software
> + *  distributed under the License is distributed on an "AS IS" BASIS,
> + *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or  
> implied.
> + *
> + *  See the License for the specific language governing permissions  
> and
> + *  limitations under the License.
> + */
> +package org.apache.geronimo.interop.adapter;
> +
> +import java.util.Hashtable;
> +
> +
> +public class AdapterManager {
> +    protected Hashtable _adapters;
> +    protected static AdapterManager _me = new AdapterManager();

Underscores and protected...

> +    protected AdapterManager() {
> +        _adapters = new Hashtable();
> +    }
> +
> +    public static AdapterManager getInstance() {
> +        return _me;
> +    }

This is a bad idea.  Instead we should have a manger gbean and the  
singletonness would be handled by there being only one GBean instance  
registered.

> +    public void registerAdapter(Adapter a) {
> +
> +        _adapters.put(a.getBindName(), a);
> +    }
> +
> +    public Adapter getAdapter(String objectName) {
> +        return (Adapter) _adapters.get(objectName);
> +    }

I prefer HashMap and synchronizing the methods that access the Map, but  
this works in this case.

> 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.getInst 
> ance(env);
> +    }
> +}

Is the _startMap variable used anywhere?

> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> client/InitialContextFactoryBuilder.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/client/InitialContextFactoryBuilder.java? 
> view=auto&rev=126264
> ======================================================================= 
> =======
> +public class InitialContextFactoryBuilder
> +        implements javax.naming.spi.InitialContextFactoryBuilder {
> +    public javax.naming.spi.InitialContextFactory  
> createInitialContextFactory(Hashtable env) {
> +        return new InitialContextFactory();
> +    }
> +}

What is this for?  Just curious...

> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> generator/GenException.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/generator/GenException.java? 
> view=auto&rev=126264
> ======================================================================= 
> =======
> +/**
> + * User: Mark
> + * Date: Dec 21, 2004
> + * Time: 3:49:45 PM
> + */

Oops :)

> +public class GenException
> +        extends Exception {
> +    public GenException() {
> +        super();
> +    }
> +
> +    public GenException(String message) {
> +        super(message);
> +    }
> +
> +    public GenException(Throwable cause) {
> +        super(cause);
> +    }
> +
> +    public GenException(String message, Throwable cause) {
> +        super(message, cause);
> +    }
> +}
>
> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> generator/GenOptions.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/generator/GenOptions.java? 
> view=auto&rev=126264
> ======================================================================= 
> =======
> +public class GenOptions {
> +    protected String _genDir = "./";
> +    protected boolean _overwrite = false;
> +    protected boolean _verbose = false;
> +
> +    public GenOptions() {
> +    }
> +
> +    public GenOptions(String genDir, boolean overwrite, boolean  
> verbose) {
> +        _genDir = genDir;
> +        _overwrite = overwrite;
> +        _verbose = verbose;
> +    }
> +
> +    public String getGenDir() {
> +        return _genDir;
> +    }
> +
> +    public void setGenDir(String genDir) {
> +        _genDir = genDir;
> +    }
> +
> +    public boolean isOverwrite() {
> +        return _overwrite;
> +    }
> +
> +    public void setOverwrite(boolean overwrite) {
> +        _overwrite = overwrite;
> +    }
> +
> +    public boolean isVerbose() {
> +        return _verbose;
> +    }
> +
> +    public void setVerbose(boolean verbose) {
> +        _verbose = verbose;
> +    }
> +
> +}

Do we need both the constructor args and the setters?  If I set a  
variable is whatever is using this class going to adapt to the recently  
changed variable?

> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> naming/NameService.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/naming/NameService.java? 
> view=auto&rev=126264
> ======================================================================= 
> =======
> --- (empty file)
> +++  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> naming/NameService.java	Sun Jan 23 23:33:10 2005
> @@ -0,0 +1,66 @@
> +/**
> + *
> + *  Copyright 2004-2005 The Apache Software Foundation
> + *
> + *  Licensed under the Apache License, Version 2.0 (the "License");
> + *  you may not use this file except in compliance with the License.
> + *  You may obtain a copy of the License at
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + *  Unless required by applicable law or agreed to in writing,  
> software
> + *  distributed under the License is distributed on an "AS IS" BASIS,
> + *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or  
> implied.
> + *
> + *  See the License for the specific language governing permissions  
> and
> + *  limitations under the License.
> + */
> +package org.apache.geronimo.interop.naming;
> +
> +import java.util.HashMap;
> +import javax.naming.NamingException;
> +
> +import org.apache.geronimo.interop.adapter.Adapter;
> +
> +
> +public class NameService {
> +    protected static NameService _ns = null;

protected and underscore

> +    public static NameService getInstance() {
> +        if (_ns == null) {
> +            synchronized (NameService.class) {
> +                if (_ns == null) {
> +                    _ns = new NameService();
> +                    _ns.init();
> +                }
> +            }
> +        }
> +
> +        return _ns;
> +    }

Double check locking

> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> naming/NamingContext.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/naming/NamingContext.java? 
> view=auto&rev=126264
> ======================================================================= 
> =======
> --- (empty file)
> +++  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> naming/NamingContext.java	Sun Jan 23 23:33:10 2005
> @@ -0,0 +1,150 @@
> +/**
> + *
> + *  Copyright 2004-2005 The Apache Software Foundation
> + *
> + *  Licensed under the Apache License, Version 2.0 (the "License");
> + *  you may not use this file except in compliance with the License.
> + *  You may obtain a copy of the License at
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + *  Unless required by applicable law or agreed to in writing,  
> software
> + *  distributed under the License is distributed on an "AS IS" BASIS,
> + *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or  
> implied.
> + *
> + *  See the License for the specific language governing permissions  
> and
> + *  limitations under the License.
> + */
> +package org.apache.geronimo.interop.naming;
> +
> +import java.util.HashMap;
> +import javax.naming.NameNotFoundException;
> +import javax.naming.NamingException;
> +
> +import org.apache.geronimo.interop.adapter.Adapter;
> +
> +
> +public class NamingContext {
> +    public static final NamingContext getInstance(Class baseClass) {
> +        NamingContext context;
> +        synchronized (_contextMap) {
> +            context = (NamingContext) _contextMap.get(baseClass);
> +            if (context == null) {
> +                context = new NamingContext();
> +                _contextMap.put(baseClass, context);
> +                context.init(baseClass);
> +            }
> +        }
> +        return context;
> +    }
> +
> +    private static ThreadLocal _current = new ThreadLocal();

Should this be an InheritableThreadLocal?

> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> properties/BooleanProperty.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/properties/BooleanProperty.java? 
> view=auto&rev=126264
>
> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> properties/ByteProperty.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/properties/ByteProperty.java? 
> view=auto&rev=126264
>
> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> properties/DoubleProperty.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/properties/DoubleProperty.java? 
> view=auto&rev=126264
>
> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> properties/FloatProperty.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/properties/FloatProperty.java? 
> view=auto&rev=126264
>
> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> properties/IntProperty.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/properties/IntProperty.java? 
> view=auto&rev=126264
>
> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> properties/LongProperty.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/properties/LongProperty.java? 
> view=auto&rev=126264
>
> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> properties/ShortProperty.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/properties/ShortProperty.java? 
> view=auto&rev=126264
>
> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> properties/StringProperty.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/properties/StringProperty.java? 
> view=auto&rev=126264

How are these used?

> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> properties/SystemProperties.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/properties/SystemProperties.java? 
> view=auto&rev=126264

Is this a bridge to java.lang.System properties?  If so, do we really  
want to support system properties?

> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> repository/Repository.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/repository/Repository.java? 
> view=auto&rev=126264
> ======================================================================= 
> =======
> +public class Repository {
> +    // ??
> +}

Say what? :)

> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> rmi/iiop/IiopVersion.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/rmi/iiop/IiopVersion.java? 
> view=auto&rev=126264
> ======================================================================= 
> =======
> +public class IiopVersion {
> +}

What's this for?

> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> rmi/iiop/ListenerInfo.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/rmi/iiop/ListenerInfo.java? 
> view=auto&rev=126264
> +public class ListenerInfo {
> +    public int protocol;
> +
> +    public String host;
> +
> +    public int port;
> +}

Do these have to be public fields?  Normally we use, private fields  
with getters and setters.

> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> rmi/iiop/NameServiceOperations_Skeleton.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/rmi/iiop/ 
> NameServiceOperations_Skeleton.java?view=auto&rev=126264

Do we want the Skeletons checked in?

> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> rmi/iiop/ObjectKey.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/rmi/iiop/ObjectKey.java? 
> view=auto&rev=126264
> ======================================================================= 
> =======
> +public class ObjectKey {
> +    public static final int TYPE_MANAGER = 'M';
> +
> +    public static final int TYPE_SESSION = 'S';
> +
> +    public int type;
> +    public String username = "";
> +    public String password = "";
> +    public String component = "";
> +    public String sessionID = "";

public fields?

> +    public void checkPassword() {
> +        User.getInstance(username).login(password);
> +    }

Shouldn't this go though Geronimo security?

> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> rmi/iiop/ObjectRef.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/rmi/iiop/ObjectRef.java? 
> view=auto&rev=126264
> ======================================================================= 
> =======
> +    public int $getIiopVersion() {
> +        return _iiopVersion;
> +    }

Why do all the methods in this class start with a dollar sign?

> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> rmi/iiop/client/ClientNamingContext.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/rmi/iiop/client/ClientNamingContext.java? 
> view=auto&rev=126264

We should consider merging this with the Geronimo naming stuff... not a  
task for today, but (much) later on :)

> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> rmi/iiop/compiler/StubCompiler.txt
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/rmi/iiop/compiler/StubCompiler.txt? 
> view=auto&rev=126264

Why is this a text file?

> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> rmi/iiop/server/SocketListener.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/rmi/iiop/server/SocketListener.java? 
> view=auto&rev=126264
> ======================================================================= 
> =======
> +public class SocketListener extends Thread {
> +    public static SocketListener getInstance() {
> +        return new SocketListener();
> +    }

Why is there a static factory?

> +    // private data
> +
> +    private String _name;
> +
> +    private String _host;
> +
> +    private int _port;
> +
> +    private int _listenBacklog;
> +
> +    private java.net.ServerSocket _listener;
> +
> +    // internal methods
> +
> +    protected void init() {
> +        _host = "localhost";
> +        _port = 2000;
> +        _listenBacklog = 10;
> +        setDaemon(true);
> +    }
> +
> +    // public methods
> +
> +    public void setHost(String host) {
> +        _host = host;
> +    }
> +
> +    public void setPort(int port) {
> +        _port = port;
> +    }
> +
> +    public void setListenBacklog(int backlog) {
> +        _listenBacklog = backlog;
> +    }
> +
> +    public void run() {
> +        String iiopURL = "iiop://" + _host + ":" + _port;
> +        ListenerInfo listenerInfo = new ListenerInfo();
> +        listenerInfo.protocol = Protocol.IIOP; // TODO: other  
> protocols (IIOPS etc.)
> +        listenerInfo.host = _host;
> +        listenerInfo.port = _port;
> +        try {
> +            InetAddress addr = InetAddress.getByName(_host);
> +            _listener = new java.net.ServerSocket(_port,  
> _listenBacklog, addr);
> +        } catch (Exception ex) {
> +            System.out.println("SocketListener: Error creating server  
> socket.");
> +            ex.printStackTrace();
> +            try {
> +                Socket socket = new Socket(_host, _port);
> +                socket.close();
> +                System.out.println("SocketListener: Error server  
> already running: " + iiopURL);
> +                ex.printStackTrace();
> +            } catch (Exception ignore) {
> +            }
> +            return;
> +        }
> +        new CheckConnect().start();
> +        for (; ;) {
> +            java.net.Socket socket;
> +            try {
> +                socket = _listener.accept();
> +            } catch (Exception ex) {
> +                throw new SystemException(ex); // TODO: log error  
> message
> +            }
> +            MessageHandler.getInstance(listenerInfo, socket).start();
> +        }
> +    }
> +
> +    private class CheckConnect extends Thread {
> +        public void run() {
> +            try {
> +                Socket socket = new Socket(_host, _port);
> +                socket.close();
> +                if (!SystemProperties.quiet()) {
> +                     
> System.out.println(formatAcceptingIiopConnections());
> +                }
> +            } catch (Exception ex) {
> +                warnConnectFailed(_host, _port);
> +            }
> +        }
> +    }
> +
> +    // format methods
> +
> +    protected String formatAcceptingIiopConnections() {
> +        String msg = "SocketListener.formatAcceptingIiopConnection():  
> ";
> +        return msg;
> +    }
> +
> +    // log methods
> +
> +    protected void warnConnectFailed(String host, int port) {
> +        System.out.println("SocketListener.warnConnectFailed(): host  
> = " + host + ", port = " + port);
> +    }
> +}

I suggest, you either require all state data such as host and port be  
passed in via the constructor, or make the setters throw an "already  
running" exception.  Otherwise people mistakenly think the can change  
the port.

> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> security/Role.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/security/Role.java?view=auto&rev=126264
>
> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> security/SimpleSubject.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/security/SimpleSubject.java? 
> view=auto&rev=126264
>
> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> security/User.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/security/User.java?view=auto&rev=126264

What is the plan for these with regards to the Geronimo security module?

> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> util/SystemUtil.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/util/SystemUtil.java?view=auto&rev=126264
> ======================================================================= 
> =======
> +public class SystemUtil {
> +    // properties
> +
> +    public static final StringProperty vmVersionProperty =
> +            new StringProperty(SystemProperties.class,  
> "java.vm.version");
> +
> +    // private data
> +
> +    private static String _vmVersion = vmVersionProperty.getString();
> +
> +    private static boolean _isJDK13 = _vmVersion.startsWith("1.3")
> +                                      ||  
> _vmVersion.startsWith("CrE-ME V4.00");
> +
> +    private static boolean _isJDK14 = _vmVersion.startsWith("1.4");
> +
> +    // public methods
> +
> +    public static String getExecutableSuffix() {
> +        return isWindows() ? ".exe" : "";
> +    }
> +
> +    public static String getShellScriptSuffix() {
> +        return isWindows() ? ".bat" : ".sh";
> +    }
> +
> +    public static boolean isJDK13() {
> +        return _isJDK13;
> +    }
> +
> +    public static boolean isJDK14() {
> +        return _isJDK14;
> +    }
> +
> +    public static boolean isWindows() {
> +        return java.io.File.separatorChar == '\\';
> +    }
> +}

How is this used?

> Added:  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> util/ThreadContext.java
> Url:  
> http://svn.apache.org/viewcvs/geronimo/trunk/modules/interop/src/java/ 
> org/apache/geronimo/interop/util/ThreadContext.java? 
> view=auto&rev=126264
> ======================================================================= 
> =======
> --- (empty file)
> +++  
> geronimo/trunk/modules/interop/src/java/org/apache/geronimo/interop/ 
> util/ThreadContext.java	Sun Jan 23 23:33:10 2005
> @@ -0,0 +1,135 @@
> +/**
> + *
> + *  Copyright 2004-2005 The Apache Software Foundation
> + *
> + *  Licensed under the Apache License, Version 2.0 (the "License");
> + *  you may not use this file except in compliance with the License.
> + *  You may obtain a copy of the License at
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + *  Unless required by applicable law or agreed to in writing,  
> software
> + *  distributed under the License is distributed on an "AS IS" BASIS,
> + *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or  
> implied.
> + *
> + *  See the License for the specific language governing permissions  
> and
> + *  limitations under the License.
> + */
> +package org.apache.geronimo.interop.util;
> +
> +import java.util.HashMap;
> +
> +import org.apache.geronimo.interop.SystemException;
> +
> +
> +public abstract class ThreadContext {
> +    private static HashMap _primTypes;
> +
> +    private static ThreadLocal _defaultRmiHost = new ThreadLocal();
> +
> +    private static ThreadLocal _defaultRmiPort = new ThreadLocal();
> +
> +    static {
> +        _primTypes = new HashMap();
> +        _primTypes.put("boolean", boolean.class);
> +        _primTypes.put("char", char.class);
> +        _primTypes.put("byte", byte.class);
> +        _primTypes.put("short", short.class);
> +        _primTypes.put("int", int.class);
> +        _primTypes.put("long", long.class);
> +        _primTypes.put("float", float.class);
> +        _primTypes.put("double", double.class);
> +        _primTypes.put("boolean[]", boolean[].class);
> +        _primTypes.put("char[]", char[].class);
> +        _primTypes.put("byte[]", byte[].class);
> +        _primTypes.put("short[]", short[].class);
> +        _primTypes.put("int[]", int[].class);
> +        _primTypes.put("long[]", long[].class);
> +        _primTypes.put("float[]", float[].class);
> +        _primTypes.put("double[]", double[].class);
> +    }
> +
> +    public static String getDefaultRmiHost() {
> +        String host = (String) _defaultRmiHost.get();
> +        if (host == null) {
> +            host = "0";
> +        }
> +        return host;
> +    }
> +
> +    public static int getDefaultRmiPort() {
> +        Integer port = (Integer) _defaultRmiPort.get();
> +        if (port == null) {
> +            port = IntegerCache.get(0);
> +        }
> +        return port.intValue();
> +    }
> +
> +    public static Class loadClass(String className) {
> +        Class t = (Class) _primTypes.get(className);
> +        if (t != null) {
> +            return t;
> +        }
> +        try {
> +            ClassLoader loader =  
> Thread.currentThread().getContextClassLoader();
> +            if (loader == null) {
> +                return Class.forName(className);
> +            } else {
> +                return loader.loadClass(className);
> +            }
> +        } catch (RuntimeException ex) {
> +            throw (RuntimeException) ex;
> +        } catch (Exception ex) {
> +            throw new SystemException(ex);
> +        }
> +    }

This is unlikely to work in Geronimo as we almost never set context  
class loader.  Normally we try to explicitly pass around the  
classloader.

> +
> +    public static Class loadClass(String className, Class  
> parentClass) {
> +        if (parentClass == null) {
> +            return loadClass(className);
> +        }
> +        Class t = (Class) _primTypes.get(className);
> +        if (t != null) {
> +            return t;
> +        }
> +        try {
> +            ClassLoader loader = parentClass.getClassLoader();
> +            if (loader == null) {
> +                return loadClass(className);
> +            } else {
> +                return loader.loadClass(className);
> +            }
> +        } catch (RuntimeException ex) {
> +            throw (RuntimeException) ex;
> +        } catch (Exception ex) {
> +            throw new SystemException(ex);
> +        }
> +    }
> +
> +    public static Class loadClassOrReturnNullIfNotFound(String  
> className) {
> +        try {
> +            return loadClass(className);
> +        } catch (RuntimeException ex) {
> +            return null;
> +        }
> +    }
> +
> +    public static Class loadClassOrReturnNullIfNotFound(String  
> className, Class parentClass) {
> +        if (parentClass == null) {
> +            return loadClassOrReturnNullIfNotFound(className);
> +        }
> +        try {
> +            return loadClass(className, parentClass);
> +        } catch (RuntimeException ex) {
> +            return null;
> +        }
> +    }

Most of this class loader stuff is handled by the class ClassLoading  
which handles some of the more weird stuff.


Mime
View raw message