geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gianny Damour <gianny.dam...@optusnet.com.au>
Subject Re: svn commit: r485321 - in /geronimo/server/trunk/modules: geronimo-kernel/src/main/java/org/apache/geronimo/gbean/ geronimo-kernel/src/main/java/org/apache/geronimo/gbean/runtime/ geronimo-kernel/src/test/java/org/apache/geronimo/gbean/ geronimo-kernel/...
Date Mon, 11 Dec 2006 09:45:15 GMT
Hi,

I am quickly scanning this commit and I would like to know if it was  
not a little bit less intrusive to keep the existing addOperation and  
search for the return type of the added operations against the target  
gbeanType. This way, developers do not need to specify the return  
type of the operations (also, the migration of the existing GBeanInfo  
could have been avoided).

After reading GERONIMO-2607, it seems that the goal of the change was  
to have a return type defined within JConsole for the GBean  
operations. It seems that patching JMXUtil.toMBeanInfo would have  
been another implementation approach: while getting the exposed  
operations, the GBean class could be searched for returned types. One  
of the advantages would have been to keep backward compatibility.

Thanks,
Gianny

On 11/12/2006, at 11:14 AM, akulshreshtha@apache.org wrote:

> Author: akulshreshtha
> Date: Sun Dec 10 16:14:46 2006
> New Revision: 485321
>
> URL: http://svn.apache.org/viewvc?view=rev&rev=485321
> Log:
> GERONIMO-2607 Added returnType to GOperationInfo, This modifies  
> GBeanInfoBuilder and breaks backward compatibility
>
> Modified:
>     geronimo/server/trunk/modules/geronimo-kernel/src/main/java/org/ 
> apache/geronimo/gbean/DynamicGOperationInfo.java
>     geronimo/server/trunk/modules/geronimo-kernel/src/main/java/org/ 
> apache/geronimo/gbean/GBeanInfoBuilder.java
>     geronimo/server/trunk/modules/geronimo-kernel/src/main/java/org/ 
> apache/geronimo/gbean/GOperationInfo.java
>     geronimo/server/trunk/modules/geronimo-kernel/src/main/java/org/ 
> apache/geronimo/gbean/runtime/GBeanOperation.java
>     geronimo/server/trunk/modules/geronimo-kernel/src/test/java/org/ 
> apache/geronimo/gbean/GBeanInfoTest.java
>     geronimo/server/trunk/modules/geronimo-kernel/src/test/java/org/ 
> apache/geronimo/kernel/MockGBean.java
>     geronimo/server/trunk/modules/geronimo-kernel/src/test/java/org/ 
> apache/geronimo/kernel/config/MyGBean.java
>     geronimo/server/trunk/modules/geronimo-system/src/main/java/org/ 
> apache/geronimo/system/jmx/JMXUtil.java
>     geronimo/server/trunk/modules/geronimo-system/src/main/java/org/ 
> apache/geronimo/system/logging/log4j/Log4jService.java
>
> Modified: geronimo/server/trunk/modules/geronimo-kernel/src/main/ 
> java/org/apache/geronimo/gbean/DynamicGOperationInfo.java
> URL: http://svn.apache.org/viewvc/geronimo/server/trunk/modules/ 
> geronimo-kernel/src/main/java/org/apache/geronimo/gbean/ 
> DynamicGOperationInfo.java?view=diff&rev=485321&r1=485320&r2=485321
> ====================================================================== 
> ========
> --- geronimo/server/trunk/modules/geronimo-kernel/src/main/java/org/ 
> apache/geronimo/gbean/DynamicGOperationInfo.java (original)
> +++ geronimo/server/trunk/modules/geronimo-kernel/src/main/java/org/ 
> apache/geronimo/gbean/DynamicGOperationInfo.java Sun Dec 10  
> 16:14:46 2006
> @@ -24,14 +24,14 @@
>   */
>  public class DynamicGOperationInfo extends GOperationInfo {
>      public DynamicGOperationInfo(String name) {
> -        super(name);
> +        super(name, "java.lang.Object");
>      }
>
>      public DynamicGOperationInfo(String name, String[] paramTypes) {
> -        super(name, paramTypes);
> +        super(name, paramTypes, "java.lang.Object");
>      }
>
>      public DynamicGOperationInfo(String name, List parameters) {
> -        super(name, parameters);
> +        super(name, parameters, "java.lang.Object");
>      }
>  }
>
> Modified: geronimo/server/trunk/modules/geronimo-kernel/src/main/ 
> java/org/apache/geronimo/gbean/GBeanInfoBuilder.java
> URL: http://svn.apache.org/viewvc/geronimo/server/trunk/modules/ 
> geronimo-kernel/src/main/java/org/apache/geronimo/gbean/ 
> GBeanInfoBuilder.java?view=diff&rev=485321&r1=485320&r2=485321
> ====================================================================== 
> ========
> --- geronimo/server/trunk/modules/geronimo-kernel/src/main/java/org/ 
> apache/geronimo/gbean/GBeanInfoBuilder.java (original)
> +++ geronimo/server/trunk/modules/geronimo-kernel/src/main/java/org/ 
> apache/geronimo/gbean/GBeanInfoBuilder.java Sun Dec 10 16:14:46 2006
> @@ -187,8 +187,7 @@
>
>              for (Iterator i = source.getOperations().iterator();  
> i.hasNext();) {
>                  GOperationInfo operationInfo = (GOperationInfo)  
> i.next();
> -                operations.put(new GOperationSignature 
> (operationInfo.getName(),
> -                        operationInfo.getParameterList()),  
> operationInfo);
> +                operations.put(new GOperationSignature 
> (operationInfo.getName(), operationInfo.getParameterList()),  
> operationInfo);
>              }
>
>              for (Iterator iterator = source.getReferences 
> ().iterator(); iterator.hasNext();) {
> @@ -346,7 +345,7 @@
>                                      method.getName()));
>                  }
>              } else {
> -                addOperation(new GOperationInfo(method.getName(),  
> method.getParameterTypes()));
> +                addOperation(new GOperationInfo(method.getName(),  
> method.getParameterTypes(), method.getReturnType().getName()));
>              }
>          }
>          addInterface(interfaces, intf);
> @@ -401,13 +400,27 @@
>      public void addOperation(GOperationInfo operationInfo) {
>          operations.put(new GOperationSignature 
> (operationInfo.getName(), operationInfo.getParameterList()),  
> operationInfo);
>      }
> -
> +
> +    /**
> +     * @deprecated
> +     */
>      public void addOperation(String name) {
> -        addOperation(new GOperationInfo(name, NO_ARGS));
> +        addOperation(new GOperationInfo(name, NO_ARGS, ""));
>      }
>
> +    /**
> +     * @deprecated
> +     */
>      public void addOperation(String name, Class[] paramTypes) {
> -        addOperation(new GOperationInfo(name, paramTypes));
> +        addOperation(new GOperationInfo(name, paramTypes, ""));
> +    }
> +
> +    public void addOperation(String name, String returnType) {
> +        addOperation(new GOperationInfo(name, NO_ARGS, returnType));
> +    }
> +
> +    public void addOperation(String name, Class[] paramTypes,  
> String returnType) {
> +        addOperation(new GOperationInfo(name, paramTypes,  
> returnType));
>      }
>
>      public void addReference(GReferenceInfo info) {
>
> Modified: geronimo/server/trunk/modules/geronimo-kernel/src/main/ 
> java/org/apache/geronimo/gbean/GOperationInfo.java
> URL: http://svn.apache.org/viewvc/geronimo/server/trunk/modules/ 
> geronimo-kernel/src/main/java/org/apache/geronimo/gbean/ 
> GOperationInfo.java?view=diff&rev=485321&r1=485320&r2=485321
> ====================================================================== 
> ========
> --- geronimo/server/trunk/modules/geronimo-kernel/src/main/java/org/ 
> apache/geronimo/gbean/GOperationInfo.java (original)
> +++ geronimo/server/trunk/modules/geronimo-kernel/src/main/java/org/ 
> apache/geronimo/gbean/GOperationInfo.java Sun Dec 10 16:14:46 2006
> @@ -33,7 +33,12 @@
>       * The name of this method.
>       */
>      private final String name;
> -
> +
> +    /**
> +     * The return type of this method.
> +     */
> +    private final String type;
> +
>      /**
>       * Parameters of this method.
>       */
> @@ -44,12 +49,13 @@
>       */
>      private final String methodName;
>
> -    public GOperationInfo(String name) {
> -        this(name, name, Collections.EMPTY_LIST);
> +    public GOperationInfo(String name, String type) {
> +        this(name, name, Collections.EMPTY_LIST, type);
>      }
>
> -    public GOperationInfo(String name, Class[] paramTypes) {
> +    public GOperationInfo(String name, Class[] paramTypes, String  
> type) {
>          this.name = this.methodName = name;
> +        this.type = type;
>          String[] args = new String[paramTypes.length];
>          for (int i = 0; i < args.length; i++) {
>              args[i] = paramTypes[i].getName();
> @@ -57,16 +63,17 @@
>          this.parameters = Collections.unmodifiableList 
> (Arrays.asList(args));
>      }
>
> -    public GOperationInfo(String name, String[] paramTypes) {
> -        this(name, name, Arrays.asList(paramTypes));
> +    public GOperationInfo(String name, String[] paramTypes, String  
> type) {
> +        this(name, name, Arrays.asList(paramTypes), type);
>      }
> -
> -    public GOperationInfo(String name, List parameters) {
> -        this(name, name, parameters);
> +
> +    public GOperationInfo(String name, List parameters, String  
> type) {
> +        this(name, name, parameters, type);
>      }
> -
> -    public GOperationInfo(String name, String methodName, List  
> parameters) {
> +
> +    public GOperationInfo(String name, String methodName, List  
> parameters, String type) {
>          this.name = name;
> +        this.type = type;
>          this.methodName = methodName;
>          this.parameters = Collections.unmodifiableList(new  
> ArrayList(parameters));
>      }
> @@ -74,6 +81,10 @@
>      public String getName() {
>          return name;
>      }
> +
> +    public String getReturnType() {
> +        return type;
> +    }
>
>      public String getMethodName() {
>          return methodName;
> @@ -84,6 +95,6 @@
>      }
>
>      public String toString() {
> -        return "[GOperationInfo: name=" + name + " parameters=" +  
> parameters + "]";
> +        return "[GOperationInfo: name=" + name + " parameters=" +  
> parameters + " type =" + type + "]";
>      }
>  }
>
> Modified: geronimo/server/trunk/modules/geronimo-kernel/src/main/ 
> java/org/apache/geronimo/gbean/runtime/GBeanOperation.java
> URL: http://svn.apache.org/viewvc/geronimo/server/trunk/modules/ 
> geronimo-kernel/src/main/java/org/apache/geronimo/gbean/runtime/ 
> GBeanOperation.java?view=diff&rev=485321&r1=485320&r2=485321
> ====================================================================== 
> ========
> --- geronimo/server/trunk/modules/geronimo-kernel/src/main/java/org/ 
> apache/geronimo/gbean/runtime/GBeanOperation.java (original)
> +++ geronimo/server/trunk/modules/geronimo-kernel/src/main/java/org/ 
> apache/geronimo/gbean/runtime/GBeanOperation.java Sun Dec 10  
> 16:14:46 2006
> @@ -40,6 +40,7 @@
>      private final boolean framework;
>      private final GOperationInfo operationInfo;
>
> +    // TODO - deprecate this and add returnType
>      static GBeanOperation createFrameworkOperation(GBeanInstance  
> gbeanInstance, String name, List parameterTypes, MethodInvoker  
> methodInvoker) {
>          return new GBeanOperation(gbeanInstance, name,  
> parameterTypes, methodInvoker);
>      }
> @@ -50,7 +51,7 @@
>          this.name = name;
>          this.parameterTypes = Collections.unmodifiableList(new  
> ArrayList(parameterTypes));
>          this.methodInvoker = methodInvoker;
> -        this.operationInfo = new GOperationInfo(this.name,  
> this.parameterTypes);
> +        this.operationInfo = new GOperationInfo(this.name,  
> this.parameterTypes, "java.lang.Object");
>      }
>
>      public GBeanOperation(GBeanInstance gbeanInstance,  
> GOperationInfo operationInfo) throws InvalidConfigurationException {
> @@ -97,6 +98,7 @@
>                  throw new InvalidConfigurationException("Target  
> does not have specified method (declared in a GBeanInfo operation):" +
>                          " name=" + operationInfo.getName() +
>                          " methodName=" +  
> operationInfo.getMethodName() +
> +                        " returnType=" +  
> operationInfo.getReturnType() +
>                          " targetClass=" + gbeanInstance.getType 
> ().getName());
>              }
>          }
>
> Modified: geronimo/server/trunk/modules/geronimo-kernel/src/test/ 
> java/org/apache/geronimo/gbean/GBeanInfoTest.java
> URL: http://svn.apache.org/viewvc/geronimo/server/trunk/modules/ 
> geronimo-kernel/src/test/java/org/apache/geronimo/gbean/ 
> GBeanInfoTest.java?view=diff&rev=485321&r1=485320&r2=485321
> ====================================================================== 
> ========
> --- geronimo/server/trunk/modules/geronimo-kernel/src/test/java/org/ 
> apache/geronimo/gbean/GBeanInfoTest.java (original)
> +++ geronimo/server/trunk/modules/geronimo-kernel/src/test/java/org/ 
> apache/geronimo/gbean/GBeanInfoTest.java Sun Dec 10 16:14:46 2006
> @@ -101,7 +101,7 @@
>          assertEquals(gbeanInfo.toString(), MockGBean.getGBeanInfo 
> ().toString());
>      }
>
> -    public void testBackwardCompatibility() throws Exception {
> +    public void xtestBackwardCompatibility() throws Exception {
>          FileInputStream fis = new FileInputStream(resolveFile("src/ 
> test/data/gbeaninfo/SERIALIZATION_-6198804067155550221.ser"));
>          ObjectInputStream is = new ObjectInputStream(fis);
>          GBeanInfo beanInfo = (GBeanInfo) is.readObject();
> @@ -131,7 +131,7 @@
>
>      final static GAttributeInfo persistentAttrInfo = new  
> GAttributeInfo(persistentAttrName, String.class.getName(), true,  
> false, "getFoo", "setFoo");
>
> -    final static GOperationInfo opInfo = new GOperationInfo 
> ("operation");
> +    final static GOperationInfo opInfo = new GOperationInfo 
> ("operation", "java.lang.Object");
>
>      final static GReferenceInfo refInfo = new GReferenceInfo 
> ("reference", String.class.getName(), String.class.getName(),  
> "setReference", "Fooifier");
>
>
> Modified: geronimo/server/trunk/modules/geronimo-kernel/src/test/ 
> java/org/apache/geronimo/kernel/MockGBean.java
> URL: http://svn.apache.org/viewvc/geronimo/server/trunk/modules/ 
> geronimo-kernel/src/test/java/org/apache/geronimo/kernel/ 
> MockGBean.java?view=diff&rev=485321&r1=485320&r2=485321
> ====================================================================== 
> ========
> --- geronimo/server/trunk/modules/geronimo-kernel/src/test/java/org/ 
> apache/geronimo/kernel/MockGBean.java (original)
> +++ geronimo/server/trunk/modules/geronimo-kernel/src/test/java/org/ 
> apache/geronimo/kernel/MockGBean.java Sun Dec 10 16:14:46 2006
> @@ -74,11 +74,11 @@
>          infoFactory.addAttribute("endpointMutableInt",  
> Integer.TYPE, false);
>          infoFactory.addAttribute("someObject", Object.class, true);
>
> -        infoFactory.addOperation("echo", new Class[]{String.class});
> -        infoFactory.addOperation("checkEndpoint");
> -        infoFactory.addOperation("checkEndpointCollection");
> -        infoFactory.addOperation("doSomething", new Class[] 
> {String.class});
> -        infoFactory.addOperation("fetchValue");
> +        infoFactory.addOperation("echo", new Class[] 
> {String.class}, "java.lang.Object");
> +        infoFactory.addOperation("checkEndpoint",  
> "java.lang.Object");
> +        infoFactory.addOperation("checkEndpointCollection",  
> "java.lang.Object");
> +        infoFactory.addOperation("doSomething", new Class[] 
> {String.class}, "java.lang.Object");
> +        infoFactory.addOperation("fetchValue", "java.lang.Object");
>
>          infoFactory.addInterface(MockEndpoint.class, new String[] 
> {"mutableInt"});
>          infoFactory.addInterface(MockParentInterface1.class, new  
> String[]{"value"});
>
> Modified: geronimo/server/trunk/modules/geronimo-kernel/src/test/ 
> java/org/apache/geronimo/kernel/config/MyGBean.java
> URL: http://svn.apache.org/viewvc/geronimo/server/trunk/modules/ 
> geronimo-kernel/src/test/java/org/apache/geronimo/kernel/config/ 
> MyGBean.java?view=diff&rev=485321&r1=485320&r2=485321
> ====================================================================== 
> ========
> --- geronimo/server/trunk/modules/geronimo-kernel/src/test/java/org/ 
> apache/geronimo/kernel/config/MyGBean.java (original)
> +++ geronimo/server/trunk/modules/geronimo-kernel/src/test/java/org/ 
> apache/geronimo/kernel/config/MyGBean.java Sun Dec 10 16:14:46 2006
> @@ -32,7 +32,7 @@
>
>      static {
>          GBeanInfoBuilder infoFactory =  
> GBeanInfoBuilder.createStatic(MyGBean.class);
> -        infoFactory.addOperation("main", new Class[]{String 
> [].class});
> +        infoFactory.addOperation("main", new Class[]{String 
> [].class}, "void");
>          GBEAN_INFO = infoFactory.getBeanInfo();
>      }
>  }
>
> Modified: geronimo/server/trunk/modules/geronimo-system/src/main/ 
> java/org/apache/geronimo/system/jmx/JMXUtil.java
> URL: http://svn.apache.org/viewvc/geronimo/server/trunk/modules/ 
> geronimo-system/src/main/java/org/apache/geronimo/system/jmx/ 
> JMXUtil.java?view=diff&rev=485321&r1=485320&r2=485321
> ====================================================================== 
> ========
> --- geronimo/server/trunk/modules/geronimo-system/src/main/java/org/ 
> apache/geronimo/system/jmx/JMXUtil.java (original)
> +++ geronimo/server/trunk/modules/geronimo-system/src/main/java/org/ 
> apache/geronimo/system/jmx/JMXUtil.java Sun Dec 10 16:14:46 2006
> @@ -74,7 +74,7 @@
>                  parameters[p] = new MBeanParameterInfo("parameter"  
> + p, type, "no description available");
>                  p++;
>              }
> -            operations[o] = new MBeanOperationInfo 
> (gOperationInfo.getName(), "no description available", parameters,  
> "java.lang.Object", MBeanOperationInfo.UNKNOWN);
> +            operations[o] = new MBeanOperationInfo 
> (gOperationInfo.getName(), "no description available", parameters,  
> gOperationInfo.getReturnType() , MBeanOperationInfo.UNKNOWN);
>              o++;
>          }
>
>
> Modified: geronimo/server/trunk/modules/geronimo-system/src/main/ 
> java/org/apache/geronimo/system/logging/log4j/Log4jService.java
> URL: http://svn.apache.org/viewvc/geronimo/server/trunk/modules/ 
> geronimo-system/src/main/java/org/apache/geronimo/system/logging/ 
> log4j/Log4jService.java?view=diff&rev=485321&r1=485320&r2=485321
> ====================================================================== 
> ========
> --- geronimo/server/trunk/modules/geronimo-system/src/main/java/org/ 
> apache/geronimo/system/logging/log4j/Log4jService.java (original)
> +++ geronimo/server/trunk/modules/geronimo-system/src/main/java/org/ 
> apache/geronimo/system/logging/log4j/Log4jService.java Sun Dec 10  
> 16:14:46 2006
> @@ -712,10 +712,10 @@
>
>          infoFactory.addReference("ServerInfo", ServerInfo.class,  
> "GBean");
>
> -        infoFactory.addOperation("reconfigure");
> -        infoFactory.addOperation("setLoggerLevel", new Class[] 
> {String.class, String.class});
> -        infoFactory.addOperation("getLoggerLevel", new Class[] 
> {String.class});
> -        infoFactory.addOperation("getLoggerEffectiveLevel", new  
> Class[]{String.class});
> +        infoFactory.addOperation("reconfigure", "void");
> +        infoFactory.addOperation("setLoggerLevel", new Class[] 
> {String.class, String.class}, "void");
> +        infoFactory.addOperation("getLoggerLevel", new Class[] 
> {String.class}, "java.lang.String");
> +        infoFactory.addOperation("getLoggerEffectiveLevel", new  
> Class[]{String.class}, "java.lang.String");
>          infoFactory.addInterface(SystemLog.class);
>
>          infoFactory.setConstructor(new String[]{"configFileName",  
> "refreshPeriodSeconds", "ServerInfo"});
>
>


Mime
View raw message