From: Maros Marsalek Date: Wed, 3 Dec 2014 15:23:24 +0000 (+0100) Subject: BUG-2283 Fix close order when reconfiguring config modules. X-Git-Tag: release/lithium~691 X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?p=controller.git;a=commitdiff_plain;h=3f130bc99b874e917fe397c4835f22229f60842a BUG-2283 Fix close order when reconfiguring config modules. Close is called from top(dependency source) to bottom(dependency target) while createInstance is called in opposite direction. This commit changes API and SPI of config subsystem and thus breaks runtime compatibility. Change-Id: I5b03f0673c5ecb95efbfccad7fa6ed7a490ff61b Signed-off-by: Maros Marsalek --- diff --git a/opendaylight/config/config-api/src/main/java/org/opendaylight/controller/config/api/DependencyResolver.java b/opendaylight/config/config-api/src/main/java/org/opendaylight/controller/config/api/DependencyResolver.java index 40ff7e1703..466f1ed60c 100644 --- a/opendaylight/config/config-api/src/main/java/org/opendaylight/controller/config/api/DependencyResolver.java +++ b/opendaylight/config/config-api/src/main/java/org/opendaylight/controller/config/api/DependencyResolver.java @@ -51,6 +51,7 @@ public interface DependencyResolver extends Identifiable { T resolveInstance(Class expectedType, ObjectName objectName, JmxAttribute jmxAttribute); + /** * To be used during commit phase to resolve identity-ref config attributes. * @@ -86,4 +87,12 @@ public interface DependencyResolver extends Identifiable { */ T newMXBeanProxy(ObjectName objectName, Class interfaceClass); + /** + * Check whether a dependency will be reused or (re)created. Useful when deciding if current module could be also reused. + * + * @param objectName ObjectName ID of a dependency + * @param jmxAttribute JMXAttribute ID of a dependency + * @return true if the dependency will be reused false otherwise + */ + boolean canReuseDependency(ObjectName objectName, JmxAttribute jmxAttribute); } diff --git a/opendaylight/config/config-api/src/main/java/org/opendaylight/controller/config/spi/AbstractModule.java b/opendaylight/config/config-api/src/main/java/org/opendaylight/controller/config/spi/AbstractModule.java new file mode 100644 index 0000000000..772bacce89 --- /dev/null +++ b/opendaylight/config/config-api/src/main/java/org/opendaylight/controller/config/spi/AbstractModule.java @@ -0,0 +1,119 @@ +package org.opendaylight.controller.config.spi; + +import org.opendaylight.controller.config.api.DependencyResolver; +import org.opendaylight.controller.config.api.ModuleIdentifier; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Base implementation of Module. This implementation contains base logic for Module reconfiguration with associated fields. + * @param Type of module implementation. Enables easier implementation for the {@link #isSame} method + */ +public abstract class AbstractModule> implements org.opendaylight.controller.config.spi.Module { + + private static final Logger LOG = LoggerFactory.getLogger(AbstractModule.class); + + private final M oldModule; + private final AutoCloseable oldInstance; + protected final ModuleIdentifier identifier; + private AutoCloseable instance; + protected final DependencyResolver dependencyResolver; + + /** + * Called when module is configured. + * + * @param identifier id of current instance. + * @param dependencyResolver resolver used in dependency injection and validation. + */ + public AbstractModule(ModuleIdentifier identifier, DependencyResolver dependencyResolver) { + this(identifier, dependencyResolver, null, null); + } + + /** + * Called when module is reconfigured. + * + * @param identifier id of current instance. + * @param dependencyResolver resolver used in dependency injection and validation. + * @param oldModule old instance of module that is being reconfigred(replaced) by current instance. The old instance can be examined for reuse. + * @param oldInstance old instance wrapped by the old module. This is the resource that is actually being reused if possible or closed otherwise. + */ + public AbstractModule(ModuleIdentifier identifier, DependencyResolver dependencyResolver, M oldModule, AutoCloseable oldInstance) { + this.identifier = identifier; + this.dependencyResolver = dependencyResolver; + this.oldModule = oldModule; + this.oldInstance = oldInstance; + } + + @Override + public ModuleIdentifier getIdentifier() { + return identifier; + } + + /** + * + * General algorithm for spawning/closing and reusing wrapped instances. + * + * @return current instance of wrapped resource either by reusing the old one (if present) or constructing a brand new. + */ + @Override + public final AutoCloseable getInstance() { + if(instance==null) { + if(oldInstance!=null && canReuseInstance(oldModule)) { + resolveDependencies(); + instance = reuseInstance(oldInstance); + } else { + if(oldInstance!=null) { + try { + oldInstance.close(); + } catch(Exception e) { + LOG.error("An error occurred while closing old instance {} for module {}", oldInstance, getIdentifier(), e); + } + } + resolveDependencies(); + instance = createInstance(); + if (instance == null) { + throw new IllegalStateException("Error in createInstance - null is not allowed as return value. Module: " + getIdentifier()); + } + } + } + return instance; + } + + /** + * @return Brand new instance of wrapped class in case no previous instance is present or reconfiguration is impossible. + */ + protected abstract AutoCloseable createInstance(); + + @Override + public final boolean canReuse(Module oldModule) { + // Just cast into a specific instance + // TODO unify this method with canReuseInstance (required Module interface to be generic which requires quite a lot of changes) + return getClass().isInstance(oldModule) ? canReuseInstance((M) oldModule) : false; + } + + /** + * + * Users are welcome to override this method to provide custom logic for advanced reusability detection. + * + * @param oldModule old instance of a Module + * @return true if the old instance is reusable false if a new one should be spawned + */ + protected abstract boolean canReuseInstance(final M oldModule); + + /** + * By default the oldInstance is returned since this method is by default called only if the oldModule had the same configuration and dependencies configured. + * Users are welcome to override this method to provide custom logic for advanced reusability. + * + * @param oldInstance old instance of a class wrapped by the module + * @return reused instance + */ + protected AutoCloseable reuseInstance(AutoCloseable oldInstance) { + // implement if instance reuse should be supported. Override canReuseInstance to change the criteria. + return oldInstance; + } + + /** + * Inject all the dependencies using dependency resolver instance. + */ + protected abstract void resolveDependencies(); +} diff --git a/opendaylight/config/config-api/src/main/java/org/opendaylight/controller/config/spi/Module.java b/opendaylight/config/config-api/src/main/java/org/opendaylight/controller/config/spi/Module.java index b557108d4d..53f03a2398 100644 --- a/opendaylight/config/config-api/src/main/java/org/opendaylight/controller/config/spi/Module.java +++ b/opendaylight/config/config-api/src/main/java/org/opendaylight/controller/config/spi/Module.java @@ -54,4 +54,17 @@ public interface Module extends Identifiable{ */ AutoCloseable getInstance(); + + /** + * Compare current module with oldModule and if the instance/live object + * produced by the old module can be reused in this module as well return true. + * Typically true should be returned if the old module had the same configuration. + * + * + * @param oldModule old instance of Module + * @return true if the instance produced by oldModule can be reused with current instance as well. + */ + public boolean canReuse(Module oldModule); + + } diff --git a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/ConfigRegistryImpl.java b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/ConfigRegistryImpl.java index 96739fb822..91c67b8b02 100644 --- a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/ConfigRegistryImpl.java +++ b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/ConfigRegistryImpl.java @@ -430,8 +430,7 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe } } - // destroy all live objects one after another in order of the dependency - // hierarchy + // destroy all live objects one after another in order of the dependency hierarchy, from top to bottom List destroyedModules = currentConfig .getModulesToBeDestroyed(); for (DestroyedModule destroyedModule : destroyedModules) { diff --git a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/ConfigTransactionControllerImpl.java b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/ConfigTransactionControllerImpl.java index bb61c4acc7..37c2e2d777 100644 --- a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/ConfigTransactionControllerImpl.java +++ b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/ConfigTransactionControllerImpl.java @@ -10,6 +10,7 @@ package org.opendaylight.controller.config.manager.impl; import static com.google.common.base.Preconditions.checkNotNull; import static java.lang.String.format; +import com.google.common.collect.Lists; import java.util.ArrayList; import java.util.Collection; import java.util.HashSet; @@ -383,32 +384,34 @@ class ConfigTransactionControllerImpl implements LOG.trace("Committing transaction {}", getTransactionIdentifier()); - // call getInstance() - for (Entry entry : dependencyResolverManager - .getAllModules().entrySet()) { - Module module = entry.getValue(); - ModuleIdentifier name = entry.getKey(); + Map allModules = dependencyResolverManager.getAllModules(); + + // call getInstance() on all Modules from top to bottom (from source to target of the dependency relation) + // The source of a dependency closes itself and calls getInstance recursively on the dependencies (in case of reconfiguration) + // This makes close() calls from top to bottom while createInstance() calls are performed bottom to top + List sortedModuleIdentifiers = Lists.reverse(dependencyResolverManager.getSortedModuleIdentifiers()); + for (ModuleIdentifier moduleIdentifier : sortedModuleIdentifiers) { + Module module = allModules.get(moduleIdentifier); + try { LOG.debug("About to commit {} in transaction {}", - name, getTransactionIdentifier()); + moduleIdentifier, getTransactionIdentifier()); AutoCloseable instance = module.getInstance(); - checkNotNull(instance, "Instance is null:{} in transaction {}", name, getTransactionIdentifier()); + checkNotNull(instance, "Instance is null:{} in transaction {}", moduleIdentifier, getTransactionIdentifier()); } catch (Exception e) { - LOG.error("Commit failed on {} in transaction {}", name, + LOG.error("Commit failed on {} in transaction {}", moduleIdentifier, getTransactionIdentifier(), e); internalAbort(); throw new IllegalStateException( format("Error - getInstance() failed for %s in transaction %s", - name, getTransactionIdentifier()), e); + moduleIdentifier, getTransactionIdentifier()), e); } } - // count dependency order - LOG.trace("Committed configuration {}", getTransactionIdentifier()); transactionStatus.setCommitted(); - return dependencyResolverManager.getSortedModuleIdentifiers(); + return sortedModuleIdentifiers; } @Override diff --git a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/dependencyresolver/DependencyResolverImpl.java b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/dependencyresolver/DependencyResolverImpl.java index 0741dab69d..024518ca98 100644 --- a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/dependencyresolver/DependencyResolverImpl.java +++ b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/dependencyresolver/DependencyResolverImpl.java @@ -9,6 +9,7 @@ package org.opendaylight.controller.config.manager.impl.dependencyresolver; import static java.lang.String.format; +import com.google.common.base.Preconditions; import java.util.HashSet; import java.util.LinkedHashSet; import java.util.Set; @@ -151,28 +152,17 @@ final class DependencyResolverImpl implements DependencyResolver, @Override public T resolveInstance(Class expectedType, ObjectName dependentReadOnlyON, JmxAttribute jmxAttribute) { - if (expectedType == null || dependentReadOnlyON == null || jmxAttribute == null) { - throw new IllegalArgumentException(format( - "Null parameters not allowed, got %s %s %s", expectedType, - dependentReadOnlyON, jmxAttribute)); - } - ObjectName translatedDependentReadOnlyON = translateServiceRefIfPossible(dependentReadOnlyON); - transactionStatus.checkCommitStarted(); - transactionStatus.checkNotCommitted(); + Module module = resolveModuleInstance(dependentReadOnlyON, jmxAttribute); - ModuleIdentifier dependentModuleIdentifier = ObjectNameUtil.fromON( - translatedDependentReadOnlyON, ObjectNameUtil.TYPE_MODULE); - Module module = modulesHolder.findModule(dependentModuleIdentifier, - jmxAttribute); synchronized (this) { - dependencies.add(dependentModuleIdentifier); + dependencies.add(module.getIdentifier()); } AutoCloseable instance = module.getInstance(); if (instance == null) { String message = format( "Error while %s resolving instance %s. getInstance() returned null. " + "Expected type %s , attribute %s", name, - dependentModuleIdentifier, expectedType, jmxAttribute + module.getIdentifier(), expectedType, jmxAttribute ); throw new JmxAttributeValidationException(message, jmxAttribute); } @@ -188,6 +178,36 @@ final class DependencyResolverImpl implements DependencyResolver, } } + private Module resolveModuleInstance(ObjectName dependentReadOnlyON, + JmxAttribute jmxAttribute) { + Preconditions.checkArgument(dependentReadOnlyON != null ,"dependentReadOnlyON"); + Preconditions.checkArgument(jmxAttribute != null, "jmxAttribute"); + ObjectName translatedDependentReadOnlyON = translateServiceRefIfPossible(dependentReadOnlyON); + transactionStatus.checkCommitStarted(); + transactionStatus.checkNotCommitted(); + + ModuleIdentifier dependentModuleIdentifier = ObjectNameUtil.fromON( + translatedDependentReadOnlyON, ObjectNameUtil.TYPE_MODULE); + + return Preconditions.checkNotNull(modulesHolder.findModule(dependentModuleIdentifier, jmxAttribute)); + } + + @Override + public boolean canReuseDependency(ObjectName objectName, JmxAttribute jmxAttribute) { + Preconditions.checkNotNull(objectName); + Preconditions.checkNotNull(jmxAttribute); + + Module currentModule = resolveModuleInstance(objectName, jmxAttribute); + ModuleIdentifier identifier = currentModule.getIdentifier(); + ModuleInternalTransactionalInfo moduleInternalTransactionalInfo = modulesHolder.findModuleInternalTransactionalInfo(identifier); + + if(moduleInternalTransactionalInfo.hasOldModule()) { + Module oldModule = moduleInternalTransactionalInfo.getOldInternalInfo().getReadableModule().getModule(); + return currentModule.canReuse(oldModule); + } + return false; + } + @Override public Class resolveIdentity(IdentityAttributeRef identityRef, Class expectedBaseClass) { final QName qName = QName.create(identityRef.getqNameOfIdentity()); @@ -217,7 +237,7 @@ final class DependencyResolverImpl implements DependencyResolver, @Override public int compareTo(DependencyResolverImpl o) { - transactionStatus.checkCommitted(); + transactionStatus.checkCommitStarted(); return Integer.compare(getMaxDependencyDepth(), o.getMaxDependencyDepth()); } @@ -232,7 +252,11 @@ final class DependencyResolverImpl implements DependencyResolver, } void countMaxDependencyDepth(DependencyResolverManager manager) { - transactionStatus.checkCommitted(); + // We can calculate the dependency after second phase commit was started + // Second phase commit starts after validation and validation adds the dependencies into the dependency resolver, which are necessary for the calculation + // FIXME generated code for abstract module declares validate method as non-final + // Overriding the validate would cause recreate every time instead of reuse + also possibly wrong close order if there is another module depending + transactionStatus.checkCommitStarted(); if (maxDependencyDepth == null) { maxDependencyDepth = getMaxDepth(this, manager, new LinkedHashSet()); diff --git a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/dependencyresolver/DependencyResolverManager.java b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/dependencyresolver/DependencyResolverManager.java index da6349ac53..0014a5924d 100644 --- a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/dependencyresolver/DependencyResolverManager.java +++ b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/dependencyresolver/DependencyResolverManager.java @@ -88,7 +88,7 @@ public class DependencyResolverManager implements DependencyResolverFactory, Aut * things? */ private List getAllSorted() { - transactionStatus.checkCommitted(); + transactionStatus.checkCommitStarted(); List sorted = new ArrayList<>( moduleIdentifiersToDependencyResolverMap.values()); for (DependencyResolverImpl dri : sorted) { diff --git a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/dependencyresolver/ModuleInternalTransactionalInfo.java b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/dependencyresolver/ModuleInternalTransactionalInfo.java index 26d8c74fb3..210b6d2aa7 100644 --- a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/dependencyresolver/ModuleInternalTransactionalInfo.java +++ b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/dependencyresolver/ModuleInternalTransactionalInfo.java @@ -7,6 +7,7 @@ */ package org.opendaylight.controller.config.manager.impl.dependencyresolver; +import com.google.common.base.Preconditions; import javax.annotation.Nullable; import org.opendaylight.controller.config.api.ModuleIdentifier; import org.opendaylight.controller.config.manager.impl.ModuleInternalInfo; @@ -70,10 +71,7 @@ public class ModuleInternalTransactionalInfo implements Identifiable sortedModuleIdentifiers = tested .getSortedModuleIdentifiers(); assertEquals( @@ -109,6 +112,7 @@ public class DependencyResolverManagerTest extends AbstractLockedPlatformMBeanSe private static Module mockedModule() { Module mockedModule = mock(Module.class); doReturn(mock(AutoCloseable.class)).when(mockedModule).getInstance(); + doReturn(new ModuleIdentifier("fact", "instance")).when(mockedModule).getIdentifier(); return mockedModule; } diff --git a/opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/testingservices/parallelapsp/TestingParallelAPSPModule.java b/opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/testingservices/parallelapsp/TestingParallelAPSPModule.java index d25956b774..1857aaa22f 100644 --- a/opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/testingservices/parallelapsp/TestingParallelAPSPModule.java +++ b/opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/testingservices/parallelapsp/TestingParallelAPSPModule.java @@ -146,6 +146,11 @@ public class TestingParallelAPSPModule implements Module, return instance; } + @Override + public boolean canReuse(final Module oldModule) { + return false; + } + @Override public ModuleIdentifier getIdentifier() { return identifier; diff --git a/opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/testingservices/parallelapsp/test/MockedDependenciesTest.java b/opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/testingservices/parallelapsp/test/MockedDependenciesTest.java index 7deade5de2..95de90d3af 100644 --- a/opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/testingservices/parallelapsp/test/MockedDependenciesTest.java +++ b/opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/testingservices/parallelapsp/test/MockedDependenciesTest.java @@ -73,6 +73,11 @@ public class MockedDependenciesTest extends AbstractParallelAPSPTest { } + @Override + public boolean canReuse(Module oldModule) { + return false; + } + @Override public Closeable getInstance() { return new MockedThreadPool(threadCount); diff --git a/opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/testingservices/scheduledthreadpool/TestingScheduledThreadPoolModule.java b/opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/testingservices/scheduledthreadpool/TestingScheduledThreadPoolModule.java index 1811e7dd94..e35509d090 100644 --- a/opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/testingservices/scheduledthreadpool/TestingScheduledThreadPoolModule.java +++ b/opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/testingservices/scheduledthreadpool/TestingScheduledThreadPoolModule.java @@ -61,6 +61,11 @@ public class TestingScheduledThreadPoolModule implements Module, "Parameter 'ThreadCount' must be greater than 0"); } + @Override + public boolean canReuse(final Module oldModule) { + return false; + } + @Override public int getThreadCount() { return threadCount; diff --git a/opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/testingservices/threadpool/TestingFixedThreadPoolModule.java b/opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/testingservices/threadpool/TestingFixedThreadPoolModule.java index 464f786e6f..c773dd232b 100644 --- a/opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/testingservices/threadpool/TestingFixedThreadPoolModule.java +++ b/opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/testingservices/threadpool/TestingFixedThreadPoolModule.java @@ -69,6 +69,11 @@ public class TestingFixedThreadPoolModule implements "Parameter 'threadCount' must be greater than 0"); } + @Override + public boolean canReuse(final Module oldModule) { + return isReusable() && triggerNewInstanceCreation == false; + } + @Override public Closeable getInstance() { if (instance == null) { diff --git a/opendaylight/config/threadpool-config-impl/src/test/java/org/opendaylight/controller/config/threadpool/fixed/FixedThreadPoolConfigBeanTest.java b/opendaylight/config/threadpool-config-impl/src/test/java/org/opendaylight/controller/config/threadpool/fixed/FixedThreadPoolConfigBeanTest.java index 64e83a544f..5bf4ac2d7b 100644 --- a/opendaylight/config/threadpool-config-impl/src/test/java/org/opendaylight/controller/config/threadpool/fixed/FixedThreadPoolConfigBeanTest.java +++ b/opendaylight/config/threadpool-config-impl/src/test/java/org/opendaylight/controller/config/threadpool/fixed/FixedThreadPoolConfigBeanTest.java @@ -40,6 +40,7 @@ public class FixedThreadPoolConfigBeanTest extends AbstractConfigTest { private FixedThreadPoolModuleFactory factory; private final String nameInstance = "fixedInstance"; + private ObjectName threadFactoryON; @Before public void setUp() { @@ -73,10 +74,12 @@ public class FixedThreadPoolConfigBeanTest extends AbstractConfigTest { assertBeanCount(1, factory.getImplementationName()); transaction = configRegistryClient.createTransaction(); + NamingThreadFactoryModuleMXBean namingThreadFactoryModuleMXBean = transaction.newMXBeanProxy(threadFactoryON, NamingThreadFactoryModuleMXBean.class); + namingThreadFactoryModuleMXBean.setNamePrefix("newPrefix"); CommitStatus status = transaction.commit(); assertBeanCount(1, factory.getImplementationName()); - assertStatus(status, 0, 0, 2); + assertStatus(status, 0, 2, 0); } @Test @@ -161,7 +164,7 @@ public class FixedThreadPoolConfigBeanTest extends AbstractConfigTest { FixedThreadPoolModuleMXBean mxBean = transaction.newMXBeanProxy(nameCreated, FixedThreadPoolModuleMXBean.class); mxBean.setMaxThreadCount(numberOfThreads); - ObjectName threadFactoryON = transaction.createModule(NamingThreadFactoryModuleFactory.NAME, "naming"); + threadFactoryON = transaction.createModule(NamingThreadFactoryModuleFactory.NAME, "naming"); NamingThreadFactoryModuleMXBean namingThreadFactoryModuleMXBean = transaction.newMXBeanProxy(threadFactoryON, NamingThreadFactoryModuleMXBean.class); namingThreadFactoryModuleMXBean.setNamePrefix(prefix); diff --git a/opendaylight/config/threadpool-config-impl/src/test/java/org/opendaylight/controller/config/threadpool/fixed/TestingFixedThreadPoolModule.java b/opendaylight/config/threadpool-config-impl/src/test/java/org/opendaylight/controller/config/threadpool/fixed/TestingFixedThreadPoolModule.java index d6171263af..bd57fe98f6 100644 --- a/opendaylight/config/threadpool-config-impl/src/test/java/org/opendaylight/controller/config/threadpool/fixed/TestingFixedThreadPoolModule.java +++ b/opendaylight/config/threadpool-config-impl/src/test/java/org/opendaylight/controller/config/threadpool/fixed/TestingFixedThreadPoolModule.java @@ -32,5 +32,4 @@ public class TestingFixedThreadPoolModule extends AbstractMockedModule implement doReturn(mock(ExecutorService.class)).when(pool).getExecutor(); return pool; } - } diff --git a/opendaylight/config/threadpool-config-impl/src/test/java/org/opendaylight/controller/config/threadpool/naming/TestingNamingThreadPoolFactoryModule.java b/opendaylight/config/threadpool-config-impl/src/test/java/org/opendaylight/controller/config/threadpool/naming/TestingNamingThreadPoolFactoryModule.java index 37fb839070..f91ec47ad9 100644 --- a/opendaylight/config/threadpool-config-impl/src/test/java/org/opendaylight/controller/config/threadpool/naming/TestingNamingThreadPoolFactoryModule.java +++ b/opendaylight/config/threadpool-config-impl/src/test/java/org/opendaylight/controller/config/threadpool/naming/TestingNamingThreadPoolFactoryModule.java @@ -62,4 +62,9 @@ public class TestingNamingThreadPoolFactoryModule implements Module, ThreadFacto return fact; } + @Override + public boolean canReuse(Module oldModule) { + return false; + } + } diff --git a/opendaylight/config/yang-jmx-generator-plugin/src/main/java/org/opendaylight/controller/config/yangjmxgenerator/plugin/ftl/AbstractModuleTemplate.java b/opendaylight/config/yang-jmx-generator-plugin/src/main/java/org/opendaylight/controller/config/yangjmxgenerator/plugin/ftl/AbstractModuleTemplate.java index 2402014fd3..487bf63e62 100644 --- a/opendaylight/config/yang-jmx-generator-plugin/src/main/java/org/opendaylight/controller/config/yangjmxgenerator/plugin/ftl/AbstractModuleTemplate.java +++ b/opendaylight/config/yang-jmx-generator-plugin/src/main/java/org/opendaylight/controller/config/yangjmxgenerator/plugin/ftl/AbstractModuleTemplate.java @@ -22,13 +22,12 @@ public class AbstractModuleTemplate extends GeneralClassTemplate { private final String registratorType; public AbstractModuleTemplate(Header header, String packageName, - String abstractModuleName, List implementedIfcs, - List moduleFields, List methods, + String abstractModuleName, List extendedClasses, + List implementedIfcs, List moduleFields, List methods, boolean isRuntime, String registratorType) { - super(header, packageName, abstractModuleName, Collections - . emptyList(), implementedIfcs, Collections - . emptyList(), methods, true, false, Collections - . emptyList()); + super(header, packageName, abstractModuleName, extendedClasses, + implementedIfcs, Collections. emptyList(), methods, + true, false, Collections. emptyList()); this.moduleFields = moduleFields; this.runtime = isRuntime; this.registratorType = registratorType; diff --git a/opendaylight/config/yang-jmx-generator-plugin/src/main/java/org/opendaylight/controller/config/yangjmxgenerator/plugin/ftl/GeneralClassTemplate.java b/opendaylight/config/yang-jmx-generator-plugin/src/main/java/org/opendaylight/controller/config/yangjmxgenerator/plugin/ftl/GeneralClassTemplate.java index 3f4917f2ff..4adb2b0cf5 100644 --- a/opendaylight/config/yang-jmx-generator-plugin/src/main/java/org/opendaylight/controller/config/yangjmxgenerator/plugin/ftl/GeneralClassTemplate.java +++ b/opendaylight/config/yang-jmx-generator-plugin/src/main/java/org/opendaylight/controller/config/yangjmxgenerator/plugin/ftl/GeneralClassTemplate.java @@ -40,7 +40,7 @@ public class GeneralClassTemplate extends AbstractFtlTemplate { static List checkCardinality(List extendedClass) { if (extendedClass.size() > 1) { throw new IllegalArgumentException( - "Class cannot have more than one super " + "class"); + "Class cannot have more than one super class, found: " + extendedClass); } return extendedClass; } diff --git a/opendaylight/config/yang-jmx-generator-plugin/src/main/java/org/opendaylight/controller/config/yangjmxgenerator/plugin/ftl/TemplateFactory.java b/opendaylight/config/yang-jmx-generator-plugin/src/main/java/org/opendaylight/controller/config/yangjmxgenerator/plugin/ftl/TemplateFactory.java index 53ab4ef335..19e875f9b1 100644 --- a/opendaylight/config/yang-jmx-generator-plugin/src/main/java/org/opendaylight/controller/config/yangjmxgenerator/plugin/ftl/TemplateFactory.java +++ b/opendaylight/config/yang-jmx-generator-plugin/src/main/java/org/opendaylight/controller/config/yangjmxgenerator/plugin/ftl/TemplateFactory.java @@ -23,7 +23,7 @@ import org.opendaylight.controller.config.api.IdentityAttributeRef; import org.opendaylight.controller.config.api.RuntimeBeanRegistratorAwareModule; import org.opendaylight.controller.config.api.annotations.AbstractServiceInterface; import org.opendaylight.controller.config.api.runtime.RuntimeBean; -import org.opendaylight.controller.config.spi.Module; +import org.opendaylight.controller.config.spi.AbstractModule; import org.opendaylight.controller.config.yangjmxgenerator.AbstractEntry; import org.opendaylight.controller.config.yangjmxgenerator.ModuleMXBeanEntry; import org.opendaylight.controller.config.yangjmxgenerator.RuntimeBeanEntry; @@ -196,7 +196,6 @@ public class TemplateFactory { List moduleFields = attrProcessor.getModuleFields(); List implementedIfcs = Lists.newArrayList( - Module.class.getCanonicalName(), mbe.getFullyQualifiedName(mbe.getMXBeanInterfaceName())); for (String implementedService : mbe.getProvidedServices().keySet()) { @@ -218,9 +217,11 @@ public class TemplateFactory { .getCanonicalName()); } + List extendedClasses = Collections.singletonList(AbstractModule.class.getCanonicalName() + "<" + mbe.getAbstractModuleName() + ">"); + AbstractModuleTemplate abstractModuleTemplate = new AbstractModuleTemplate( getHeaderFromEntry(mbe), mbe.getPackageName(), - mbe.getAbstractModuleName(), implementedIfcs, moduleFields, + mbe.getAbstractModuleName(), extendedClasses, implementedIfcs, moduleFields, attrProcessor.getMethods(), generateRuntime, registratorFullyQualifiedName); diff --git a/opendaylight/config/yang-jmx-generator-plugin/src/main/java/org/opendaylight/controller/config/yangjmxgenerator/plugin/gofactory/AbsModuleGeneratedObjectFactory.java b/opendaylight/config/yang-jmx-generator-plugin/src/main/java/org/opendaylight/controller/config/yangjmxgenerator/plugin/gofactory/AbsModuleGeneratedObjectFactory.java index 64a13d3f80..92a0eec586 100644 --- a/opendaylight/config/yang-jmx-generator-plugin/src/main/java/org/opendaylight/controller/config/yangjmxgenerator/plugin/gofactory/AbsModuleGeneratedObjectFactory.java +++ b/opendaylight/config/yang-jmx-generator-plugin/src/main/java/org/opendaylight/controller/config/yangjmxgenerator/plugin/gofactory/AbsModuleGeneratedObjectFactory.java @@ -10,9 +10,13 @@ package org.opendaylight.controller.config.yangjmxgenerator.plugin.gofactory; import static com.google.common.base.Preconditions.checkState; import static java.lang.String.format; +import com.google.common.base.Function; import com.google.common.base.Joiner; import com.google.common.base.Optional; -import java.util.ArrayList; +import com.google.common.collect.Collections2; +import com.google.common.collect.Iterables; +import com.google.common.collect.Lists; +import java.util.Collections; import java.util.HashMap; import java.util.LinkedHashMap; import java.util.List; @@ -25,8 +29,10 @@ import org.opendaylight.controller.config.yangjmxgenerator.ModuleMXBeanEntry; import org.opendaylight.controller.config.yangjmxgenerator.plugin.ftl.AbstractModuleTemplate; import org.opendaylight.controller.config.yangjmxgenerator.plugin.ftl.TemplateFactory; import org.opendaylight.controller.config.yangjmxgenerator.plugin.ftl.model.Annotation; +import org.opendaylight.controller.config.yangjmxgenerator.plugin.ftl.model.Field; import org.opendaylight.controller.config.yangjmxgenerator.plugin.ftl.model.IdentityRefModuleField; import org.opendaylight.controller.config.yangjmxgenerator.plugin.ftl.model.Method; +import org.opendaylight.controller.config.yangjmxgenerator.plugin.ftl.model.MethodDefinition; import org.opendaylight.controller.config.yangjmxgenerator.plugin.ftl.model.ModuleField; import org.opendaylight.controller.config.yangjmxgenerator.plugin.java.FullyQualifiedName; import org.opendaylight.controller.config.yangjmxgenerator.plugin.java.GeneratedObject; @@ -39,16 +45,26 @@ import org.slf4j.LoggerFactory; public class AbsModuleGeneratedObjectFactory { + private static final Function FULLY_QUALIFIED_NAME_FUNCTION = new Function() { + @Override + public FullyQualifiedName apply(final String input) { + return FullyQualifiedName.fromString(input); + } + }; + public GeneratedObject toGeneratedObject(ModuleMXBeanEntry mbe, Optional copyright) { FullyQualifiedName abstractFQN = new FullyQualifiedName(mbe.getPackageName(), mbe.getAbstractModuleName()); Optional classJavaDoc = Optional.fromNullable(mbe.getNullableDescription()); AbstractModuleTemplate abstractModuleTemplate = TemplateFactory.abstractModuleTemplateFromMbe(mbe); Optional header = abstractModuleTemplate.getHeaderString(); - List implementedInterfaces = new ArrayList<>(); - for(String implemented: abstractModuleTemplate.getTypeDeclaration().getImplemented()) { - implementedInterfaces.add(FullyQualifiedName.fromString(implemented)); - } + List implementedInterfaces = Lists.transform(abstractModuleTemplate.getTypeDeclaration().getImplemented(), FULLY_QUALIFIED_NAME_FUNCTION); + + Optional extended = + Optional.fromNullable( + Iterables.getFirst( + Collections2.transform(abstractModuleTemplate.getTypeDeclaration().getExtended(), FULLY_QUALIFIED_NAME_FUNCTION), null)); + Optional maybeRegistratorType; if (abstractModuleTemplate.isRuntime()) { maybeRegistratorType = Optional.of(FullyQualifiedName.fromString(abstractModuleTemplate.getRegistratorType())); @@ -56,7 +72,7 @@ public class AbsModuleGeneratedObjectFactory { maybeRegistratorType = Optional.absent(); } - return toGeneratedObject(abstractFQN, copyright, header, classJavaDoc, implementedInterfaces, + return toGeneratedObject(abstractFQN, copyright, header, classJavaDoc, extended, implementedInterfaces, abstractModuleTemplate.getModuleFields(), maybeRegistratorType, abstractModuleTemplate.getMethods(), mbe.getYangModuleQName()); } @@ -65,6 +81,7 @@ public class AbsModuleGeneratedObjectFactory { Optional copyright, Optional header, Optional classJavaDoc, + Optional extended, List implementedInterfaces, List moduleFields, Optional maybeRegistratorType, @@ -84,6 +101,9 @@ public class AbsModuleGeneratedObjectFactory { for(FullyQualifiedName implemented: implementedInterfaces) { b.addImplementsFQN(implemented); } + if(extended.isPresent()) { + b.addExtendsFQN(extended.get()); + } if (classJavaDoc.isPresent()) { b.addClassAnnotation(format("@%s(value=\"%s\")", Description.class.getCanonicalName(), classJavaDoc.get())); } @@ -99,9 +119,6 @@ public class AbsModuleGeneratedObjectFactory { b.addToBody("//attributes end"); - b.addToBody(getCommonFields(abstractFQN)); - - b.addToBody(getNewConstructor(abstractFQN)); b.addToBody(getCopyFromOldConstructor(abstractFQN)); @@ -110,11 +127,12 @@ public class AbsModuleGeneratedObjectFactory { b.addToBody(getCachesOfResolvedDependencies(moduleFields)); b.addToBody(getCachesOfResolvedIdentityRefs(moduleFields)); - b.addToBody(getGetInstance(moduleFields)); + b.addToBody(getResolveDependencies(moduleFields)); b.addToBody(getReuseLogic(moduleFields, abstractFQN)); b.addToBody(getEqualsAndHashCode(abstractFQN)); b.addToBody(getMethods(methods)); + b.addToBody(getGetLogger()); return new GeneratedObjectBuilder(b.build()).toGeneratedObject(); } @@ -163,28 +181,31 @@ public class AbsModuleGeneratedObjectFactory { // loop through fields, do deep equals on each field for (ModuleField moduleField : moduleFields) { + result += format( + "if (java.util.Objects.deepEquals(%s, other.%1$s) == false) {\n"+ + "return false;\n"+ + "}\n", moduleField.getName()); + if (moduleField.isListOfDependencies()) { result += format( - "if (%1$sDependency.equals(other.%1$sDependency) == false) {\n"+ - "return false;\n"+ - "}\n"+ - "for (int idx = 0; idx < %1$sDependency.size(); idx++) {\n"+ - "if (%1$sDependency.get(idx) != other.%1$sDependency.get(idx)) {\n"+ - "return false;\n"+ - "}\n"+ - "}\n" ,moduleField.getName()); + "for (int idx = 0; idx < %1$s.size(); idx++) {\n"+ + "if (!dependencyResolver.canReuseDependency(%1$s.get(idx), %1$sJmxAttribute)) {\n"+ + "return false;\n"+ + "}\n"+ + "}\n" , moduleField.getName()); } else if (moduleField.isDependent()) { result += format( - "if (%sDependency != other.%1$sDependency) { // reference to dependency must be same\n"+ - "return false;\n"+ - "}\n",moduleField.getName()); - } else { - result += format( - "if (java.util.Objects.deepEquals(%s, other.%1$s) == false) {\n"+ - "return false;\n"+ - "}\n", moduleField.getName()); + // If a reference is null (ie optional reference) it makes no sens to call canReuse on it + // In such case we continue in the isSame method because if we have null here, the previous value was null as well + // If the previous value was not null and current is or vice verse, the deepEquals comparison would return false + "if(%1$s!= null) {\n" + + "if (!dependencyResolver.canReuseDependency(%1$s, %1$sJmxAttribute)) { // reference to dependency must be reusable as well\n" + + "return false;\n" + + "}\n" + + "}\n", moduleField.getName()); } } + result += "\n"+ "return true;\n"+ "}\n"; @@ -192,13 +213,7 @@ public class AbsModuleGeneratedObjectFactory { return result; } - private static String getGetInstance(List moduleFields) { - String result = "\n"+ - "@Override\n"+ - format("public final %s getInstance() {\n", AutoCloseable.class.getCanonicalName())+ - "if(instance==null) {\n"; - // create instance start - + private static String getResolveDependencies(final List moduleFields) { // loop through dependent fields, use dependency resolver to instantiate dependencies. Do it in loop in case field represents list of dependencies. Map resolveDependenciesMap = new HashMap<>(); for(ModuleField moduleField: moduleFields) { @@ -207,19 +222,21 @@ public class AbsModuleGeneratedObjectFactory { String osgi = moduleField.getDependency().getSie().getExportedOsgiClassName(); if (moduleField.isList()) { str = format( - "%sDependency = new java.util.ArrayList<%s>();\n"+ - "for(javax.management.ObjectName dep : %1$s) {\n"+ - "%1$sDependency.add(dependencyResolver.resolveInstance(%2$s.class, dep, %1$sJmxAttribute));\n"+ - "}\n", moduleField.getName(), osgi); + "%sDependency = new java.util.ArrayList<%s>();\n"+ + "for(javax.management.ObjectName dep : %1$s) {\n"+ + "%1$sDependency.add(dependencyResolver.resolveInstance(%2$s.class, dep, %1$sJmxAttribute));\n"+ + "}\n", moduleField.getName(), osgi); } else { str = format( - "%1$sDependency = dependencyResolver.resolveInstance(%2$s.class, %1$s, %1$sJmxAttribute);\n", - moduleField.getName(), osgi); + "%1$sDependency = dependencyResolver.resolveInstance(%2$s.class, %1$s, %1$sJmxAttribute);\n", + moduleField.getName(), osgi); } resolveDependenciesMap.put(moduleField, str); } } + String result = "\n" + + "protected final void resolveDependencies() {\n"; // wrap each field resolvation statement with if !=null when dependency is not mandatory for (Map.Entry entry : resolveDependenciesMap.entrySet()) { if (entry.getKey().getDependency().isMandatory() == false) { @@ -236,9 +253,9 @@ public class AbsModuleGeneratedObjectFactory { result += format("if (%s!=null){\n", moduleField.getName()); if (moduleField.isList()) { result += format( - "for(%s candidate : %s) {\n"+ - "candidate.injectDependencyResolver(dependencyResolver);\n"+ - "}\n", moduleField.getGenericInnerType(), moduleField.getName()); + "for(%s candidate : %s) {\n"+ + "candidate.injectDependencyResolver(dependencyResolver);\n"+ + "}\n", moduleField.getGenericInnerType(), moduleField.getName()); } else { result += format("%s.injectDependencyResolver(dependencyResolver);\n", moduleField.getName()); } @@ -256,44 +273,10 @@ public class AbsModuleGeneratedObjectFactory { result += "}\n"; } } - - // create instance end: reuse and recreate logic - result += "if(oldInstance!=null && canReuseInstance(oldModule)) {\n"+ - "instance = reuseInstance(oldInstance);\n"+ - "} else {\n"+ - "if(oldInstance!=null) {\n"+ - "try {\n"+ - "oldInstance.close();\n"+ - "} catch(Exception e) {\n"+ - "logger.error(\"An error occurred while closing old instance \" + oldInstance, e);\n"+ - "}\n"+ - "}\n"+ - "instance = createInstance();\n"+ - "if (instance == null) {\n"+ - "throw new IllegalStateException(\"Error in createInstance - null is not allowed as return value\");\n"+ - "}\n"+ - "}\n"+ - "}\n"+ - "return instance;\n"+ - "}\n"+ - format("public abstract %s createInstance();\n", AutoCloseable.class.getCanonicalName()); - + result += "}\n"; return result; } - private static String getCommonFields(FullyQualifiedName abstractFQN) { - return "\n"+ - format("private final %s oldModule;\n", abstractFQN.getTypeName())+ - format("private final %s oldInstance;\n", AutoCloseable.class.getCanonicalName())+ - format("private %s instance;\n", AutoCloseable.class.getCanonicalName())+ - format("protected final %s dependencyResolver;\n", DependencyResolver.class.getCanonicalName())+ - format("private final %s identifier;\n", ModuleIdentifier.class.getCanonicalName())+ - "@Override\n"+ - format("public %s getIdentifier() {\n", ModuleIdentifier.class.getCanonicalName())+ - "return identifier;\n"+ - "}\n"; - } - private static String getCachesOfResolvedIdentityRefs(List moduleFields) { StringBuilder result = new StringBuilder(); for (ModuleField moduleField : moduleFields) { @@ -355,7 +338,7 @@ public class AbsModuleGeneratedObjectFactory { "public void validate() {\n"; // validate each mandatory dependency for(ModuleField moduleField: moduleFields) { - if (moduleField.isDependent() && moduleField.getDependency().isMandatory()) { + if (moduleField.isDependent()) { if (moduleField.isList()) { result += "" + format("for(javax.management.ObjectName dep : %s) {\n", moduleField.getName()) + @@ -363,8 +346,14 @@ public class AbsModuleGeneratedObjectFactory { moduleField.getDependency().getSie().getFullyQualifiedName(), moduleField.getName()) + "}\n"; } else { - result += format("dependencyResolver.validateDependency(%s.class, %s, %sJmxAttribute);", + if(moduleField.getDependency().isMandatory() == false) { + result += format("if(%s != null) {\n", moduleField.getName()); + } + result += format("dependencyResolver.validateDependency(%s.class, %s, %sJmxAttribute);\n", moduleField.getDependency().getSie().getFullyQualifiedName(), moduleField.getName(), moduleField.getName()); + if(moduleField.getDependency().isMandatory() == false) { + result += "}\n"; + } } } } @@ -378,7 +367,7 @@ public class AbsModuleGeneratedObjectFactory { } private static String getLoggerDefinition(FullyQualifiedName fqn) { - return format("private static final %s logger = %s.getLogger(%s.class);", + return format("private static final %s LOGGER = %s.getLogger(%s.class);", Logger.class.getCanonicalName(), LoggerFactory.class.getCanonicalName(), fqn); } @@ -386,14 +375,9 @@ public class AbsModuleGeneratedObjectFactory { private static String getConstructorStart(FullyQualifiedName fqn, LinkedHashMap parameters, String after) { String paramString = Joiner.on(",").withKeyValueSeparator(" ").join(parameters); - String setters = ""; - for (String paramName : parameters.values()) { - setters += format("this.%s = %1$s;\n", paramName); - } return format("public %s(", fqn.getTypeName()) + paramString + ") {\n" + - setters + after + "}\n"; } @@ -402,10 +386,8 @@ public class AbsModuleGeneratedObjectFactory { LinkedHashMap parameters = new LinkedHashMap<>(); parameters.put(ModuleIdentifier.class.getCanonicalName(), "identifier"); parameters.put(DependencyResolver.class.getCanonicalName(), "dependencyResolver"); - - String setToNulls = "this.oldInstance=null;\n" + - "this.oldModule=null;\n"; - return getConstructorStart(abstractFQN, parameters, setToNulls); + String init = "super(identifier, dependencyResolver);\n"; + return getConstructorStart(abstractFQN, parameters, init); } private static String getCopyFromOldConstructor(FullyQualifiedName abstractFQN) { @@ -414,6 +396,11 @@ public class AbsModuleGeneratedObjectFactory { parameters.put(DependencyResolver.class.getCanonicalName(), "dependencyResolver"); parameters.put(abstractFQN.getTypeName(), "oldModule"); parameters.put(AutoCloseable.class.getCanonicalName(), "oldInstance"); - return getConstructorStart(abstractFQN, parameters, ""); + String init = "super(identifier, dependencyResolver, oldModule, oldInstance);\n"; + return getConstructorStart(abstractFQN, parameters, init); + } + + public String getGetLogger() { + return new MethodDefinition(Logger.class.getCanonicalName(), "getLogger", Collections.emptyList(), "return LOGGER;").toString(); } } diff --git a/opendaylight/config/yang-jmx-generator-plugin/src/test/java/org/opendaylight/controller/config/yangjmxgenerator/plugin/JMXGeneratorTest.java b/opendaylight/config/yang-jmx-generator-plugin/src/test/java/org/opendaylight/controller/config/yangjmxgenerator/plugin/JMXGeneratorTest.java index 8c2b5d69eb..a6cfc58c34 100644 --- a/opendaylight/config/yang-jmx-generator-plugin/src/test/java/org/opendaylight/controller/config/yangjmxgenerator/plugin/JMXGeneratorTest.java +++ b/opendaylight/config/yang-jmx-generator-plugin/src/test/java/org/opendaylight/controller/config/yangjmxgenerator/plugin/JMXGeneratorTest.java @@ -63,6 +63,7 @@ import org.opendaylight.controller.config.api.DynamicMBeanWithInstance; import org.opendaylight.controller.config.api.annotations.Description; import org.opendaylight.controller.config.api.annotations.RequireInterface; import org.opendaylight.controller.config.api.annotations.ServiceInterfaceAnnotation; +import org.opendaylight.controller.config.spi.AbstractModule; import org.opendaylight.controller.config.spi.Module; import org.opendaylight.controller.config.spi.ModuleFactory; import org.opendaylight.controller.config.yangjmxgenerator.ConfigConstants; @@ -520,11 +521,11 @@ public class JMXGeneratorTest extends AbstractGeneratorTest { assertContains(visitor.implmts, PackageTranslatorTest.EXPECTED_PACKAGE_PREFIX + ".threads.java.DynamicThreadPoolModuleMXBean", - Module.class.getCanonicalName(), PackageTranslatorTest.EXPECTED_PACKAGE_PREFIX + ".threads.ScheduledThreadPoolServiceInterface", PackageTranslatorTest.EXPECTED_PACKAGE_PREFIX + ".threads.ThreadPoolServiceInterface"); + assertContains(visitor.extnds, AbstractModule.class.getCanonicalName()); assertEquals(2, visitor.constructors.size()); Set fieldDeclarations = visitor.fieldDeclarations; assertDeclaredField(fieldDeclarations, @@ -538,7 +539,7 @@ public class JMXGeneratorTest extends AbstractGeneratorTest { assertDeclaredField(fieldDeclarations, "private java.lang.Long coreSize"); assertDeclaredField(fieldDeclarations, "private byte[] binary"); - assertEquals(22, fieldDeclarations.size()); + assertEquals(17, fieldDeclarations.size()); assertEquals(1, visitor.requireIfc.size()); String reqIfc = visitor.requireIfc.get("setThreadfactory"); @@ -546,7 +547,7 @@ public class JMXGeneratorTest extends AbstractGeneratorTest { assertContains(reqIfc, PackageTranslatorTest.EXPECTED_PACKAGE_PREFIX + ".threads.ThreadFactoryServiceInterface"); - assertEquals("Incorrenct number of generated methods", 27, + assertEquals("Incorrenct number of generated methods", 26, visitor.methods.size()); assertEquals("Incorrenct number of generated method descriptions", 3, visitor.methodDescriptions.size()); diff --git a/opendaylight/config/yang-test/src/main/java/org/opendaylight/controller/config/yang/test/impl/.gitignore b/opendaylight/config/yang-test/src/main/java/org/opendaylight/controller/config/yang/test/impl/.gitignore index 27d1535693..afd1e9d6dd 100644 --- a/opendaylight/config/yang-test/src/main/java/org/opendaylight/controller/config/yang/test/impl/.gitignore +++ b/opendaylight/config/yang-test/src/main/java/org/opendaylight/controller/config/yang/test/impl/.gitignore @@ -1 +1,2 @@ *.java +!CheckedAutoCloseable.java diff --git a/opendaylight/config/yang-test/src/main/java/org/opendaylight/controller/config/yang/test/impl/CheckedAutoCloseable.java b/opendaylight/config/yang-test/src/main/java/org/opendaylight/controller/config/yang/test/impl/CheckedAutoCloseable.java new file mode 100644 index 0000000000..74cf617d3f --- /dev/null +++ b/opendaylight/config/yang-test/src/main/java/org/opendaylight/controller/config/yang/test/impl/CheckedAutoCloseable.java @@ -0,0 +1,17 @@ +package org.opendaylight.controller.config.yang.test.impl; + +import com.google.common.base.Preconditions; + +public class CheckedAutoCloseable implements AutoCloseable { + private boolean closed = false; + + @Override + public synchronized void close() throws Exception { + Preconditions.checkState(closed == false); + this.closed = true; + } + + public synchronized boolean isClosed() { + return this.closed; + } +} diff --git a/opendaylight/config/yang-test/src/main/java/org/opendaylight/controller/config/yang/test/impl/DepTestImplModuleStub.txt b/opendaylight/config/yang-test/src/main/java/org/opendaylight/controller/config/yang/test/impl/DepTestImplModuleStub.txt index 80c1e54a15..2359b50e0a 100644 --- a/opendaylight/config/yang-test/src/main/java/org/opendaylight/controller/config/yang/test/impl/DepTestImplModuleStub.txt +++ b/opendaylight/config/yang-test/src/main/java/org/opendaylight/controller/config/yang/test/impl/DepTestImplModuleStub.txt @@ -1,5 +1 @@ - return new AutoCloseable() { - @Override - public void close() throws Exception { - } - }; + return new org.opendaylight.controller.config.yang.test.impl.CheckedAutoCloseable(); diff --git a/opendaylight/config/yang-test/src/main/java/org/opendaylight/controller/config/yang/test/impl/IdentityTestModuleStub.txt b/opendaylight/config/yang-test/src/main/java/org/opendaylight/controller/config/yang/test/impl/IdentityTestModuleStub.txt index 7fa0c10eb2..458ec01165 100644 --- a/opendaylight/config/yang-test/src/main/java/org/opendaylight/controller/config/yang/test/impl/IdentityTestModuleStub.txt +++ b/opendaylight/config/yang-test/src/main/java/org/opendaylight/controller/config/yang/test/impl/IdentityTestModuleStub.txt @@ -9,8 +9,4 @@ } logger.info("IdentityContainer Afi class: {}", getIdentitiesContainer().resolveAfi()); - return new AutoCloseable() { - @Override - public void close() throws Exception { - } - }; + return new org.opendaylight.controller.config.yang.test.impl.CheckedAutoCloseable(); diff --git a/opendaylight/config/yang-test/src/main/java/org/opendaylight/controller/config/yang/test/impl/MultipleDependenciesModuleStub.txt b/opendaylight/config/yang-test/src/main/java/org/opendaylight/controller/config/yang/test/impl/MultipleDependenciesModuleStub.txt index 80c1e54a15..bbf2a71faa 100644 --- a/opendaylight/config/yang-test/src/main/java/org/opendaylight/controller/config/yang/test/impl/MultipleDependenciesModuleStub.txt +++ b/opendaylight/config/yang-test/src/main/java/org/opendaylight/controller/config/yang/test/impl/MultipleDependenciesModuleStub.txt @@ -1,5 +1,16 @@ - return new AutoCloseable() { + return new org.opendaylight.controller.config.yang.test.impl.CheckedAutoCloseable() { @Override - public void close() throws Exception { + public synchronized void close() throws Exception { + if(getSingleDependency() != null && getSingleDependency().isClosed() == true) { + // Simulate a cleanup on dependencies that should not be closed yet + throw new java.lang.Error("Dependency was closed first"); + } + for (org.opendaylight.controller.config.yang.test.impl.CheckedAutoCloseable autoCloseable : getTestingDepsDependency()) { + if(autoCloseable.isClosed() == true) { + // Simulate a cleanup on dependencies that should not be closed yet + throw new java.lang.Error("Dependency was closed first"); + } + } + super.close(); } - }; + }; \ No newline at end of file diff --git a/opendaylight/config/yang-test/src/main/java/org/opendaylight/controller/config/yang/test/impl/NetconfTestImplModuleStub.txt b/opendaylight/config/yang-test/src/main/java/org/opendaylight/controller/config/yang/test/impl/NetconfTestImplModuleStub.txt index 4b5b78f70a..7e155e05c7 100644 --- a/opendaylight/config/yang-test/src/main/java/org/opendaylight/controller/config/yang/test/impl/NetconfTestImplModuleStub.txt +++ b/opendaylight/config/yang-test/src/main/java/org/opendaylight/controller/config/yang/test/impl/NetconfTestImplModuleStub.txt @@ -1 +1,7 @@ - return org.opendaylight.controller.config.yang.test.util.NetconfTestImplModuleUtil.registerRuntimeBeans(this); + final AutoCloseable runtime = org.opendaylight.controller.config.yang.test.util.NetconfTestImplModuleUtil.registerRuntimeBeans(this); + return new org.opendaylight.controller.config.yang.test.impl.CheckedAutoCloseable() { + @Override + public synchronized void close() throws Exception { + runtime.close(); + } + }; \ No newline at end of file diff --git a/opendaylight/config/yang-test/src/main/java/org/opendaylight/controller/config/yang/test/impl/TestImplModuleStub.txt b/opendaylight/config/yang-test/src/main/java/org/opendaylight/controller/config/yang/test/impl/TestImplModuleStub.txt index 80c1e54a15..dc933b4f3b 100644 --- a/opendaylight/config/yang-test/src/main/java/org/opendaylight/controller/config/yang/test/impl/TestImplModuleStub.txt +++ b/opendaylight/config/yang-test/src/main/java/org/opendaylight/controller/config/yang/test/impl/TestImplModuleStub.txt @@ -1,5 +1 @@ - return new AutoCloseable() { - @Override - public void close() throws Exception { - } - }; + return new org.opendaylight.controller.config.yang.test.impl.CheckedAutoCloseable(); \ No newline at end of file diff --git a/opendaylight/config/yang-test/src/main/yang/config-test-impl.yang b/opendaylight/config/yang-test/src/main/yang/config-test-impl.yang index 093d7b3f13..f772a26898 100644 --- a/opendaylight/config/yang-test/src/main/yang/config-test-impl.yang +++ b/opendaylight/config/yang-test/src/main/yang/config-test-impl.yang @@ -468,7 +468,9 @@ module config-test-impl { } } } + } + } } @@ -485,6 +487,19 @@ module config-test-impl { } } } + container single { + uses config:service-ref { + refine type { + mandatory false; + config:required-identity test:testing; + } + } + } + + leaf simple { + type boolean; + default false; + } } } } diff --git a/opendaylight/config/yang-test/src/main/yang/config-test.yang b/opendaylight/config/yang-test/src/main/yang/config-test.yang index 2daf4055db..4843ebc895 100644 --- a/opendaylight/config/yang-test/src/main/yang/config-test.yang +++ b/opendaylight/config/yang-test/src/main/yang/config-test.yang @@ -18,6 +18,6 @@ module config-test { "Test api"; base "config:service-type"; - config:java-class "java.lang.AutoCloseable"; + config:java-class "org.opendaylight.controller.config.yang.test.impl.CheckedAutoCloseable"; } } diff --git a/opendaylight/config/yang-test/src/test/java/org/opendaylight/controller/config/yang/test/impl/MultipleDependenciesModuleTest.java b/opendaylight/config/yang-test/src/test/java/org/opendaylight/controller/config/yang/test/impl/MultipleDependenciesModuleTest.java index 2df15d0dc1..997e4e3196 100644 --- a/opendaylight/config/yang-test/src/test/java/org/opendaylight/controller/config/yang/test/impl/MultipleDependenciesModuleTest.java +++ b/opendaylight/config/yang-test/src/test/java/org/opendaylight/controller/config/yang/test/impl/MultipleDependenciesModuleTest.java @@ -11,13 +11,17 @@ package org.opendaylight.controller.config.yang.test.impl; import static java.util.Arrays.asList; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.fail; import static org.opendaylight.controller.config.api.jmx.ObjectNameUtil.getInstanceName; import static org.opendaylight.controller.config.api.jmx.ObjectNameUtil.getTransactionName; import java.util.List; import javax.management.ObjectName; +import org.hamcrest.CoreMatchers; import org.junit.Before; import org.junit.Test; +import org.opendaylight.controller.config.api.ValidationException; import org.opendaylight.controller.config.manager.impl.AbstractConfigTest; import org.opendaylight.controller.config.manager.impl.factoriesresolver.HardcodedModuleFactoriesResolver; import org.opendaylight.controller.config.util.ConfigTransactionJMXClient; @@ -49,4 +53,53 @@ public class MultipleDependenciesModuleTest extends AbstractConfigTest { assertNull(getTransactionName(d1WithoutTxName)); transaction.commit(); } + + @Test + public void testCloseOrdering() throws Exception { + // Tests whether close is called in correct order on the module instances on following graph + // Each module tests whether its dependencies were closed before it (to simulate resource clean up failure) + // R1 + // | \ + // M1 M2 + // | + // L1 + ConfigTransactionJMXClient transaction = configRegistryClient.createTransaction(); + ObjectName r1 = transaction.createModule(factory.getImplementationName(), "root1"); + ObjectName m1 = transaction.createModule(factory.getImplementationName(), "middle1"); + ObjectName m2 = transaction.createModule(factory.getImplementationName(), "middle2"); + ObjectName l1 = transaction.createModule(factory.getImplementationName(), "leaf1"); + + MultipleDependenciesModuleMXBean r1Proxy = transaction.newMXBeanProxy(r1, MultipleDependenciesModuleMXBean.class); + MultipleDependenciesModuleMXBean i1Proxy = transaction.newMXBeanProxy(m1, MultipleDependenciesModuleMXBean.class); + r1Proxy.setSingle(m1); + i1Proxy.setSingle(l1); + r1Proxy.setTestingDeps(asList(m2)); + transaction.commit(); + + configRegistryClient.createTransaction().commit(); + transaction = configRegistryClient.createTransaction(); + MultipleDependenciesModuleMXBean l1Proxy = transaction.newMXBeanProxy(l1, MultipleDependenciesModuleMXBean.class); + l1Proxy.setSimple(true); + transaction.commit(); + } + + @Test + public void testDestroyModuleDependency() throws Exception { + ConfigTransactionJMXClient transaction = configRegistryClient.createTransaction(); + ObjectName r1 = transaction.createModule(factory.getImplementationName(), "root1"); + ObjectName m1 = transaction.createModule(factory.getImplementationName(), "middle1"); + + MultipleDependenciesModuleMXBean r1Proxy = transaction.newMXBeanProxy(r1, MultipleDependenciesModuleMXBean.class); + r1Proxy.setSingle(m1); + transaction.commit(); + + transaction = configRegistryClient.createTransaction(); + transaction.destroyModule(factory.getImplementationName(), "middle1"); + try { + transaction.commit(); + fail("Validation exception expected"); + } catch (ValidationException e) { + assertThat(e.getFailedValidations().keySet(), CoreMatchers.hasItem("multiple-dependencies")); + } + } }