QPID-4093: Resolve some outstanding TODOs in new JMX module

git-svn-id: https://svn.apache.org/repos/asf/qpid/trunk@1356248 13f79535-47bb-0310-9956-ffa450edef68

12 files changed, 150 insertions, 57 deletions

diff --git a/qpid/java/broker-plugins/jmx/src/main/java/org/apache/qpid/server/jmx/mbeans/AbstractStatisticsGatheringMBean.java b/qpid/java/broker-plugins/jmx/src/main/java/org/apache/qpid/server/jmx/mbeans/AbstractStatisticsGatheringMBean.java
index 4115f9f363..6ab7db3629 100644
--- a/qpid/java/broker-plugins/jmx/src/main/java/org/apache/qpid/server/jmx/mbeans/AbstractStatisticsGatheringMBean.java
+++ b/qpid/java/broker-plugins/jmx/src/main/java/org/apache/qpid/server/jmx/mbeans/AbstractStatisticsGatheringMBean.java
@@ -1,3 +1,23 @@
+/*
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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.qpid.server.jmx.mbeans;
 
 import javax.management.NotCompliantMBeanException;
@@ -7,22 +27,6 @@ import org.apache.qpid.server.jmx.AMQManagedObject;
 import org.apache.qpid.server.jmx.ManagedObjectRegistry;
 import org.apache.qpid.server.model.VirtualHost;
 
-/**
- * Licensed to the Apache Software Foundation (ASF) under one or more
- * contributor license agreements.  See the NOTICE file distributed with
- * this work for additional information regarding copyright ownership.
- * The ASF licenses this file to You 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
- * <p/>
- * http://www.apache.org/licenses/LICENSE-2.0
- * <p/>
- * 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.
- */
 abstract class AbstractStatisticsGatheringMBean<T extends ConfiguredObject> extends AMQManagedObject
 {
     private long _lastStatUpdateTime;
diff --git a/qpid/java/broker-plugins/jmx/src/main/java/org/apache/qpid/server/jmx/mbeans/ConnectionMBean.java b/qpid/java/broker-plugins/jmx/src/main/java/org/apache/qpid/server/jmx/mbeans/ConnectionMBean.java
index 024ee39318..d0c0d5e73f 100644
--- a/qpid/java/broker-plugins/jmx/src/main/java/org/apache/qpid/server/jmx/mbeans/ConnectionMBean.java
+++ b/qpid/java/broker-plugins/jmx/src/main/java/org/apache/qpid/server/jmx/mbeans/ConnectionMBean.java
@@ -169,7 +169,6 @@ public class ConnectionMBean extends AbstractStatisticsGatheringMBean<Connection
 
     public void setStatisticsEnabled(boolean enabled)
     {
-        // TODO - Implement setStatisticsEnabled
         updateStats();
     }
 
diff --git a/qpid/java/broker-plugins/jmx/src/main/java/org/apache/qpid/server/jmx/mbeans/ServerInformationMBean.java b/qpid/java/broker-plugins/jmx/src/main/java/org/apache/qpid/server/jmx/mbeans/ServerInformationMBean.java
index 597b98ccaa..67d5861dec 100644
--- a/qpid/java/broker-plugins/jmx/src/main/java/org/apache/qpid/server/jmx/mbeans/ServerInformationMBean.java
+++ b/qpid/java/broker-plugins/jmx/src/main/java/org/apache/qpid/server/jmx/mbeans/ServerInformationMBean.java
@@ -24,7 +24,6 @@ import java.io.IOException;
 import javax.management.JMException;
 import javax.management.NotCompliantMBeanException;
 
-import org.apache.qpid.common.QpidProperties;
 import org.apache.qpid.management.common.mbeans.ServerInformation;
 import org.apache.qpid.management.common.mbeans.annotations.MBeanDescription;
 import org.apache.qpid.server.jmx.ManagedObject;
@@ -34,16 +33,13 @@ import org.apache.qpid.server.model.Broker;
 @MBeanDescription("Server Information Interface")
 public class ServerInformationMBean extends AbstractStatisticsGatheringMBean<Broker> implements ServerInformation
 {
-    private String _buildVersion;
-    private String _productVersion;
+    private final Broker _broker;
 
     public ServerInformationMBean(ManagedObjectRegistry registry, Broker broker) 
                                                     throws NotCompliantMBeanException, JMException
     {
         super(ServerInformation.class, ServerInformation.TYPE, registry, broker);
-
-        _buildVersion = QpidProperties.getBuildVersion();
-        _productVersion = QpidProperties.getReleaseVersion();
+        _broker = broker;
 
         register();
     }
@@ -69,19 +65,19 @@ public class ServerInformationMBean extends AbstractStatisticsGatheringMBean<Bro
     @Override
     public String getBuildVersion() throws IOException
     {
-        return _buildVersion;
+        return (String) _broker.getAttribute(Broker.BUILD_VERSION);
     }
 
     @Override
     public String getProductVersion() throws IOException
     {
-        return _productVersion;
+        return (String) _broker.getAttribute(Broker.PRODUCT_VERSION);
     }
 
     @Override
     public boolean isStatisticsEnabled()
     {
-        return false;
+        return true;
     }
 
     @Override
diff --git a/qpid/java/broker-plugins/jmx/src/main/java/org/apache/qpid/server/jmx/mbeans/VirtualHostMBean.java b/qpid/java/broker-plugins/jmx/src/main/java/org/apache/qpid/server/jmx/mbeans/VirtualHostMBean.java
index 85f53d9c0d..6990a40dee 100644
--- a/qpid/java/broker-plugins/jmx/src/main/java/org/apache/qpid/server/jmx/mbeans/VirtualHostMBean.java
+++ b/qpid/java/broker-plugins/jmx/src/main/java/org/apache/qpid/server/jmx/mbeans/VirtualHostMBean.java
@@ -21,6 +21,7 @@
 
 package org.apache.qpid.server.jmx.mbeans;
 
+import org.apache.log4j.Logger;
 import org.apache.qpid.server.jmx.AMQManagedObject;
 import org.apache.qpid.server.jmx.ManagedObject;
 import org.apache.qpid.server.jmx.ManagedObjectRegistry;
@@ -42,6 +43,8 @@ import java.util.Map;
 
 public class VirtualHostMBean extends AMQManagedObject implements ManagedVirtualHost, ConfigurationChangeListener
 {
+    private static final Logger LOGGER = Logger.getLogger(VirtualHostMBean.class);
+
     private final VirtualHost _virtualHost;
 
     private final Map<ConfiguredObject, AMQManagedObject> _children =
@@ -145,13 +148,13 @@ public class VirtualHostMBean extends AMQManagedObject implements ManagedVirtual
                 }
                 else
                 {
-                    // TODO
+                    LOGGER.debug("Unsupported child : " + child.getName() + " type : " + child.getClass());
                 }
 
             }
             catch(JMException e)
             {
-                e.printStackTrace();  //TODO - report error on adding child MBean
+                LOGGER.error("Failed to add mbean for child : " + child.getName(), e);
             }
         }
     }
@@ -169,7 +172,7 @@ public class VirtualHostMBean extends AMQManagedObject implements ManagedVirtual
                 }
                 catch(JMException e)
                 {
-                    e.printStackTrace();  //TODO - report error on removing child MBean
+                    LOGGER.error("Failed to remove mbean for child : " + child.getName(), e);
                 }
             }
         }
diff --git a/qpid/java/broker-plugins/jmx/src/main/java/org/apache/qpid/server/jmx/mbeans/VirtualHostManagerMBean.java b/qpid/java/broker-plugins/jmx/src/main/java/org/apache/qpid/server/jmx/mbeans/VirtualHostManagerMBean.java
index 9d12d8a493..b3dbbc424a 100644
--- a/qpid/java/broker-plugins/jmx/src/main/java/org/apache/qpid/server/jmx/mbeans/VirtualHostManagerMBean.java
+++ b/qpid/java/broker-plugins/jmx/src/main/java/org/apache/qpid/server/jmx/mbeans/VirtualHostManagerMBean.java
@@ -36,8 +36,6 @@ import javax.management.ObjectName;
 import javax.management.OperationsException;
 
 import org.apache.log4j.Logger;
-import org.apache.qpid.AMQInvalidArgumentException;
-import org.apache.qpid.AMQUnknownExchangeType;
 import org.apache.qpid.management.common.mbeans.ManagedBroker;
 import org.apache.qpid.management.common.mbeans.ManagedQueue;
 import org.apache.qpid.management.common.mbeans.annotations.MBeanConstructor;
diff --git a/qpid/java/broker-plugins/jmx/src/test/java/org/apache/qpid/server/jmx/mbeans/ConnectionMBeanTest.java b/qpid/java/broker-plugins/jmx/src/test/java/org/apache/qpid/server/jmx/mbeans/ConnectionMBeanTest.java
index f97c5a7210..0c5fb0bf1f 100644
--- a/qpid/java/broker-plugins/jmx/src/test/java/org/apache/qpid/server/jmx/mbeans/ConnectionMBeanTest.java
+++ b/qpid/java/broker-plugins/jmx/src/test/java/org/apache/qpid/server/jmx/mbeans/ConnectionMBeanTest.java
@@ -172,6 +172,11 @@ public class ConnectionMBeanTest extends TestCase
         assertAttribute("authorizedId", "testAuthorizedId", Connection.PRINCIPAL);
     }
 
+    public void testGetClientId() throws Exception
+    {
+        assertAttribute("clientId", "testClientId", Connection.CLIENT_ID);
+    }
+
     public void testGetVersion() throws Exception
     {
         assertAttribute("version", "testVersion", Connection.CLIENT_VERSION);
diff --git a/qpid/java/broker-plugins/jmx/src/test/java/org/apache/qpid/server/jmx/mbeans/ServerInformationMBeanTest.java b/qpid/java/broker-plugins/jmx/src/test/java/org/apache/qpid/server/jmx/mbeans/ServerInformationMBeanTest.java
new file mode 100644
index 0000000000..86eab0245e
--- /dev/null
+++ b/qpid/java/broker-plugins/jmx/src/test/java/org/apache/qpid/server/jmx/mbeans/ServerInformationMBeanTest.java
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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.qpid.server.jmx.mbeans;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import org.apache.qpid.server.jmx.ManagedObjectRegistry;
+import org.apache.qpid.server.model.Broker;
+import org.apache.qpid.server.model.Connection;
+import org.apache.qpid.server.model.Statistics;
+
+import junit.framework.TestCase;
+
+public class ServerInformationMBeanTest extends TestCase
+{
+    private ManagedObjectRegistry _mockManagedObjectRegistry;
+    private Broker _mockBroker;
+    private Statistics _mockBrokerStatistics;
+    private ServerInformationMBean _mbean;
+
+    @Override
+    protected void setUp() throws Exception
+    {
+        _mockManagedObjectRegistry = mock(ManagedObjectRegistry.class);
+        _mockBroker = mock(Broker.class);
+        _mockBrokerStatistics = mock(Statistics.class);
+        when(_mockBroker.getStatistics()).thenReturn(_mockBrokerStatistics);
+
+        _mbean = new ServerInformationMBean(_mockManagedObjectRegistry, _mockBroker);
+    }
+
+    public void testMBeanRegistersItself() throws Exception
+    {
+        ServerInformationMBean mbean = new ServerInformationMBean(_mockManagedObjectRegistry, _mockBroker);
+        verify(_mockManagedObjectRegistry).registerObject(mbean);
+    }
+
+    /**********  Statistics **********/
+
+    public void testGetMessageCount() throws Exception
+    {
+        assertStatistic("totalDataDelivered", 16384l, Connection.BYTES_OUT);
+    }
+
+    /**********  Attributes **********/
+
+    public void testBuildVersion() throws Exception
+    {
+        assertAttribute("buildVersion", "0.0.1", Broker.BUILD_VERSION);
+    }
+
+    public void testProductVersion() throws Exception
+    {
+        assertAttribute("productVersion", "0.0.1", Broker.PRODUCT_VERSION);
+    }
+
+    /**********  Other Attributes **********/
+
+    public void testIsStatisticsEnabled() throws Exception
+    {
+        assertTrue("isStatisticsEnabled", _mbean.isStatisticsEnabled());
+    }
+
+    private void assertStatistic(String jmxAttributeName, Object expectedValue, String underlyingAttributeName) throws Exception
+    {
+        when(_mockBrokerStatistics.getStatistic(underlyingAttributeName)).thenReturn(expectedValue);
+        MBeanTestUtils.assertMBeanAttribute(_mbean, jmxAttributeName, expectedValue);
+    }
+
+    private void assertAttribute(String jmxAttributeName, Object expectedValue, String underlyingAttributeName) throws Exception
+    {
+        when(_mockBroker.getAttribute(underlyingAttributeName)).thenReturn(expectedValue);
+        MBeanTestUtils.assertMBeanAttribute(_mbean, jmxAttributeName, expectedValue);
+    }
+}
diff --git a/qpid/java/broker-plugins/jmx/src/test/java/org/apache/qpid/server/jmx/mbeans/VirtualHostManagerMBeanTest.java b/qpid/java/broker-plugins/jmx/src/test/java/org/apache/qpid/server/jmx/mbeans/VirtualHostManagerMBeanTest.java
index 93a80665a9..e3fac9f711 100644
--- a/qpid/java/broker-plugins/jmx/src/test/java/org/apache/qpid/server/jmx/mbeans/VirtualHostManagerMBeanTest.java
+++ b/qpid/java/broker-plugins/jmx/src/test/java/org/apache/qpid/server/jmx/mbeans/VirtualHostManagerMBeanTest.java
@@ -38,7 +38,6 @@ import org.apache.qpid.server.model.Queue;
 import org.apache.qpid.server.model.State;
 import org.apache.qpid.server.model.VirtualHost;
 import org.apache.qpid.server.queue.AMQQueueFactory;
-import org.mockito.verification.VerificationMode;
 
 public class VirtualHostManagerMBeanTest extends TestCase
 {
diff --git a/qpid/java/broker-plugins/management/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/RestServlet.java b/qpid/java/broker-plugins/management/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/RestServlet.java
index 96e260da56..dbf9f9b48d 100644
--- a/qpid/java/broker-plugins/management/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/RestServlet.java
+++ b/qpid/java/broker-plugins/management/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/RestServlet.java
@@ -1,18 +1,3 @@
-package org.apache.qpid.server.management.plugin.servlet.rest;
-
-import java.io.IOException;
-import java.io.PrintWriter;
-import java.net.SocketAddress;
-import java.util.*;
-import javax.servlet.ServletConfig;
-import javax.servlet.ServletException;
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
-import org.apache.qpid.AMQSecurityException;
-import org.apache.qpid.server.model.*;
-import org.codehaus.jackson.map.ObjectMapper;
-import org.codehaus.jackson.map.SerializationConfig;
-
 /**
  * Licensed to the Apache Software Foundation (ASF) under one or more
  * contributor license agreements.  See the NOTICE file distributed with
@@ -29,6 +14,22 @@ import org.codehaus.jackson.map.SerializationConfig;
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+package org.apache.qpid.server.management.plugin.servlet.rest;
+
+import java.io.IOException;
+import java.io.PrintWriter;
+import java.net.SocketAddress;
+import java.util.*;
+import javax.servlet.ServletConfig;
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import org.apache.qpid.AMQSecurityException;
+import org.apache.qpid.server.model.*;
+import org.codehaus.jackson.map.ObjectMapper;
+import org.codehaus.jackson.map.SerializationConfig;
+
+
 public class RestServlet extends AbstractServlet
 {
     /**
diff --git a/qpid/java/broker-plugins/management/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/SaslServlet.java b/qpid/java/broker-plugins/management/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/SaslServlet.java
index 4ca2d270dd..0787e7ec1d 100644
--- a/qpid/java/broker-plugins/management/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/SaslServlet.java
+++ b/qpid/java/broker-plugins/management/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/SaslServlet.java
@@ -24,6 +24,7 @@ import org.apache.commons.codec.binary.Base64;
 import org.codehaus.jackson.map.ObjectMapper;
 import org.codehaus.jackson.map.SerializationConfig;
 
+import org.apache.log4j.Logger;
 import org.apache.qpid.server.model.Broker;
 import org.apache.qpid.server.registry.ApplicationRegistry;
 import org.apache.qpid.server.security.auth.manager.AuthenticationManager;
@@ -47,6 +48,7 @@ import java.util.Random;
 
 public class SaslServlet extends AbstractServlet
 {
+    private static final Logger LOGGER = Logger.getLogger(SaslServlet.class);
 
     private static final SecureRandom SECURE_RANDOM = new SecureRandom();
     private static final String ATTR_RANDOM = "SaslServlet.Random";
@@ -181,14 +183,12 @@ public class SaslServlet extends AbstractServlet
         }
         catch(IOException e)
         {
-            //TODO
-            e.printStackTrace();
+            LOGGER.error("Error processing SASL request", e);
             throw e;
         }
         catch(RuntimeException e)
         {
-            //TODO
-            e.printStackTrace();
+            LOGGER.error("Error processing SASL request", e);
             throw e;
         }
 
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/model/Broker.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/model/Broker.java
index ca1de0f189..08b01a1b65 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/model/Broker.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/model/Broker.java
@@ -35,7 +35,6 @@ public interface Broker extends ConfiguredObject
     String PLATFORM = "platform";
     String PROCESS_PID = "processPid";
     String PRODUCT_VERSION = "productVersion";
-    String STATISTICS_ENABLED = "statisticsEnabled";
     String SUPPORTED_STORE_TYPES = "supportedStoreTypes";
     String CREATED = "created";
     String DURABLE = "durable";
@@ -55,7 +54,6 @@ public interface Broker extends ConfiguredObject
                               PLATFORM,
                               PROCESS_PID,
                               PRODUCT_VERSION,
-                              STATISTICS_ENABLED,
                               SUPPORTED_STORE_TYPES,
                               CREATED,
                               DURABLE,
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java
index ad88dbc613..e1b7c8fb26 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java
@@ -463,10 +463,6 @@ public class BrokerAdapter extends AbstractAdapter implements Broker, VirtualHos
         {
             return QpidProperties.getReleaseVersion();
         }
-        else if(STATISTICS_ENABLED.equals(name))
-        {
-            // TODO
-        }
         else if(SUPPORTED_STORE_TYPES.equals(name))
         {
             // TODO