From: Tom Pantelis Date: Tue, 3 Nov 2015 15:06:27 +0000 (-0500) Subject: Bug 4560: Improve config system logging for debuggability X-Git-Tag: release/beryllium~188 X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?p=controller.git;a=commitdiff_plain;h=edc7fa3aa93bf109acb36c6f2c69a19cf8e38af2 Bug 4560: Improve config system logging for debuggability 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 --- diff --git a/opendaylight/config/config-persister-impl/src/main/java/org/opendaylight/controller/config/persist/impl/ConfigPusherImpl.java b/opendaylight/config/config-persister-impl/src/main/java/org/opendaylight/controller/config/persist/impl/ConfigPusherImpl.java index 77f355e0b2..90f71ca51a 100644 --- a/opendaylight/config/config-persister-impl/src/main/java/org/opendaylight/controller/config/persist/impl/ConfigPusherImpl.java +++ b/opendaylight/config/config-persister-impl/src/main/java/org/opendaylight/controller/config/persist/impl/ConfigPusherImpl.java @@ -69,13 +69,15 @@ public class ConfigPusherImpl implements ConfigPusher { this.facade = facade; } - public void process(List autoCloseables, MBeanServerConnection platformMBeanServer, Persister persisterAggregator) throws InterruptedException { - while(true) { - processSingle(autoCloseables, platformMBeanServer, persisterAggregator); + public void process(List autoCloseables, MBeanServerConnection platformMBeanServer, + Persister persisterAggregator, boolean propagateExceptions) throws InterruptedException { + while(processSingle(autoCloseables, platformMBeanServer, persisterAggregator, propagateExceptions)) { + ; } } - void processSingle(final List autoCloseables, final MBeanServerConnection platformMBeanServer, final Persister persisterAggregator) throws InterruptedException { + boolean processSingle(final List autoCloseables, final MBeanServerConnection platformMBeanServer, + final Persister persisterAggregator, boolean propagateExceptions) throws InterruptedException { final List 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); } } diff --git a/opendaylight/config/config-persister-impl/src/main/java/org/opendaylight/controller/config/persist/impl/osgi/ConfigPersisterActivator.java b/opendaylight/config/config-persister-impl/src/main/java/org/opendaylight/controller/config/persist/impl/osgi/ConfigPersisterActivator.java index ef53ecd8f2..5f25d6c3b2 100644 --- a/opendaylight/config/config-persister-impl/src/main/java/org/opendaylight/controller/config/persist/impl/osgi/ConfigPersisterActivator.java +++ b/opendaylight/config/config-persister-impl/src/main/java/org/opendaylight/controller/config/persist/impl/osgi/ConfigPersisterActivator.java @@ -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"); } diff --git a/opendaylight/config/config-persister-impl/src/test/java/org/opendaylight/controller/config/persist/impl/ConfigPusherImplTest.java b/opendaylight/config/config-persister-impl/src/test/java/org/opendaylight/controller/config/persist/impl/ConfigPusherImplTest.java index 3583e32e26..8bfb3d73b9 100644 --- a/opendaylight/config/config-persister-impl/src/test/java/org/opendaylight/controller/config/persist/impl/ConfigPusherImplTest.java +++ b/opendaylight/config/config-persister-impl/src/test/java/org/opendaylight/controller/config/persist/impl/ConfigPusherImplTest.java @@ -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.newArrayList(), ManagementFactory.getPlatformMBeanServer(), mockedAggregator); + configPusher.process(Lists.newArrayList(), ManagementFactory.getPlatformMBeanServer(), + mockedAggregator, true); } catch(IllegalStateException e) { - assertNotNull(e.getCause()); - assertTrue(e.getCause() instanceof ConfigPusherImpl.NotEnoughCapabilitiesException); - final Set missingCaps = ((ConfigPusherImpl.NotEnoughCapabilitiesException) e.getCause()).getMissingCaps(); + Throwable cause = Throwables.getRootCause(e); + assertTrue(cause instanceof ConfigPusherImpl.NotEnoughCapabilitiesException); + final Set 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.newArrayList(), mBeanServer, mockedAggregator); + configPusher.processSingle(Lists.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.newArrayList(), mBeanServer, mockedAggregator); + configPusher.processSingle(Lists.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.newArrayList(), mBeanServer, mockedAggregator); + configPusher.processSingle(Lists.newArrayList(), mBeanServer, mockedAggregator, true); verify(facade, times(3)).executeConfigExecution(cfgExec); verify(facade, times(3)).commitSilentTransaction();