minor cleanups following #338
Project: http://git-wip-us.apache.org/repos/asf/brooklyn-server/repo Commit: http://git-wip-us.apache.org/repos/asf/brooklyn-server/commit/88a09bae Tree: http://git-wip-us.apache.org/repos/asf/brooklyn-server/tree/88a09bae Diff: http://git-wip-us.apache.org/repos/asf/brooklyn-server/diff/88a09bae Branch: refs/heads/master Commit: 88a09bae443548cab24bd585b7e84f40125b0d4c Parents: a65aae7 Author: Alex Heneveld <[email protected]> Authored: Fri Apr 21 17:07:34 2017 +0100 Committer: Alex Heneveld <[email protected]> Committed: Fri Apr 21 23:45:53 2017 +0100 ---------------------------------------------------------------------- .../internal/AbstractBrooklynObjectSpec.java | 55 ++++++++++-------- .../brooklyn/api/objs/BrooklynObject.java | 9 ++- .../brooklyn/catalog/CatalogYamlEntityTest.java | 39 +++++-------- .../core/catalog/internal/CatalogItemDo.java | 5 ++ .../core/mgmt/rebind/RebindIteration.java | 9 ++- .../core/objs/AbstractBrooklynObject.java | 59 ++++++++++++-------- .../core/objs/BrooklynObjectInternal.java | 1 + 7 files changed, 98 insertions(+), 79 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/88a09bae/api/src/main/java/org/apache/brooklyn/api/internal/AbstractBrooklynObjectSpec.java ---------------------------------------------------------------------- diff --git a/api/src/main/java/org/apache/brooklyn/api/internal/AbstractBrooklynObjectSpec.java b/api/src/main/java/org/apache/brooklyn/api/internal/AbstractBrooklynObjectSpec.java index d88078a..adbb48b 100644 --- a/api/src/main/java/org/apache/brooklyn/api/internal/AbstractBrooklynObjectSpec.java +++ b/api/src/main/java/org/apache/brooklyn/api/internal/AbstractBrooklynObjectSpec.java @@ -22,10 +22,8 @@ import static com.google.common.base.Preconditions.checkNotNull; import java.io.Serializable; import java.lang.reflect.Modifier; -import java.util.ArrayDeque; import java.util.Collection; import java.util.Collections; -import java.util.Deque; import java.util.List; import java.util.Map; import java.util.Set; @@ -36,6 +34,7 @@ import org.apache.brooklyn.api.objs.BrooklynObject; import org.apache.brooklyn.api.objs.SpecParameter; import org.apache.brooklyn.config.ConfigKey; import org.apache.brooklyn.config.ConfigKey.HasConfigKey; +import org.apache.brooklyn.util.collections.MutableList; import org.apache.brooklyn.util.collections.MutableSet; import org.apache.brooklyn.util.exceptions.Exceptions; import org.slf4j.Logger; @@ -69,7 +68,7 @@ public abstract class AbstractBrooklynObjectSpec<T, SpecT extends AbstractBrookl private final Class<? extends T> type; private String displayName; private String catalogItemId; - private Deque<String> catalogItemIdSearchPath = new ArrayDeque<>(); + private Collection<String> catalogItemIdSearchPath = MutableSet.of(); private Set<Object> tags = MutableSet.of(); private List<SpecParameter<?>> parameters = ImmutableList.of(); @@ -117,16 +116,30 @@ public abstract class AbstractBrooklynObjectSpec<T, SpecT extends AbstractBrookl * Set the immediate catalog item ID of this object, and the search path of other catalog items used to define it. */ public synchronized SpecT catalogItemIdAndSearchPath(String catalogItemId, Collection<String> searchPath) { + // TODO if ID is null should we really ignore search path? if (catalogItemId != null) { catalogItemId(catalogItemId); - catalogItemIdSearchPath.clear(); - catalogItemIdSearchPath.addAll(searchPath); + synchronized (catalogItemIdSearchPath) { + catalogItemIdSearchPath.clear(); + if (searchPath!=null) { + catalogItemIdSearchPath.addAll(searchPath); + } + } } return self(); } + public synchronized SpecT addSearchPath(List<String> searchPath) { + if (searchPath!=null) { + synchronized (catalogItemIdSearchPath) { + catalogItemIdSearchPath.addAll(searchPath); + } + } + return self(); + } + /** - * @deprecated since 0.11.0, use {@link #stackCatalogItemId(String)} instead + * @deprecated since 0.11.0, most callers would want {@link #stackCatalogItemId(String)} instead, though semantics are different */ @Deprecated @Beta @@ -139,7 +152,7 @@ public abstract class AbstractBrooklynObjectSpec<T, SpecT extends AbstractBrookl protected Object readResolve() { if (catalogItemIdSearchPath == null) { - catalogItemIdSearchPath = new ArrayDeque<>(); + catalogItemIdSearchPath = MutableList.of(); } return this; } @@ -160,7 +173,13 @@ public abstract class AbstractBrooklynObjectSpec<T, SpecT extends AbstractBrookl public SpecT stackCatalogItemId(String val) { if (null != val) { if (null != catalogItemId && !catalogItemId.equals(val)) { - catalogItemIdSearchPath.addFirst(catalogItemId); + synchronized (catalogItemIdSearchPath) { + Set<String> newPath = MutableSet.of(); + newPath.add(catalogItemId); + newPath.addAll(catalogItemIdSearchPath); + catalogItemIdSearchPath.clear(); + catalogItemIdSearchPath.addAll(newPath); + } } catalogItemId(val); } @@ -253,26 +272,16 @@ public abstract class AbstractBrooklynObjectSpec<T, SpecT extends AbstractBrookl return displayName; } + /** Same as {@link BrooklynObject#getCatalogItemId()}. */ public final String getCatalogItemId() { return catalogItemId; } - /** - * An immutable list of ids of catalog items that define this item. - * Wrapping items are first in the list (i.e. wrapping items precede wrapped items), - * for example, for a spec of an item of type Z, where the catalog is: - * <pre> - * items: - * - id: X - * - id: Y - * item: X - * - id: Z - * item: Y - * </pre> - * then the spec will have getCatalogId() of Z and getCatalogItemIdSearchPath() of Y, X. - */ + /** Same as {@link BrooklynObject#getCatalogItemIdSearchPath()}. */ public final List<String> getCatalogItemIdSearchPath() { - return ImmutableList.copyOf(catalogItemIdSearchPath); + synchronized (catalogItemIdSearchPath) { + return ImmutableList.copyOf(catalogItemIdSearchPath); + } } public final Set<Object> getTags() { http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/88a09bae/api/src/main/java/org/apache/brooklyn/api/objs/BrooklynObject.java ---------------------------------------------------------------------- diff --git a/api/src/main/java/org/apache/brooklyn/api/objs/BrooklynObject.java b/api/src/main/java/org/apache/brooklyn/api/objs/BrooklynObject.java index d72fe6a..265813a 100644 --- a/api/src/main/java/org/apache/brooklyn/api/objs/BrooklynObject.java +++ b/api/src/main/java/org/apache/brooklyn/api/objs/BrooklynObject.java @@ -60,8 +60,10 @@ public interface BrooklynObject extends Identifiable, Configurable { String getCatalogItemId(); /** - * An immutable list of ids of catalog items that define this item. - * e.g. if the catalog item is defined as a Z where + * An immutable list of ids of catalog items that this item depends on in some way, + * with the item that directly defines it implicit, but other items it references explicit. + * Wrapping items are first in the list (i.e. wrapping items precede wrapped items), + * so for example, if the catalog is: * <pre> * items: * - id: X @@ -70,7 +72,8 @@ public interface BrooklynObject extends Identifiable, Configurable { * - id: Z * item: Y * </pre> - * then the list will contain X, Y. + * the spec for Z will have getCatalogId() of Z and getCatalogItemIdSearchPath() of Y, X. + * (The self catalog ID is implicit at the head of the search path.) */ List<String> getCatalogItemIdSearchPath(); http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/88a09bae/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java ---------------------------------------------------------------------- diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java index 847e461..bc9f9ad 100644 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java @@ -22,7 +22,6 @@ import static com.google.common.base.Preconditions.checkNotNull; import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertNotNull; import static org.testng.Assert.assertTrue; -import static org.testng.Assert.fail; import org.apache.brooklyn.api.catalog.BrooklynCatalog; import org.apache.brooklyn.api.entity.Entity; @@ -39,8 +38,7 @@ import org.apache.brooklyn.core.test.entity.TestEntityImpl; import org.apache.brooklyn.core.typereg.RegisteredTypes; import org.apache.brooklyn.entity.stock.BasicApplication; import org.apache.brooklyn.entity.stock.BasicEntity; -import org.apache.brooklyn.util.exceptions.Exceptions; -import org.apache.brooklyn.util.osgi.OsgiTestResources; +import org.apache.brooklyn.test.Asserts; import org.testng.Assert; import org.testng.annotations.Test; @@ -50,15 +48,6 @@ import com.google.common.collect.Iterables; public class CatalogYamlEntityTest extends AbstractYamlTest { - - private static final String SIMPLE_ENTITY_TYPE = OsgiTestResources.BROOKLYN_TEST_OSGI_ENTITIES_SIMPLE_ENTITY; - private static final String MORE_ENTITIES_POM_PROPERTIES_PATH = - "META-INF/maven/org.apache.brooklyn.test.resources.osgi/brooklyn-test-osgi-more-entities/pom.properties"; - - @Override - protected boolean disableOsgi() { - return false; - } @Test public void testAddCatalogItemVerySimple() throws Exception { @@ -205,7 +194,7 @@ public class CatalogYamlEntityTest extends AbstractYamlTest { RegisteredType referrer = mgmt().getTypeRegistry().get(referrerSymbolicName, TEST_VERSION); String planYaml = RegisteredTypes.getImplementationDataStringForSpec(referrer); - Assert.assertTrue(planYaml.contains("services"), "expected services in: "+planYaml); + Asserts.assertStringContains(planYaml, "services"); Entity app = createAndStartApplication("services:", "- type: " + ver(referrerSymbolicName, TEST_VERSION)); @@ -334,10 +323,9 @@ public class CatalogYamlEntityTest extends AbstractYamlTest { " - type: " + BasicEntity.class.getName(), " brooklyn.children:", " - type: " + ver(referrerSymbolicName, TEST_VERSION)); - fail("Expected to throw"); + Asserts.shouldHaveFailedPreviously(); } catch (Exception e) { - Exceptions.propagateIfFatal(e); - assertTrue(e.getMessage().contains(referrerSymbolicName), "message was: "+e); + Asserts.expectedFailureContains(e, referrerSymbolicName); } } @@ -384,9 +372,9 @@ public class CatalogYamlEntityTest extends AbstractYamlTest { try { addCatalogEntity(IdAndVersion.of(symbolicName, TEST_VERSION + "-update"), symbolicName); - fail("Catalog addition expected to fail due to recursive reference to " + symbolicName); + Asserts.shouldHaveFailedPreviously("Catalog addition expected to fail due to recursive reference to " + symbolicName); } catch (IllegalStateException e) { - assertTrue(e.toString().contains("recursive"), "Unexpected error message: "+e); + Asserts.expectedFailureContains(e, "recursive", symbolicName); } } @@ -400,9 +388,9 @@ public class CatalogYamlEntityTest extends AbstractYamlTest { String versionedId = CatalogUtils.getVersionedId(symbolicName, TEST_VERSION); try { addCatalogEntity(IdAndVersion.of(symbolicName, TEST_VERSION + "-update"), versionedId); - fail("Catalog addition expected to fail due to recursive reference to " + versionedId); + Asserts.shouldHaveFailedPreviously("Catalog addition expected to fail due to recursive reference to " + versionedId); } catch (IllegalStateException e) { - assertTrue(e.toString().contains("recursive"), "Unexpected error message: "+e); + Asserts.expectedFailureContains(e, "recursive", symbolicName, versionedId); } } @@ -419,9 +407,9 @@ public class CatalogYamlEntityTest extends AbstractYamlTest { try { addCatalogEntity(IdAndVersion.of(callerSymbolicName, TEST_VERSION), calleeSymbolicName); - fail(); + Asserts.shouldHaveFailedPreviously(); } catch (IllegalStateException e) { - assertTrue(e.toString().contains("recursive"), "Unexpected error message: "+e); + Asserts.expectedFailureContains(e, "recursive"); } } @@ -450,9 +438,9 @@ public class CatalogYamlEntityTest extends AbstractYamlTest { " - type: " + BasicEntity.class.getName(), " brooklyn.children:", " - type: " + calleeSymbolicName); - fail(); + Asserts.shouldHaveFailedPreviously(); } catch (IllegalStateException e) { - assertTrue(e.toString().contains("recursive"), "Unexpected error message: "+e); + Asserts.expectedFailureContains(e, "recursive"); } } @@ -606,7 +594,8 @@ public class CatalogYamlEntityTest extends AbstractYamlTest { String yaml = Joiner.on("\n").join( "name: simple-app-yaml", - "location: localhost", + "location:", + "localhost: { latitude: 0, longitude: 0 }", // prevent host geo lookup delay (slowing down test on my network) "services:", " - type: "+id+":"+version); Entity app = createAndStartApplication(yaml); http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/88a09bae/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogItemDo.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogItemDo.java b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogItemDo.java index 0738d76..c263e59 100644 --- a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogItemDo.java +++ b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogItemDo.java @@ -135,6 +135,11 @@ public class CatalogItemDo<T,SpecT> implements CatalogItem<T,SpecT>, BrooklynObj public void setCatalogItemIdAndSearchPath(String catalogItemId, List<String> ids) { itemDto.setCatalogItemIdAndSearchPath(catalogItemId, ids); } + + @Override + public void addSearchPath(List<String> searchPath) { + itemDto.addSearchPath(searchPath); + } @Override public List<String> getCatalogItemIdSearchPath() { http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/88a09bae/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/RebindIteration.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/RebindIteration.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/RebindIteration.java index e4fbaa6..ad12c52 100644 --- a/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/RebindIteration.java +++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/RebindIteration.java @@ -1124,8 +1124,7 @@ public abstract class RebindIteration { */ protected Policy newPolicy(PolicyMemento memento) { String id = memento.getId(); - LoadedClass<? extends Policy> loaded = load(Policy.class, memento.getType(), memento.getCatalogItemId(), - memento.getCatalogItemIdSearchPath(), id); + LoadedClass<? extends Policy> loaded = load(Policy.class, memento); Class<? extends Policy> policyClazz = loaded.clazz; Policy policy; @@ -1151,7 +1150,7 @@ public abstract class RebindIteration { policy = invokeConstructor(null, policyClazz, new Object[] {flags}); } - setCatalogItemIds(policy, memento.getCatalogItemId(), memento.getCatalogItemIdSearchPath()); + setCatalogItemIds(policy, loaded.catalogItemId, loaded.searchPath); return policy; } @@ -1186,7 +1185,7 @@ public abstract class RebindIteration { enricher = invokeConstructor(reflections, enricherClazz, new Object[] {flags}); } - setCatalogItemIds(enricher, memento.getCatalogItemId(), memento.getCatalogItemIdSearchPath()); + setCatalogItemIds(enricher, loaded.catalogItemId, loaded.searchPath); return enricher; } @@ -1210,7 +1209,7 @@ public abstract class RebindIteration { "; type="+feedClazz); } - setCatalogItemIds(feed, memento.getCatalogItemId(), memento.getCatalogItemIdSearchPath()); + setCatalogItemIds(feed, loaded.catalogItemId, loaded.searchPath); return feed; } http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/88a09bae/core/src/main/java/org/apache/brooklyn/core/objs/AbstractBrooklynObject.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/objs/AbstractBrooklynObject.java b/core/src/main/java/org/apache/brooklyn/core/objs/AbstractBrooklynObject.java index 9a409ca..9ba5a61 100644 --- a/core/src/main/java/org/apache/brooklyn/core/objs/AbstractBrooklynObject.java +++ b/core/src/main/java/org/apache/brooklyn/core/objs/AbstractBrooklynObject.java @@ -18,23 +18,12 @@ */ package org.apache.brooklyn.core.objs; -import java.util.ArrayDeque; +import java.util.Collection; import java.util.Collections; -import java.util.Deque; import java.util.List; import java.util.Map; import java.util.Set; -import com.google.common.collect.ImmutableList; - -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Iterables; -import com.google.common.collect.Maps; -import com.google.common.collect.Sets; - import org.apache.brooklyn.api.internal.ApiObjectsFactory; import org.apache.brooklyn.api.mgmt.ManagementContext; import org.apache.brooklyn.core.entity.AbstractEntity; @@ -42,9 +31,19 @@ import org.apache.brooklyn.core.mgmt.internal.ManagementContextInternal; import org.apache.brooklyn.core.mgmt.rebind.RebindManagerImpl; import org.apache.brooklyn.core.objs.proxy.InternalFactory; import org.apache.brooklyn.core.relations.ByObjectBasicRelationSupport; +import org.apache.brooklyn.util.collections.MutableList; +import org.apache.brooklyn.util.collections.MutableSet; import org.apache.brooklyn.util.core.config.ConfigBag; import org.apache.brooklyn.util.core.flags.SetFromFlag; import org.apache.brooklyn.util.text.Identifiers; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterables; +import com.google.common.collect.Maps; +import com.google.common.collect.Sets; public abstract class AbstractBrooklynObject implements BrooklynObjectInternal { @@ -57,7 +56,7 @@ public abstract class AbstractBrooklynObject implements BrooklynObjectInternal { private String id = Identifiers.makeRandomLowercaseId(10); private String catalogItemId; - private Deque<String> searchPath = new ArrayDeque<>(); + private Collection<String> searchPath = MutableSet.of(); /** callers (only in TagSupport) should synchronize on this for all access */ @SetFromFlag("tags") @@ -86,18 +85,15 @@ public abstract class AbstractBrooklynObject implements BrooklynObjectInternal { _legacyConstruction = true; } - // TODO this will be overridden if the spec has a catalog item ID - // correct behaviour should be to inherit context's search path, perhaps, though maybe that's better done as spec? - // in any case, should not define it as _the_ catalog item ID; also see assignment based on parent - // in CatalogUtils.setCatalogItemIdOnAddition - setCatalogItemId(ApiObjectsFactory.get().getCatalogItemIdFromContext()); + // inherit any context for search purposes (though maybe that should be done when creating the spec?) + addSearchPath(MutableList.<String>of().appendIfNotNull(ApiObjectsFactory.get().getCatalogItemIdFromContext())); // rely on sub-class to call configure(properties), because otherwise its fields will not have been initialised } protected Object readResolve() { if (searchPath == null) { - searchPath = new ArrayDeque<>(); + searchPath = MutableList.of(); } return this; } @@ -210,22 +206,39 @@ public abstract class AbstractBrooklynObject implements BrooklynObjectInternal { @Override public void setCatalogItemIdAndSearchPath(String catalogItemId, List<String> ids) { setCatalogItemId(catalogItemId); - searchPath.clear(); - searchPath.addAll(ids); + synchronized (searchPath) { + searchPath.clear(); + searchPath.addAll(ids); + } + } + + @Override + public void addSearchPath(List<String> ids) { + synchronized (searchPath) { + searchPath.addAll(ids); + } } @Override public void stackCatalogItemId(String id) { if (null != id) { if (null != catalogItemId && !catalogItemId.equals(id)) { - searchPath.addFirst(catalogItemId); + synchronized (searchPath) { + Set<String> newPath = MutableSet.of(); + newPath.add(catalogItemId); + newPath.addAll(searchPath); + searchPath.clear(); + searchPath.addAll(newPath); + } } setCatalogItemId(id); } } public List<String> getCatalogItemIdSearchPath() { - return ImmutableList.copyOf(searchPath); + synchronized (searchPath) { + return ImmutableList.copyOf(searchPath); + } } @Override http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/88a09bae/core/src/main/java/org/apache/brooklyn/core/objs/BrooklynObjectInternal.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/objs/BrooklynObjectInternal.java b/core/src/main/java/org/apache/brooklyn/core/objs/BrooklynObjectInternal.java index d7fe17b..44ed3b8 100644 --- a/core/src/main/java/org/apache/brooklyn/core/objs/BrooklynObjectInternal.java +++ b/core/src/main/java/org/apache/brooklyn/core/objs/BrooklynObjectInternal.java @@ -39,6 +39,7 @@ public interface BrooklynObjectInternal extends BrooklynObject, Rebindable { void setCatalogItemId(String id); void setCatalogItemIdAndSearchPath(String catalogItemId, List<String> searchPath); + void addSearchPath(List<String> searchPath); /** * Moves the current catalog item id onto the start of the search path,
