QPID-4313: Address review comments from QPID-4109 (Reenable LoggingManagement)

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

7 files changed, 53 insertions, 48 deletions

diff --git a/java/broker-plugins/management-jmx/src/main/java/org/apache/qpid/server/jmx/JMXService.java b/java/broker-plugins/management-jmx/src/main/java/org/apache/qpid/server/jmx/JMXService.java
index 7a232d2584..c1049fa3a0 100644
--- a/java/broker-plugins/management-jmx/src/main/java/org/apache/qpid/server/jmx/JMXService.java
+++ b/java/broker-plugins/management-jmx/src/main/java/org/apache/qpid/server/jmx/JMXService.java
@@ -39,7 +39,7 @@ import org.apache.qpid.server.jmx.mbeans.ConfigurationManagementMBean;
 import org.apache.qpid.server.jmx.mbeans.ServerInformationMBean;
 import org.apache.qpid.server.jmx.mbeans.Shutdown;
 import org.apache.qpid.server.jmx.mbeans.VirtualHostMBean;
-import org.apache.qpid.server.logging.log4j.LoggingFacade;
+import org.apache.qpid.server.logging.log4j.LoggingManagementFacade;
 import org.apache.qpid.server.model.Broker;
 import org.apache.qpid.server.model.ConfigurationChangeListener;
 import org.apache.qpid.server.model.ConfiguredObject;
@@ -83,7 +83,7 @@ public class JMXService implements ConfigurationChangeListener
         _shutdown = new Shutdown(_objectRegistry);
         _serverInfo = new ServerInformationMBean(_objectRegistry, _broker);
         _configManagement = new ConfigurationManagementMBean(_objectRegistry);
-        _loggingManagement = new LoggingManagementMBean(LoggingFacade.getCurrentInstance(), _objectRegistry);
+        _loggingManagement = new LoggingManagementMBean(LoggingManagementFacade.getCurrentInstance(), _objectRegistry);
     }
     
     public void start() throws IOException, ConfigurationException
diff --git a/java/broker-plugins/management-jmx/src/main/java/org/apache/qpid/server/jmx/mbeans/LoggingManagementMBean.java b/java/broker-plugins/management-jmx/src/main/java/org/apache/qpid/server/jmx/mbeans/LoggingManagementMBean.java
index 0dac8ebe37..d6f4b5d8c9 100644
--- a/java/broker-plugins/management-jmx/src/main/java/org/apache/qpid/server/jmx/mbeans/LoggingManagementMBean.java
+++ b/java/broker-plugins/management-jmx/src/main/java/org/apache/qpid/server/jmx/mbeans/LoggingManagementMBean.java
@@ -26,7 +26,7 @@ import org.apache.qpid.management.common.mbeans.annotations.MBeanDescription;
 import org.apache.qpid.server.jmx.AMQManagedObject;
 import org.apache.qpid.server.jmx.ManagedObject;
 import org.apache.qpid.server.jmx.ManagedObjectRegistry;
-import org.apache.qpid.server.logging.log4j.LoggingFacade;
+import org.apache.qpid.server.logging.log4j.LoggingManagementFacade;
 import org.apache.qpid.server.logging.log4j.LoggingFacadeException;
 
 import javax.management.JMException;
@@ -55,7 +55,8 @@ public class LoggingManagementMBean extends AMQManagedObject implements LoggingM
     private static final TabularType LOGGER_LEVEL_TABULAR_TYE;
     private static final CompositeType LOGGER_LEVEL_COMPOSITE_TYPE;
 
-    private final LoggingFacade _configurator;
+    private final LoggingManagementFacade _loggingManagementFacade;
+    private final String[] _allAvailableLogLevels;
 
     static
     {
@@ -77,12 +78,13 @@ public class LoggingManagementMBean extends AMQManagedObject implements LoggingM
             throw new ExceptionInInitializerError(e);
         }
     }
-    
-    public LoggingManagementMBean(LoggingFacade configurator, ManagedObjectRegistry registry) throws JMException
+
+    public LoggingManagementMBean(LoggingManagementFacade loggingManagementFacade, ManagedObjectRegistry registry) throws JMException
     {
         super(LoggingManagement.class, LoggingManagement.TYPE, registry);
         register();
-        _configurator = configurator;
+        _loggingManagementFacade = loggingManagementFacade;
+        _allAvailableLogLevels = buildAllAvailableLoggerLevelsWithInheritedPsuedoLogLevel(_loggingManagementFacade);
     }
 
     @Override
@@ -100,30 +102,26 @@ public class LoggingManagementMBean extends AMQManagedObject implements LoggingM
     @Override
     public Integer getLog4jLogWatchInterval()
     {
-        return _configurator.getLog4jLogWatchInterval();
+        return _loggingManagementFacade.getLog4jLogWatchInterval();
     }
     
     @Override
     public String[] getAvailableLoggerLevels()
     {
-        List<String> levels = _configurator.getAvailableLoggerLevels();
-        List<String> mbeanLevels = new ArrayList<String>(levels);
-        mbeanLevels.add(INHERITED_PSUEDO_LOG_LEVEL);
-        
-        return mbeanLevels.toArray(new String[mbeanLevels.size()]);
+        return _allAvailableLogLevels;
     }
 
     @Override
     public TabularData viewEffectiveRuntimeLoggerLevels()
     {
-        Map<String, String> levels = _configurator.retrieveRuntimeLoggersLevels();
+        Map<String, String> levels = _loggingManagementFacade.retrieveRuntimeLoggersLevels();
         return createTabularDataFromLevelsMap(levels);
     }
 
     @Override
     public String getRuntimeRootLoggerLevel()
     {
-        return _configurator.retrieveRuntimeRootLoggerLevel();
+        return _loggingManagementFacade.retrieveRuntimeRootLoggerLevel();
     }
 
     @Override
@@ -139,7 +137,7 @@ public class LoggingManagementMBean extends AMQManagedObject implements LoggingM
             return false;
         }
 
-        _configurator.setRuntimeRootLoggerLevel(level);
+        _loggingManagementFacade.setRuntimeRootLoggerLevel(level);
         return true;
     }
 
@@ -159,7 +157,7 @@ public class LoggingManagementMBean extends AMQManagedObject implements LoggingM
 
         try
         {
-            _configurator.setRuntimeLoggerLevel(logger, validatedLevel);
+            _loggingManagementFacade.setRuntimeLoggerLevel(logger, validatedLevel);
         }
         catch (LoggingFacadeException e)
         {
@@ -175,7 +173,7 @@ public class LoggingManagementMBean extends AMQManagedObject implements LoggingM
         Map<String,String> levels;
         try
         {
-            levels = _configurator.retrieveConfigFileLoggersLevels();
+            levels = _loggingManagementFacade.retrieveConfigFileLoggersLevels();
         }
         catch (LoggingFacadeException e)
         {
@@ -191,7 +189,7 @@ public class LoggingManagementMBean extends AMQManagedObject implements LoggingM
     {
         try
         {
-            return _configurator.retrieveConfigFileRootLoggerLevel().toUpperCase();
+            return _loggingManagementFacade.retrieveConfigFileRootLoggerLevel().toUpperCase();
         }
         catch (LoggingFacadeException e)
         {
@@ -216,7 +214,7 @@ public class LoggingManagementMBean extends AMQManagedObject implements LoggingM
 
         try
         {
-            _configurator.setConfigFileLoggerLevel(logger, validatedLevel);
+            _loggingManagementFacade.setConfigFileLoggerLevel(logger, validatedLevel);
         }
         catch (LoggingFacadeException e)
         {
@@ -241,7 +239,7 @@ public class LoggingManagementMBean extends AMQManagedObject implements LoggingM
 
         try
         {
-            _configurator.setConfigFileRootLoggerLevel(level);
+            _loggingManagementFacade.setConfigFileRootLoggerLevel(level);
             return true;
         }
         catch (LoggingFacadeException e)
@@ -257,7 +255,7 @@ public class LoggingManagementMBean extends AMQManagedObject implements LoggingM
         try
         {
 
-            _configurator.reload();
+            _loggingManagementFacade.reload();
         }
         catch (LoggingFacadeException e)
         {
@@ -283,9 +281,8 @@ public class LoggingManagementMBean extends AMQManagedObject implements LoggingM
 
     private void validateLevelNotAllowingInherited(String level)
     {
-        final List<String> availableLoggerLevels = _configurator.getAvailableLoggerLevels();
-        if (!availableLoggerLevels.contains(level)
-             && !availableLoggerLevels.contains(String.valueOf(level).toUpperCase()))
+        final List<String> availableLoggerLevels = _loggingManagementFacade.getAvailableLoggerLevels();
+        if (level == null || !availableLoggerLevels.contains(level.toUpperCase()))
         {
             throw new IllegalArgumentException(level + " not known");
         }
@@ -305,7 +302,6 @@ public class LoggingManagementMBean extends AMQManagedObject implements LoggingM
         return loggerLevelList;
     }
 
-
     private CompositeData createRow(String loggerName, String level)
     {
         Object[] itemData = {loggerName, level.toUpperCase()};
@@ -321,4 +317,13 @@ public class LoggingManagementMBean extends AMQManagedObject implements LoggingM
             throw new RuntimeException(ode);
         }
     }
+
+    private String[] buildAllAvailableLoggerLevelsWithInheritedPsuedoLogLevel(LoggingManagementFacade loggingManagementFacade)
+    {
+        List<String> levels = loggingManagementFacade.getAvailableLoggerLevels();
+        List<String> mbeanLevels = new ArrayList<String>(levels);
+        mbeanLevels.add(INHERITED_PSUEDO_LOG_LEVEL);
+
+        return mbeanLevels.toArray(new String[mbeanLevels.size()]);
+    }
 }
diff --git a/java/broker-plugins/management-jmx/src/test/java/org/apache/qpid/server/jmx/mbeans/LoggingManagementMBeanTest.java b/java/broker-plugins/management-jmx/src/test/java/org/apache/qpid/server/jmx/mbeans/LoggingManagementMBeanTest.java
index ae1be5db00..0f33e78d03 100644
--- a/java/broker-plugins/management-jmx/src/test/java/org/apache/qpid/server/jmx/mbeans/LoggingManagementMBeanTest.java
+++ b/java/broker-plugins/management-jmx/src/test/java/org/apache/qpid/server/jmx/mbeans/LoggingManagementMBeanTest.java
@@ -39,7 +39,7 @@ import junit.framework.TestCase;
 
 import org.apache.qpid.management.common.mbeans.LoggingManagement;
 import org.apache.qpid.server.jmx.ManagedObjectRegistry;
-import org.apache.qpid.server.logging.log4j.LoggingFacade;
+import org.apache.qpid.server.logging.log4j.LoggingManagementFacade;
 
 public class LoggingManagementMBeanTest extends TestCase
 {
@@ -47,13 +47,13 @@ public class LoggingManagementMBeanTest extends TestCase
     private static final String TEST_LEVEL2 = "LEVEL2";
 
     private LoggingManagementMBean _loggingMBean;
-    private LoggingFacade _mockLoggingFacade;
+    private LoggingManagementFacade _mockLoggingFacade;
     private ManagedObjectRegistry _mockManagedObjectRegistry;
 
     @Override
     protected void setUp() throws Exception
     {
-        _mockLoggingFacade = mock(LoggingFacade.class);
+        _mockLoggingFacade = mock(LoggingManagementFacade.class);
         final List<String> listOfLevels = new ArrayList<String>()
         {{
             add(TEST_LEVEL1);
diff --git a/java/broker-plugins/management-jmx/src/test/java/org/apache/qpid/systest/management/jmx/LoggingManagementTest.java b/java/broker-plugins/management-jmx/src/test/java/org/apache/qpid/systest/management/jmx/LoggingManagementTest.java
index ac6730638e..0f374b67d5 100644
--- a/java/broker-plugins/management-jmx/src/test/java/org/apache/qpid/systest/management/jmx/LoggingManagementTest.java
+++ b/java/broker-plugins/management-jmx/src/test/java/org/apache/qpid/systest/management/jmx/LoggingManagementTest.java
@@ -26,7 +26,7 @@ import javax.management.openmbean.TabularData;
 
 import org.apache.qpid.management.common.mbeans.LoggingManagement;
 import org.apache.qpid.server.jmx.mbeans.LoggingManagementMBeanTest;
-import org.apache.qpid.server.logging.log4j.LoggingFacadeTest;
+import org.apache.qpid.server.logging.log4j.LoggingManagementFacadeTest;
 import org.apache.qpid.test.utils.JMXTestUtils;
 import org.apache.qpid.test.utils.QpidBrokerTestCase;
 import org.apache.qpid.util.FileUtils;
@@ -37,7 +37,7 @@ import org.apache.qpid.util.LogMonitor;
  * test-profiles/log4j-test.xml</b>.
  *
  * @see LoggingManagementMBeanTest
- * @see LoggingFacadeTest
+ * @see LoggingManagementFacadeTest
  *
  */
 public class LoggingManagementTest extends QpidBrokerTestCase
diff --git a/java/broker/src/main/java/org/apache/qpid/server/Broker.java b/java/broker/src/main/java/org/apache/qpid/server/Broker.java
index e92da34a49..afb8a0029b 100644
--- a/java/broker/src/main/java/org/apache/qpid/server/Broker.java
+++ b/java/broker/src/main/java/org/apache/qpid/server/Broker.java
@@ -36,7 +36,7 @@ import org.apache.qpid.server.logging.SystemOutMessageLogger;
 import org.apache.qpid.server.logging.actors.BrokerActor;
 import org.apache.qpid.server.logging.actors.CurrentActor;
 import org.apache.qpid.server.logging.actors.GenericActor;
-import org.apache.qpid.server.logging.log4j.LoggingFacade;
+import org.apache.qpid.server.logging.log4j.LoggingManagementFacade;
 import org.apache.qpid.server.logging.messages.BrokerMessages;
 import org.apache.qpid.server.protocol.AmqpProtocolVersion;
 import org.apache.qpid.server.protocol.MultiVersionProtocolEngineFactory;
@@ -458,7 +458,7 @@ public class Broker
                 // log4j expects the watch interval in milliseconds
                 try
                 {
-                    LoggingFacade.configureAndWatch(logConfigFile.getPath(), logWatchTime * 1000);
+                    LoggingManagementFacade.configureAndWatch(logConfigFile.getPath(), logWatchTime * 1000);
                 }
                 catch (Exception e)
                 {
@@ -469,7 +469,7 @@ public class Broker
             {
                 try
                 {
-                    LoggingFacade.configure(logConfigFile.getPath());
+                    LoggingManagementFacade.configure(logConfigFile.getPath());
                 }
                 catch (Exception e)
                 {
diff --git a/java/broker/src/main/java/org/apache/qpid/server/logging/log4j/LoggingFacade.java b/java/broker/src/main/java/org/apache/qpid/server/logging/log4j/LoggingManagementFacade.java
index 931171b175..6a961c8fa4 100644
--- a/java/broker/src/main/java/org/apache/qpid/server/logging/log4j/LoggingFacade.java
+++ b/java/broker/src/main/java/org/apache/qpid/server/logging/log4j/LoggingManagementFacade.java
@@ -58,49 +58,49 @@ import java.util.Random;
  * turn {@link Logger} on, off and control their {@link Level}, and the manipulation and reload
  * of the log4j configuration file.
  */
-public class LoggingFacade
+public class LoggingManagementFacade
 {
     private static Logger LOGGER;
-    private static transient LoggingFacade _instance;
+    private static transient LoggingManagementFacade _instance;
     private final String _filename;
     private final int _delay;
 
-    public static LoggingFacade configure(String filename) throws LoggingFacadeException
+    public static LoggingManagementFacade configure(String filename) throws LoggingFacadeException
     {
-        _instance = new LoggingFacade(filename);
+        _instance = new LoggingManagementFacade(filename);
         return _instance;
     }
 
-    public static LoggingFacade configureAndWatch(String filename, int delay) throws LoggingFacadeException
+    public static LoggingManagementFacade configureAndWatch(String filename, int delay) throws LoggingFacadeException
     {
-        _instance = new LoggingFacade(filename, delay);
+        _instance = new LoggingManagementFacade(filename, delay);
         return _instance;
     }
 
-    public static LoggingFacade getCurrentInstance()
+    public static LoggingManagementFacade getCurrentInstance()
     {
         return _instance;
     }
 
-    private LoggingFacade(String filename)
+    private LoggingManagementFacade(String filename)
     {
         DOMConfigurator.configure(filename);
 
         if(LOGGER == null)
         {
-            LOGGER = Logger.getLogger(LoggingFacade.class);
+            LOGGER = Logger.getLogger(LoggingManagementFacade.class);
         }
         _filename = filename;
         _delay = 0;
     }
 
-    private LoggingFacade(String filename, int delay)
+    private LoggingManagementFacade(String filename, int delay)
     {
         DOMConfigurator.configureAndWatch(filename, delay);
 
         if(LOGGER == null)
         {
-            LOGGER = Logger.getLogger(LoggingFacade.class);
+            LOGGER = Logger.getLogger(LoggingManagementFacade.class);
         }
 
         _filename = filename;
diff --git a/java/broker/src/test/java/org/apache/qpid/server/logging/log4j/LoggingFacadeTest.java b/java/broker/src/test/java/org/apache/qpid/server/logging/log4j/LoggingManagementFacadeTest.java
index f871baffe6..6cb9d9b2e6 100644
--- a/java/broker/src/test/java/org/apache/qpid/server/logging/log4j/LoggingFacadeTest.java
+++ b/java/broker/src/test/java/org/apache/qpid/server/logging/log4j/LoggingManagementFacadeTest.java
@@ -28,9 +28,9 @@ import org.apache.qpid.util.FileUtils;
 
 import junit.framework.TestCase;
 
-public class LoggingFacadeTest extends TestCase
+public class LoggingManagementFacadeTest extends TestCase
 {
-    private LoggingFacade _loggingFacade;
+    private LoggingManagementFacade _loggingFacade;
     private String _log4jXmlFile;
 
     @Override
@@ -38,7 +38,7 @@ public class LoggingFacadeTest extends TestCase
     {
         super.setUp();
         _log4jXmlFile = createTestLog4jXml();
-        _loggingFacade = LoggingFacade.configure(_log4jXmlFile);
+        _loggingFacade = LoggingManagementFacade.configure(_log4jXmlFile);
     }
 
     public void testGetAvailableLoggerLevels() throws Exception