QPID-1447 : Modified VirtualHostHouseKeepingPlugin to return a TimeUnit and force plugin to perform validation. Potentially could be refactored to allow all VHHKPlugins to process TimeUnits in a consistent way.

Add Config validation and UnitTest for SlowConsumerDetctionConfiguration.

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

6 files changed, 318 insertions, 36 deletions

diff --git a/java/broker-plugins/experimental/slowconsumerdetection/src/main/java/org/apache/qpid/server/configuration/plugin/SlowConsumerDetectionConfiguration.java b/java/broker-plugins/experimental/slowconsumerdetection/src/main/java/org/apache/qpid/server/configuration/plugin/SlowConsumerDetectionConfiguration.java
index e42eba9fc6..3c87ca08b2 100644
--- a/java/broker-plugins/experimental/slowconsumerdetection/src/main/java/org/apache/qpid/server/configuration/plugin/SlowConsumerDetectionConfiguration.java
+++ b/java/broker-plugins/experimental/slowconsumerdetection/src/main/java/org/apache/qpid/server/configuration/plugin/SlowConsumerDetectionConfiguration.java
@@ -22,6 +22,7 @@ package org.apache.qpid.server.configuration.plugin;
 
 import org.apache.commons.configuration.Configuration;
 import org.apache.commons.configuration.ConfigurationException;
+import org.apache.commons.configuration.ConversionException;
 import org.apache.qpid.server.configuration.plugins.ConfigurationPlugin;
 import org.apache.qpid.server.configuration.plugins.ConfigurationPluginFactory;
 
@@ -44,6 +45,9 @@ public class SlowConsumerDetectionConfiguration extends ConfigurationPlugin
         }
     }
 
+    //Set Default time unit to seconds
+    TimeUnit _timeUnit = TimeUnit.SECONDS;
+
     public String[] getElementsProcessed()
     {
         return new String[]{"delay",
@@ -55,9 +59,9 @@ public class SlowConsumerDetectionConfiguration extends ConfigurationPlugin
         return _configuration.getLong("delay", 10);
     }
 
-    public String getTimeUnit()
+    public TimeUnit getTimeUnit()
     {
-        return _configuration.getString("timeunit", TimeUnit.SECONDS.toString());
+        return  _timeUnit;
     }
 
     @Override
@@ -65,6 +69,50 @@ public class SlowConsumerDetectionConfiguration extends ConfigurationPlugin
     {
         super.setConfiguration(path, configuration);
 
+        //Validate Configuration
+
+        try
+        {
+            long delay = _configuration.getLong("delay");
+            if (delay <= 0)
+            {
+                throw new ConfigurationException("Slow Consumer Detection Delay must be a Positive Long value.");
+            }
+        }
+        catch (Exception e)
+        {
+            Throwable last = e;
+
+            // Find the first cause
+            if (e instanceof ConversionException)
+            {
+                Throwable t = e.getCause();
+                while (t != null)
+                {
+                    last = t;
+                    t = last.getCause();
+                }
+            }
+
+            throw new ConfigurationException("Unable to configure Slow Consumer Detection invalid delay:"+ _configuration.getString("delay"), last);
+        }
+
+        String timeUnit = _configuration.getString("timeunit");
+
+
+        if (timeUnit != null)
+        {
+            try
+            {
+                _timeUnit = TimeUnit.valueOf(timeUnit.toUpperCase());
+            }
+            catch (IllegalArgumentException iae)
+            {
+                throw new ConfigurationException("Unable to configure Slow Consumer Detection invalid TimeUnit:" + timeUnit);            
+            }
+        }
+
+
         System.out.println("Configured SCDC");
         System.out.println("Delay:" + getDelay());
         System.out.println("TimeUnit:" + getTimeUnit());
diff --git a/java/broker-plugins/experimental/slowconsumerdetection/src/main/java/org/apache/qpid/server/configuration/plugin/SlowConsumerDetectionQueueConfiguration.java b/java/broker-plugins/experimental/slowconsumerdetection/src/main/java/org/apache/qpid/server/configuration/plugin/SlowConsumerDetectionQueueConfiguration.java
index c5281c626d..ce3393ae70 100644
--- a/java/broker-plugins/experimental/slowconsumerdetection/src/main/java/org/apache/qpid/server/configuration/plugin/SlowConsumerDetectionQueueConfiguration.java
+++ b/java/broker-plugins/experimental/slowconsumerdetection/src/main/java/org/apache/qpid/server/configuration/plugin/SlowConsumerDetectionQueueConfiguration.java
@@ -29,10 +29,8 @@ import org.apache.qpid.server.registry.ApplicationRegistry;
 import org.apache.qpid.slowconsumerdetection.policies.SlowConsumerPolicyPlugin;
 import org.apache.qpid.slowconsumerdetection.policies.SlowConsumerPolicyPluginFactory;
 
-import java.util.HashSet;
 import java.util.Iterator;
 import java.util.Map;
-import java.util.Set;
 
 public class SlowConsumerDetectionQueueConfiguration extends ConfigurationPlugin
 {
@@ -93,23 +91,22 @@ public class SlowConsumerDetectionQueueConfiguration extends ConfigurationPlugin
         Map<String, SlowConsumerPolicyPluginFactory> factories =
                 pluginManager.getPlugins(SlowConsumerPolicyPluginFactory.class);
 
-        Iterator<?> keys = policyConfig.getConfig().getKeys();
-
-        while (keys.hasNext())
-        {
-            String key = (String) keys.next();
-
-            _logger.debug("Policy Keys:" + key);
-
-        }
-
         if (policyConfig == null)
         {
             throw new ConfigurationException("No Slow Consumer Policy specified at:" + path + ". Known Policies:" + factories.keySet());
-        }        
+        }
 
         if (_logger.isDebugEnabled())
         {
+            Iterator<?> keys = policyConfig.getConfig().getKeys();
+
+            while (keys.hasNext())
+            {
+                String key = (String) keys.next();
+
+                _logger.debug("Policy Keys:" + key);
+            }
+
             _logger.debug("Configured SCDQC");
             _logger.debug("Age:" + getMessageAge());
             _logger.debug("Depth:" + getDepth());
diff --git a/java/broker-plugins/experimental/slowconsumerdetection/src/main/java/org/apache/qpid/server/virtualhost/plugin/SlowConsumerDetection.java b/java/broker-plugins/experimental/slowconsumerdetection/src/main/java/org/apache/qpid/server/virtualhost/plugin/SlowConsumerDetection.java
index afd3059a77..7723a4109e 100644
--- a/java/broker-plugins/experimental/slowconsumerdetection/src/main/java/org/apache/qpid/server/virtualhost/plugin/SlowConsumerDetection.java
+++ b/java/broker-plugins/experimental/slowconsumerdetection/src/main/java/org/apache/qpid/server/virtualhost/plugin/SlowConsumerDetection.java
@@ -29,11 +29,12 @@ import org.apache.qpid.server.virtualhost.plugins.VirtualHostHouseKeepingPlugin;
 import org.apache.qpid.server.virtualhost.plugins.VirtualHostPluginFactory;
 import org.apache.qpid.slowconsumerdetection.policies.SlowConsumerPolicyPlugin;
 
+import java.util.concurrent.TimeUnit;
+
 class SlowConsumerDetection extends VirtualHostHouseKeepingPlugin
 {
     Logger _logger = Logger.getLogger(SlowConsumerDetection.class);
     private SlowConsumerDetectionConfiguration _config;
-    private SlowConsumerPolicyPlugin _policy;
 
     public static class SlowConsumerFactory implements VirtualHostPluginFactory
     {
@@ -88,7 +89,7 @@ class SlowConsumerDetection extends VirtualHostHouseKeepingPlugin
         return _config.getDelay();
     }
 
-    public String getTimeUnit()
+    public TimeUnit getTimeUnit()
     {
         return _config.getTimeUnit();
     }
diff --git a/java/broker-plugins/experimental/slowconsumerdetection/src/test/java/org/apache/qpid/server/virtualhost/plugin/SlowConsumerDetectionConfigurationTest.java b/java/broker-plugins/experimental/slowconsumerdetection/src/test/java/org/apache/qpid/server/virtualhost/plugin/SlowConsumerDetectionConfigurationTest.java
new file mode 100644
index 0000000000..049402d83e
--- /dev/null
+++ b/java/broker-plugins/experimental/slowconsumerdetection/src/test/java/org/apache/qpid/server/virtualhost/plugin/SlowConsumerDetectionConfigurationTest.java
@@ -0,0 +1,251 @@
+/*
+ *
+ * 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.virtualhost.plugin;
+
+import junit.framework.TestCase;
+import org.apache.commons.configuration.CompositeConfiguration;
+import org.apache.commons.configuration.ConfigurationException;
+import org.apache.commons.configuration.XMLConfiguration;
+import org.apache.qpid.server.configuration.plugin.SlowConsumerDetectionConfiguration;
+
+import java.util.concurrent.TimeUnit;
+
+/** Provide Unit Test coverage of the SlowConsumerConfiguration */
+public class SlowConsumerDetectionConfigurationTest extends TestCase
+{
+
+    public void testConfigLoadingValidConfig()
+    {
+        SlowConsumerDetectionConfiguration config = new SlowConsumerDetectionConfiguration();
+
+        XMLConfiguration xmlconfig = new XMLConfiguration();
+
+        xmlconfig.addProperty("delay", "10");
+        xmlconfig.addProperty("timeunit", TimeUnit.MICROSECONDS.toString());
+
+        // Create a CompositeConfiguration as this is what the broker uses
+        CompositeConfiguration composite = new CompositeConfiguration();
+        composite.addConfiguration(xmlconfig);
+
+        try
+        {
+            config.setConfiguration("", composite);
+        }
+        catch (ConfigurationException e)
+        {
+            e.printStackTrace();
+            fail(e.getMessage());
+        }
+    }
+
+    /**
+     * TimeUnit parsing requires the String value be in UpperCase.
+     * Ensure we can handle when the user doesn't know this.
+     */
+    public void testConfigLoadingValidConfigStrangeTimeUnit()
+    {
+        SlowConsumerDetectionConfiguration config = new SlowConsumerDetectionConfiguration();
+
+        XMLConfiguration xmlconfig = new XMLConfiguration();
+
+        xmlconfig.addProperty("delay", "10");
+        xmlconfig.addProperty("timeunit", "MiCrOsEcOnDs");
+
+        // Create a CompositeConfiguration as this is what the broker uses
+        CompositeConfiguration composite = new CompositeConfiguration();
+        composite.addConfiguration(xmlconfig);
+
+        try
+        {
+            config.setConfiguration("", composite);
+        }
+        catch (ConfigurationException e)
+        {
+            e.printStackTrace();
+            fail(e.getMessage());
+        }
+    }
+
+    /** Test that delay must be long not a string value. */
+    public void testConfigLoadingInValidDelayString()
+    {
+        SlowConsumerDetectionConfiguration config = new SlowConsumerDetectionConfiguration();
+
+        XMLConfiguration xmlconfig = new XMLConfiguration();
+
+        xmlconfig.addProperty("delay", "ten");
+        xmlconfig.addProperty("timeunit", TimeUnit.MICROSECONDS.toString());
+
+        // Create a CompositeConfiguration as this is what the broker uses
+        CompositeConfiguration composite = new CompositeConfiguration();
+        composite.addConfiguration(xmlconfig);
+
+        try
+        {
+            config.setConfiguration("", composite);
+            fail("Configuration should fail to validate");
+        }
+        catch (ConfigurationException e)
+        {
+            Throwable cause = e.getCause();
+
+            assertEquals("Cause not correct", NumberFormatException.class, cause.getClass());
+        }
+    }
+
+    /** Test that negative delays are invalid */
+    public void testConfigLoadingInValidDelayNegative()
+    {
+        SlowConsumerDetectionConfiguration config = new SlowConsumerDetectionConfiguration();
+
+        XMLConfiguration xmlconfig = new XMLConfiguration();
+
+        xmlconfig.addProperty("delay", "-10");
+        xmlconfig.addProperty("timeunit", TimeUnit.MICROSECONDS.toString());
+
+        // Create a CompositeConfiguration as this is what the broker uses
+        CompositeConfiguration composite = new CompositeConfiguration();
+        composite.addConfiguration(xmlconfig);
+
+        try
+        {
+            config.setConfiguration("", composite);
+            fail("Configuration should fail to validate");
+        }
+        catch (ConfigurationException e)
+        {
+            Throwable cause = e.getCause();
+
+            assertNotNull("Configuration Exception must not be null.", cause);
+            assertEquals("Cause not correct",
+                         ConfigurationException.class, cause.getClass());
+            assertEquals("Incorrect message.",
+                         "Slow Consumer Detection Delay must be a Positive Long value.",
+                         cause.getMessage());
+        }
+    }
+
+    /** Tet that delay cannot be 0 */
+    public void testConfigLoadingInValidDelayZero()
+    {
+        SlowConsumerDetectionConfiguration config = new SlowConsumerDetectionConfiguration();
+
+        XMLConfiguration xmlconfig = new XMLConfiguration();
+
+        xmlconfig.addProperty("delay", "0");
+        xmlconfig.addProperty("timeunit", TimeUnit.MICROSECONDS.toString());
+
+        // Create a CompositeConfiguration as this is what the broker uses
+        CompositeConfiguration composite = new CompositeConfiguration();
+        composite.addConfiguration(xmlconfig);
+
+        try
+        {
+            config.setConfiguration("", composite);
+            fail("Configuration should fail to validate");
+        }
+        catch (ConfigurationException e)
+        {
+            Throwable cause = e.getCause();
+
+            assertNotNull("Configuration Exception must not be null.", cause);
+            assertEquals("Cause not correct",
+                         ConfigurationException.class, cause.getClass());
+            assertEquals("Incorrect message.",
+                         "Slow Consumer Detection Delay must be a Positive Long value.",
+                         cause.getMessage());
+        }
+    }
+
+    /** Test that missing delay fails */
+    public void testConfigLoadingInValidMissingDelay()
+    {
+        SlowConsumerDetectionConfiguration config = new SlowConsumerDetectionConfiguration();
+
+        XMLConfiguration xmlconfig = new XMLConfiguration();
+
+        xmlconfig.addProperty("timeunit", TimeUnit.SECONDS.toString());
+
+        // Create a CompositeConfiguration as this is what the broker uses
+        CompositeConfiguration composite = new CompositeConfiguration();
+        composite.addConfiguration(xmlconfig);
+        try
+        {
+            config.setConfiguration("", composite);
+            fail("Configuration should fail to validate");
+        }
+        catch (ConfigurationException e)
+        {
+            assertEquals("Incorrect message.", "Unable to configure Slow Consumer Detection invalid delay:null", e.getMessage());
+        }
+    }
+
+    /** Test that erroneous TimeUnit fails */
+    public void testConfigLoadingInValidTimeUnit()
+    {
+        SlowConsumerDetectionConfiguration config = new SlowConsumerDetectionConfiguration();
+
+        String TIMEUNIT = "foo";
+        XMLConfiguration xmlconfig = new XMLConfiguration();
+
+        xmlconfig.addProperty("delay", "10");
+        xmlconfig.addProperty("timeunit", TIMEUNIT);
+
+        // Create a CompositeConfiguration as this is what the broker uses
+        CompositeConfiguration composite = new CompositeConfiguration();
+        composite.addConfiguration(xmlconfig);
+        try
+        {
+            config.setConfiguration("", composite);
+            fail("Configuration should fail to validate");
+        }
+        catch (ConfigurationException e)
+        {
+            assertEquals("Incorrect message.", "Unable to configure Slow Consumer Detection invalid TimeUnit:" + TIMEUNIT, e.getMessage());
+        }
+    }
+
+    /** Test Missing TimeUnit value gets default */
+    public void testConfigLoadingMissingTimeUnitDefaults()
+    {
+        SlowConsumerDetectionConfiguration config = new SlowConsumerDetectionConfiguration();
+
+        XMLConfiguration xmlconfig = new XMLConfiguration();
+
+        xmlconfig.addProperty("delay", "10");
+
+        // Create a CompositeConfiguration as this is what the broker uses
+        CompositeConfiguration composite = new CompositeConfiguration();
+        composite.addConfiguration(xmlconfig);
+        try
+        {
+            config.setConfiguration("", composite);
+        }
+        catch (ConfigurationException e)
+        {
+            e.printStackTrace();
+            fail(e.getMessage());
+        }
+
+        assertEquals("Default TimeUnit incorrect", TimeUnit.SECONDS, config.getTimeUnit());
+    }
+
+}
diff --git a/java/broker/src/main/java/org/apache/qpid/server/virtualhost/VirtualHostImpl.java b/java/broker/src/main/java/org/apache/qpid/server/virtualhost/VirtualHostImpl.java
index 4a2f796d2b..2542e1e94d 100644
--- a/java/broker/src/main/java/org/apache/qpid/server/virtualhost/VirtualHostImpl.java
+++ b/java/broker/src/main/java/org/apache/qpid/server/virtualhost/VirtualHostImpl.java
@@ -343,26 +343,9 @@ public class VirtualHostImpl implements Accessable, VirtualHost
                         VirtualHostHouseKeepingPlugin plugin =
                                 plugins.get(pluginName).newInstance(this);
 
-                        TimeUnit units = TimeUnit.MILLISECONDS;
-
-                        if (plugin.getTimeUnit() != null)
-                        {
-                            try
-                            {
-                                units = TimeUnit.valueOf(plugin.getTimeUnit());
-                            }
-                            catch (IllegalArgumentException iae)
-                            {
-                                _logger.warn("Plugin:" + pluginName +
-                                             " provided an illegal TimeUnit value:"
-                                             + plugin.getTimeUnit());
-                                // Warn and use default of millseconds
-                                // Should not occur in a well behaved plugin
-                            }
-                        }
 
                         _houseKeepingTasks.scheduleAtFixedRate(plugin, plugin.getDelay() / 2,
-                                                       plugin.getDelay(), units);
+                                                       plugin.getDelay(), plugin.getTimeUnit());
 
                         _logger.info("Loaded VirtualHostPlugin:" + plugin);
                     }
diff --git a/java/broker/src/main/java/org/apache/qpid/server/virtualhost/plugins/VirtualHostHouseKeepingPlugin.java b/java/broker/src/main/java/org/apache/qpid/server/virtualhost/plugins/VirtualHostHouseKeepingPlugin.java
index e76844fa3a..d2fd4daaa5 100644
--- a/java/broker/src/main/java/org/apache/qpid/server/virtualhost/plugins/VirtualHostHouseKeepingPlugin.java
+++ b/java/broker/src/main/java/org/apache/qpid/server/virtualhost/plugins/VirtualHostHouseKeepingPlugin.java
@@ -23,6 +23,8 @@ package org.apache.qpid.server.virtualhost.plugins;
 import org.apache.qpid.server.virtualhost.HouseKeepingTask;
 import org.apache.qpid.server.virtualhost.VirtualHost;
 
+import java.util.concurrent.TimeUnit;
+
 public abstract class VirtualHostHouseKeepingPlugin extends HouseKeepingTask
 {
     public VirtualHostHouseKeepingPlugin(VirtualHost vhost)
@@ -44,6 +46,6 @@ public abstract class VirtualHostHouseKeepingPlugin extends HouseKeepingTask
      *
      * @see java.util.concurrent.TimeUnit for valid value.
      */
-    public abstract String getTimeUnit();
+    public abstract TimeUnit getTimeUnit();
 
 }