Bug 4560: Improve config system logging for debuggability 16/29216/2
authorTom Pantelis <tpanteli@brocade.com>
Tue, 3 Nov 2015 15:06:27 +0000 (10:06 -0500)
committerGerrit Code Review <gerrit@opendaylight.org>
Tue, 10 Nov 2015 12:24:19 +0000 (12:24 +0000)
Manually cherry-picked from
https://git.opendaylight.org/gerrit/#/c/28985 as the files have moved in
master.

Also the code has changed slightly in master, specifically the
ConfigPusherImplTest no longer uses a Thread uncaught exception handler
for verification. However it does rely on exceptions thrown from the
ConfigPusherImpl so, to keep the same behavior, I added a
propagateExceptions flag to ConfigPusherImpl#process. The
ConfigPersisterActivator production code passes false so unchecked
exceptions aren't handled as uncaught exceptions.

Change-Id: Iabc22030abc22cf11a1476986ba3d3366021b4fb
Signed-off-by: Tom Pantelis <tpanteli@brocade.com>
opendaylight/config/config-persister-impl/src/main/java/org/opendaylight/controller/config/persist/impl/ConfigPusherImpl.java
opendaylight/config/config-persister-impl/src/main/java/org/opendaylight/controller/config/persist/impl/osgi/ConfigPersisterActivator.java
opendaylight/config/config-persister-impl/src/test/java/org/opendaylight/controller/config/persist/impl/ConfigPusherImplTest.java

index 77f355e..90f71ca 100644 (file)
@@ -69,13 +69,15 @@ public class ConfigPusherImpl implements ConfigPusher {
         this.facade = facade;
     }
 
-    public void process(List<AutoCloseable> autoCloseables, MBeanServerConnection platformMBeanServer, Persister persisterAggregator) throws InterruptedException {
-        while(true) {
-            processSingle(autoCloseables, platformMBeanServer, persisterAggregator);
+    public void process(List<AutoCloseable> autoCloseables, MBeanServerConnection platformMBeanServer,
+            Persister persisterAggregator, boolean propagateExceptions) throws InterruptedException {
+        while(processSingle(autoCloseables, platformMBeanServer, persisterAggregator, propagateExceptions)) {
+            ;
         }
     }
 
-    void processSingle(final List<AutoCloseable> autoCloseables, final MBeanServerConnection platformMBeanServer, final Persister persisterAggregator) throws InterruptedException {
+    boolean processSingle(final List<AutoCloseable> autoCloseables, final MBeanServerConnection platformMBeanServer,
+            final Persister persisterAggregator, boolean propagateExceptions) throws InterruptedException {
         final List<? extends ConfigSnapshotHolder> configs = queue.take();
         try {
             internalPushConfigs(configs);
@@ -90,10 +92,22 @@ public class ConfigPusherImpl implements ConfigPusher {
             }
 
             LOG.debug("ConfigPusher has pushed configs {}", configs);
-        } catch (DocumentedException e) {
-            LOG.error("Error pushing configs {}",configs);
-            throw new IllegalStateException(e);
+        } catch (Exception e) {
+            // Exceptions are logged to error downstream
+            LOG.debug("Failed to push some of configs: {}", configs, e);
+
+            if(propagateExceptions) {
+                if(e instanceof RuntimeException) {
+                    throw (RuntimeException)e;
+                } else {
+                    throw new IllegalStateException(e);
+                }
+            } else {
+                return false;
+            }
         }
+
+        return true;
     }
 
     @Override
@@ -109,15 +123,21 @@ public class ConfigPusherImpl implements ConfigPusher {
         // start pushing snapshots
         for (ConfigSnapshotHolder configSnapshotHolder : configs) {
             if (configSnapshotHolder != null) {
+                LOG.info("Pushing configuration snapshot {}", configSnapshotHolder);
                 boolean pushResult = false;
                 try {
                     pushResult = pushConfigWithConflictingVersionRetries(configSnapshotHolder);
                 } catch (ConfigSnapshotFailureException e) {
-                    LOG.warn("Failed to apply configuration snapshot: {}. Config snapshot is not semantically correct and will be IGNORED. " +
+                    LOG.error("Failed to apply configuration snapshot: {}. Config snapshot is not semantically correct and will be IGNORED. " +
                             "for detailed information see enclosed exception.", e.getConfigIdForReporting(), e);
                     throw new IllegalStateException("Failed to apply configuration snapshot " + e.getConfigIdForReporting(), e);
+                }  catch (Exception e) {
+                    String msg = String.format("Failed to apply configuration snapshot: %s", configSnapshotHolder);
+                    LOG.error(msg, e);
+                    throw new IllegalStateException(msg, e);
                 }
-                LOG.debug("Config snapshot pushed successfully: {}, result: {}", configSnapshotHolder, result);
+
+                LOG.info("Successfully pushed configuration snapshot {}", configSnapshotHolder);
                 result.put(configSnapshotHolder, pushResult);
             }
         }
index ef53ecd..5f25d6c 100644 (file)
@@ -128,7 +128,7 @@ public class ConfigPersisterActivator implements BundleActivator {
                         }
                         if(context != null) {
                             registration = context.registerService(ConfigPusher.class.getName(), configPusher, null);
-                            configPusher.process(autoCloseables, platformMBeanServer, persisterAggregator);
+                            configPusher.process(autoCloseables, platformMBeanServer, persisterAggregator, false);
                         } else {
                             LOG.warn("Unable to process configs as BundleContext is null");
                         }
index 3583e32..8bfb3d7 100644 (file)
@@ -8,7 +8,6 @@
 package org.opendaylight.controller.config.persist.impl;
 
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 import static org.mockito.Matchers.any;
@@ -20,7 +19,7 @@ import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
-
+import com.google.common.base.Throwables;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Sets;
 import java.lang.management.ManagementFactory;
@@ -92,11 +91,12 @@ public class ConfigPusherImplTest {
 
         configPusher.pushConfigs(Collections.singletonList(mockedConfigSnapshot));
         try {
-            configPusher.process(Lists.<AutoCloseable>newArrayList(), ManagementFactory.getPlatformMBeanServer(), mockedAggregator);
+            configPusher.process(Lists.<AutoCloseable>newArrayList(), ManagementFactory.getPlatformMBeanServer(),
+                    mockedAggregator, true);
         } catch(IllegalStateException e) {
-            assertNotNull(e.getCause());
-            assertTrue(e.getCause() instanceof ConfigPusherImpl.NotEnoughCapabilitiesException);
-            final Set<String> missingCaps = ((ConfigPusherImpl.NotEnoughCapabilitiesException) e.getCause()).getMissingCaps();
+            Throwable cause = Throwables.getRootCause(e);
+            assertTrue(cause instanceof ConfigPusherImpl.NotEnoughCapabilitiesException);
+            final Set<String> missingCaps = ((ConfigPusherImpl.NotEnoughCapabilitiesException) cause).getMissingCaps();
             assertEquals(missingCaps.size(), 1);
             assertEquals(missingCaps.iterator().next(), "required-cap");
             return;
@@ -121,7 +121,7 @@ public class ConfigPusherImplTest {
         final ConfigPusherImpl configPusher = new ConfigPusherImpl(facadeFactory, 0, 0);
 
         configPusher.pushConfigs(Collections.singletonList(mockedConfigSnapshot));
-        configPusher.processSingle(Lists.<AutoCloseable>newArrayList(), mBeanServer, mockedAggregator);
+        configPusher.processSingle(Lists.<AutoCloseable>newArrayList(), mBeanServer, mockedAggregator, true);
 
         verify(facade).executeConfigExecution(cfgExec);
         verify(facade).commitSilentTransaction();
@@ -144,10 +144,10 @@ public class ConfigPusherImplTest {
 
         configPusher.pushConfigs(Collections.singletonList(mockedConfigSnapshot));
         try {
-            configPusher.processSingle(Lists.<AutoCloseable>newArrayList(), mBeanServer, mockedAggregator);
+            configPusher.processSingle(Lists.<AutoCloseable>newArrayList(), mBeanServer, mockedAggregator, true);
         } catch (IllegalStateException e) {
-            assertNotNull(e.getCause());
-            assertTrue(e.getCause() instanceof ConflictingVersionException);
+            Throwable cause = Throwables.getRootCause(e);
+            assertTrue(cause instanceof ConflictingVersionException);
             return;
         }
 
@@ -174,7 +174,7 @@ public class ConfigPusherImplTest {
         final ConfigPusherImpl configPusher = new ConfigPusherImpl(facadeFactory, 5000, 5000);
 
         configPusher.pushConfigs(Collections.singletonList(mockedConfigSnapshot));
-        configPusher.processSingle(Lists.<AutoCloseable>newArrayList(), mBeanServer, mockedAggregator);
+        configPusher.processSingle(Lists.<AutoCloseable>newArrayList(), mBeanServer, mockedAggregator, true);
 
         verify(facade, times(3)).executeConfigExecution(cfgExec);
         verify(facade, times(3)).commitSilentTransaction();

©2013 OpenDaylight, A Linux Foundation Collaborative Project. All Rights Reserved.
OpenDaylight is a registered trademark of The OpenDaylight Project, Inc.
Linux Foundation and OpenDaylight are registered trademarks of the Linux Foundation.
Linux is a registered trademark of Linus Torvalds.