From 7ec7467c79427f2ce261642920d373894dd4fe83 Mon Sep 17 00:00:00 2001 From: Maros Marsalek Date: Thu, 21 Nov 2013 09:15:17 +0100 Subject: [PATCH] Fixed YangStore exception thrown at startup Fixed a bug in yang store service that caused cache to be updated at illegal state Change-Id: I567bb245af6577a53a52d003a14b74c9edc5850d Signed-off-by: Maros Marsalek --- .../yang/store/impl/ExtenderYangTracker.java | 93 +++++++++++++------ .../store/impl/YangStoreSnapshotImpl.java | 6 +- .../ExtenderYangTrackerCustomizerTest.java | 30 +++++- 3 files changed, 94 insertions(+), 35 deletions(-) diff --git a/opendaylight/config/yang-store-impl/src/main/java/org/opendaylight/controller/config/yang/store/impl/ExtenderYangTracker.java b/opendaylight/config/yang-store-impl/src/main/java/org/opendaylight/controller/config/yang/store/impl/ExtenderYangTracker.java index 2ead596d05..90b94ef120 100644 --- a/opendaylight/config/yang-store-impl/src/main/java/org/opendaylight/controller/config/yang/store/impl/ExtenderYangTracker.java +++ b/opendaylight/config/yang-store-impl/src/main/java/org/opendaylight/controller/config/yang/store/impl/ExtenderYangTracker.java @@ -18,6 +18,8 @@ import com.google.common.collect.Sets; import org.opendaylight.controller.config.yang.store.api.YangStoreException; import org.opendaylight.controller.config.yang.store.api.YangStoreService; import org.opendaylight.controller.config.yang.store.api.YangStoreSnapshot; +import org.opendaylight.controller.config.yangjmxgenerator.ModuleMXBeanEntry; +import org.opendaylight.yangtools.yang.model.api.Module; import org.osgi.framework.Bundle; import org.osgi.framework.BundleContext; import org.osgi.framework.BundleEvent; @@ -34,6 +36,7 @@ import java.util.Collection; import java.util.Collections; import java.util.Enumeration; import java.util.List; +import java.util.Map; import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -75,7 +78,7 @@ public class ExtenderYangTracker extends BundleTracker implements YangSt } @Override - public synchronized Object addingBundle(Bundle bundle, BundleEvent event) { + public Object addingBundle(Bundle bundle, BundleEvent event) { // Ignore system bundle: // system bundle might have config-api on classpath && @@ -107,37 +110,40 @@ public class ExtenderYangTracker extends BundleTracker implements YangSt proposedNewState.putAll(bundle, addedURLs); Preconditions.checkArgument(addedURLs.size() > 0, "No change can occur when no URLs are changed"); - boolean success; - String failureReason = null; + try(YangStoreSnapshotImpl snapshot = createSnapshot(mbeParser, proposedNewState)) { - updateCache(snapshot); - success = true; + onSnapshotSuccess(proposedNewState, snapshot); } catch(YangStoreException e) { - failureReason = e.toString(); - success = false; - } - if (success){ - // consistent state - // merge into - consistentBundlesToYangURLs.clear(); - consistentBundlesToYangURLs.putAll(proposedNewState); - inconsistentBundlesToYangURLs.clear(); - - logger.info("Yang store updated to new consistent state containing {} yang files", consistentBundlesToYangURLs.size()); - logger.trace("Yang store updated to new consistent state containing {}", consistentBundlesToYangURLs); - } else { - // inconsistent state - logger.debug("Yang store is falling back on last consistent state containing {}, inconsistent yang files {}, reason {}", - consistentBundlesToYangURLs, inconsistentBundlesToYangURLs, failureReason); - logger.warn("Yang store is falling back on last consistent state containing {} files, inconsistent yang files size is {}, reason {}", - consistentBundlesToYangURLs.size(), inconsistentBundlesToYangURLs.size(), failureReason); - inconsistentBundlesToYangURLs.putAll(bundle, addedURLs); + onSnapshotFailure(bundle, addedURLs, e); } } } return bundle; } + private void onSnapshotFailure(Bundle bundle, List addedURLs, Exception failureReason) { + // inconsistent state + inconsistentBundlesToYangURLs.putAll(bundle, addedURLs); + + logger.debug("Yang store is falling back on last consistent state containing {}, inconsistent yang files {}, reason {}", + consistentBundlesToYangURLs, inconsistentBundlesToYangURLs, failureReason); + logger.warn("Yang store is falling back on last consistent state containing {} files, inconsistent yang files size is {}, reason {}", + consistentBundlesToYangURLs.size(), inconsistentBundlesToYangURLs.size(), failureReason.toString()); + } + + private void onSnapshotSuccess(Multimap proposedNewState, YangStoreSnapshotImpl snapshot) { + // consistent state + // merge into + consistentBundlesToYangURLs.clear(); + consistentBundlesToYangURLs.putAll(proposedNewState); + inconsistentBundlesToYangURLs.clear(); + + updateCache(snapshot); + + logger.info("Yang store updated to new consistent state containing {} yang files", consistentBundlesToYangURLs.size()); + logger.debug("Yang store updated to new consistent state containing {}", consistentBundlesToYangURLs); + } + private void updateCache(YangStoreSnapshotImpl snapshot) { cache.cacheYangStore(consistentBundlesToYangURLs, snapshot); } @@ -153,6 +159,7 @@ public class ExtenderYangTracker extends BundleTracker implements YangSt */ @Override public synchronized void removedBundle(Bundle bundle, BundleEvent event, Object object) { + logger.debug("Removed bundle {} {} {}", bundle, event, object); inconsistentBundlesToYangURLs.removeAll(bundle); consistentBundlesToYangURLs.removeAll(bundle); } @@ -162,9 +169,10 @@ public class ExtenderYangTracker extends BundleTracker implements YangSt throws YangStoreException { Optional yangStoreOpt = cache.getSnapshotIfPossible(consistentBundlesToYangURLs); if (yangStoreOpt.isPresent()) { - logger.trace("Returning cached yang store {}", yangStoreOpt.get()); + logger.debug("Returning cached yang store {}", yangStoreOpt.get()); return yangStoreOpt.get(); } + YangStoreSnapshotImpl snapshot = createSnapshot(mbeParser, consistentBundlesToYangURLs); updateCache(snapshot); return snapshot; @@ -205,18 +213,21 @@ public class ExtenderYangTracker extends BundleTracker implements YangSt } class YangStoreCache { + @GuardedBy("this") - private Set cachedUrls = Collections.emptySet(); + private Set cachedUrls = null; @GuardedBy("this") - private Optional cachedYangStoreSnapshot = Optional.absent(); + private Optional cachedYangStoreSnapshot = getInitialSnapshot(); synchronized Optional getSnapshotIfPossible(Multimap bundlesToYangURLs) { Set urls = setFromMultimapValues(bundlesToYangURLs); - if (cachedUrls != null && cachedUrls.equals(urls)) { + + if (cachedUrls==null || cachedUrls.equals(urls)) { Preconditions.checkState(cachedYangStoreSnapshot.isPresent()); YangStoreSnapshot freshSnapshot = new YangStoreSnapshotImpl(cachedYangStoreSnapshot.get()); return Optional.of(freshSnapshot); } + return Optional.absent(); } @@ -228,7 +239,7 @@ class YangStoreCache { } synchronized void cacheYangStore(Multimap urls, - YangStoreSnapshotImpl yangStoreSnapshot) { + YangStoreSnapshot yangStoreSnapshot) { this.cachedUrls = setFromMultimapValues(urls); this.cachedYangStoreSnapshot = Optional.of(yangStoreSnapshot); } @@ -240,4 +251,28 @@ class YangStoreCache { cachedYangStoreSnapshot = Optional.absent(); } } + + private Optional getInitialSnapshot() { + YangStoreSnapshot initialSnapshot = new YangStoreSnapshot() { + @Override + public Map> getModuleMXBeanEntryMap() { + return Collections.emptyMap(); + } + + @Override + public Map> getModuleMap() { + return Collections.emptyMap(); + } + + @Override + public int countModuleMXBeanEntries() { + return 0; + } + + @Override + public void close() { + } + }; + return Optional.of(initialSnapshot); + } } diff --git a/opendaylight/config/yang-store-impl/src/main/java/org/opendaylight/controller/config/yang/store/impl/YangStoreSnapshotImpl.java b/opendaylight/config/yang-store-impl/src/main/java/org/opendaylight/controller/config/yang/store/impl/YangStoreSnapshotImpl.java index 7a5ca7debe..d5936c28ae 100644 --- a/opendaylight/config/yang-store-impl/src/main/java/org/opendaylight/controller/config/yang/store/impl/YangStoreSnapshotImpl.java +++ b/opendaylight/config/yang-store-impl/src/main/java/org/opendaylight/controller/config/yang/store/impl/YangStoreSnapshotImpl.java @@ -29,9 +29,9 @@ public class YangStoreSnapshotImpl implements YangStoreSnapshot { this.moduleMap = Collections.unmodifiableMap(moduleMap); } - public YangStoreSnapshotImpl(YangStoreSnapshotImpl yangStoreSnapshot) { - this.moduleMXBeanEntryMap = yangStoreSnapshot.moduleMXBeanEntryMap; - this.moduleMap = yangStoreSnapshot.moduleMap; + public YangStoreSnapshotImpl(YangStoreSnapshot yangStoreSnapshot) { + this.moduleMXBeanEntryMap = yangStoreSnapshot.getModuleMXBeanEntryMap(); + this.moduleMap = yangStoreSnapshot.getModuleMap(); } @Override diff --git a/opendaylight/config/yang-store-impl/src/test/java/org/opendaylight/controller/config/yang/store/impl/ExtenderYangTrackerCustomizerTest.java b/opendaylight/config/yang-store-impl/src/test/java/org/opendaylight/controller/config/yang/store/impl/ExtenderYangTrackerCustomizerTest.java index c4c523992f..b4054c242c 100644 --- a/opendaylight/config/yang-store-impl/src/test/java/org/opendaylight/controller/config/yang/store/impl/ExtenderYangTrackerCustomizerTest.java +++ b/opendaylight/config/yang-store-impl/src/test/java/org/opendaylight/controller/config/yang/store/impl/ExtenderYangTrackerCustomizerTest.java @@ -9,12 +9,14 @@ package org.opendaylight.controller.config.yang.store.impl; import com.google.common.base.Optional; import com.google.common.collect.Lists; +import com.google.common.collect.Maps; import org.junit.Before; import org.junit.Test; import org.mockito.Mock; import org.mockito.MockitoAnnotations; import org.opendaylight.controller.config.yang.store.api.YangStoreException; import org.opendaylight.controller.config.yang.store.api.YangStoreSnapshot; +import org.opendaylight.yangtools.yang.model.api.Module; import org.osgi.framework.Bundle; import org.osgi.framework.BundleContext; import org.osgi.framework.BundleListener; @@ -25,6 +27,7 @@ import java.net.URL; import java.util.Collections; import java.util.Enumeration; import java.util.List; +import java.util.Map; import java.util.regex.Pattern; import static org.junit.Assert.assertEquals; @@ -48,8 +51,28 @@ public class ExtenderYangTrackerCustomizerTest { @Mock private BundleContext bundleContext; + private Map> moduleMap = Maps.newHashMap(); + @Before public void setUp() throws YangStoreException { + + moduleMap.put("1", new Map.Entry() { + @Override + public Module getKey() { + return mock(Module.class); + } + + @Override + public String getValue() { + return "v"; + } + + @Override + public String setValue(String value) { + return "v"; + } + }); + MockitoAnnotations.initMocks(this); doNothing().when(bundleContext).addBundleListener(any(BundleListener.class)); doReturn(new Bundle[0]).when(bundleContext).getBundles(); @@ -59,7 +82,8 @@ public class ExtenderYangTrackerCustomizerTest { doReturn(22).when(yangStoreSnapshot).countModuleMXBeanEntries(); doReturn("mock yang store").when(yangStoreSnapshot).toString(); doNothing().when(yangStoreSnapshot).close(); - doReturn(Collections.emptyMap()).when(yangStoreSnapshot).getModuleMap(); + doReturn(moduleMap).when(yangStoreSnapshot).getModuleMap(); + doReturn(Collections.emptyMap()).when(yangStoreSnapshot).getModuleMXBeanEntryMap(); } @Test @@ -75,7 +99,7 @@ public class ExtenderYangTrackerCustomizerTest { returnedStore = tested.getYangStoreSnapshot(); - assertEquals(yangStoreSnapshot, returnedStore); + assertEquals(yangStoreSnapshot.getModuleMap(), returnedStore.getModuleMap()); tested.removedBundle(bundle, null, null); tested.getYangStoreSnapshot(); @@ -87,7 +111,7 @@ public class ExtenderYangTrackerCustomizerTest { tested.getYangStoreSnapshot(); } - verify(parser, times(7)).parseYangFiles(anyCollectionOf(InputStream.class)); + verify(parser, times(5)).parseYangFiles(anyCollectionOf(InputStream.class)); returnedStore = tested.getYangStoreSnapshot(); -- 2.36.6