From ba69dba8a3d45a78a64e7367553b1941966bf430 Mon Sep 17 00:00:00 2001 From: Alasdair Hodge Date: Wed, 2 Dec 2015 18:19:05 +0000 Subject: [PATCH 1/4] Resolve DeferredSuppliers when coercing config values. --- .../brooklyn/util/core/config/ConfigBag.java | 23 ++++++++++++++++--- .../util/core/flags/TypeCoercions.java | 5 ++++ 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/org/apache/brooklyn/util/core/config/ConfigBag.java b/core/src/main/java/org/apache/brooklyn/util/core/config/ConfigBag.java index 23a7485c47..94cfe8d0a5 100644 --- a/core/src/main/java/org/apache/brooklyn/util/core/config/ConfigBag.java +++ b/core/src/main/java/org/apache/brooklyn/util/core/config/ConfigBag.java @@ -32,6 +32,7 @@ import org.apache.brooklyn.util.collections.MutableMap; import org.apache.brooklyn.util.core.config.ConfigBag; import org.apache.brooklyn.util.core.flags.TypeCoercions; +import org.apache.brooklyn.util.core.task.DeferredSupplier; import org.apache.brooklyn.util.guava.Maybe; import org.apache.brooklyn.util.javalang.JavaClassNames; import org.slf4j.Logger; @@ -40,6 +41,7 @@ import com.google.common.annotations.Beta; import com.google.common.base.Objects; import com.google.common.collect.Sets; +import com.google.common.reflect.TypeToken; /** * Stores config in such a way that usage can be tracked. @@ -345,6 +347,16 @@ public T get(ConfigKey key) { return get(key, true); } + public T get(String key, Class targetType) { + return get(key, TypeToken.of(targetType)); + } + + public T get(String key, TypeToken targetType) { + markUsed(key); + Object rawValue = config.get(key); + return TypeCoercions.coerce(rawValue, targetType); + } + /** gets a value from a string-valued key or null; ConfigKey is preferred, but this is useful in some contexts (e.g. setting from flags) */ public Object getStringKey(String key) { return getStringKeyMaybe(key).orNull(); @@ -460,9 +472,14 @@ protected T get(ConfigKey key, boolean markUsed) { /** returns the first non-null value to be the type indicated by the key, or the keys default value if no non-null values are supplied */ public static T coerceFirstNonNullKeyValue(ConfigKey key, Object ...values) { - for (Object o: values) - if (o!=null) return TypeCoercions.coerce(o, key.getTypeToken()); - return TypeCoercions.coerce(key.getDefaultValue(), key.getTypeToken()); + Object rawValue = key.getDefaultValue(); + for (Object o: values) { + if (o != null) { + rawValue = o; + break; + } + } + return TypeCoercions.coerce(rawValue, key.getTypeToken()); } protected Object getStringKey(String key, boolean markUsed) { diff --git a/core/src/main/java/org/apache/brooklyn/util/core/flags/TypeCoercions.java b/core/src/main/java/org/apache/brooklyn/util/core/flags/TypeCoercions.java index f850fbd589..e4768ddfd4 100644 --- a/core/src/main/java/org/apache/brooklyn/util/core/flags/TypeCoercions.java +++ b/core/src/main/java/org/apache/brooklyn/util/core/flags/TypeCoercions.java @@ -55,6 +55,7 @@ import org.apache.brooklyn.util.collections.MutableSet; import org.apache.brooklyn.util.collections.QuorumCheck; import org.apache.brooklyn.util.collections.QuorumCheck.QuorumChecks; +import org.apache.brooklyn.util.core.task.DeferredSupplier; import org.apache.brooklyn.util.core.task.Tasks; import org.apache.brooklyn.util.exceptions.Exceptions; import org.apache.brooklyn.util.guava.Maybe; @@ -138,6 +139,10 @@ public static T coerce(Object value, TypeToken targetTypeToken) { if (value==null) return null; Class targetType = targetTypeToken.getRawType(); + if (value instanceof DeferredSupplier) { + value = ((DeferredSupplier) value).get(); + } + //recursive coercion of parameterized collections and map entries if (targetTypeToken.getType() instanceof ParameterizedType) { if (value instanceof Collection && Collection.class.isAssignableFrom(targetType)) { From b2302dce07da3461247ac073b1daf9116203f96d Mon Sep 17 00:00:00 2001 From: Alasdair Hodge Date: Wed, 2 Dec 2015 18:20:13 +0000 Subject: [PATCH 2/4] Instantiate locations in a task context, to permit resolution of certain DeferredSupplier config. --- .../mgmt/internal/LocalLocationManager.java | 68 +++++++++++++------ 1 file changed, 47 insertions(+), 21 deletions(-) diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/LocalLocationManager.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/LocalLocationManager.java index dcd1b434fe..ce4700aecf 100644 --- a/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/LocalLocationManager.java +++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/internal/LocalLocationManager.java @@ -23,6 +23,7 @@ import java.io.Closeable; import java.util.Collection; import java.util.Map; +import java.util.concurrent.Callable; import java.util.concurrent.atomic.AtomicLong; import org.apache.brooklyn.api.location.Location; @@ -37,20 +38,25 @@ import org.apache.brooklyn.core.internal.storage.BrooklynStorage; import org.apache.brooklyn.core.location.AbstractLocation; import org.apache.brooklyn.core.location.internal.LocationInternal; +import org.apache.brooklyn.core.mgmt.BrooklynTaskTags; import org.apache.brooklyn.core.mgmt.entitlement.Entitlements; import org.apache.brooklyn.core.objs.proxy.InternalLocationFactory; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; +import org.apache.brooklyn.util.collections.MutableMap; import org.apache.brooklyn.util.core.config.ConfigBag; +import org.apache.brooklyn.util.core.task.BasicExecutionContext; +import org.apache.brooklyn.util.core.task.BasicExecutionManager; import org.apache.brooklyn.util.core.task.Tasks; import org.apache.brooklyn.util.exceptions.Exceptions; import org.apache.brooklyn.util.exceptions.RuntimeInterruptedException; import org.apache.brooklyn.util.stream.Streams; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import com.google.common.annotations.Beta; import com.google.common.base.Preconditions; import com.google.common.base.Predicate; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Maps; public class LocalLocationManager implements LocationManagerInternal { @@ -63,6 +69,7 @@ public class LocalLocationManager implements LocationManagerInternal { private final LocalManagementContext managementContext; private final InternalLocationFactory locationFactory; + private final BasicExecutionContext executionContext; protected final Map locationsById = Maps.newLinkedHashMap(); private final Map preRegisteredLocationsById = Maps.newLinkedHashMap(); @@ -78,9 +85,13 @@ public class LocalLocationManager implements LocationManagerInternal { public LocalLocationManager(LocalManagementContext managementContext) { this.managementContext = checkNotNull(managementContext, "managementContext"); this.locationFactory = new InternalLocationFactory(managementContext); - + this.storage = managementContext.getStorage(); - locationTypes = storage.getMap("locations"); + this.locationTypes = storage.getMap("locations"); + + BasicExecutionManager executionManager = new BasicExecutionManager(managementContext.getManagementNodeId()); + ImmutableSet tags = ImmutableSet.of(managementContext, BrooklynTaskTags.TRANSIENT_TASK_TAG); + this.executionContext = new BasicExecutionContext(MutableMap.of("tags", tags), executionManager); } public InternalLocationFactory getLocationFactory() { @@ -90,27 +101,42 @@ public InternalLocationFactory getLocationFactory() { } @Override - public T createLocation(LocationSpec spec) { - try { - boolean createUnmanaged = ConfigBag.coerceFirstNonNullKeyValue(CREATE_UNMANAGED, - spec.getConfig().get(CREATE_UNMANAGED), spec.getFlags().get(CREATE_UNMANAGED.getName())); - if (createUnmanaged) { - spec.removeConfig(CREATE_UNMANAGED); + public T createLocation(final LocationSpec spec) { + // Ensure that the creation happens in the context of a task, so that any DeferredSupplier values + // can get hold of any necessary context objects via task tags. + Callable performCreation = new Callable() { + @Override public T call() { + boolean createUnmanaged = ConfigBag.coerceFirstNonNullKeyValue( + CREATE_UNMANAGED, + spec.getConfig().get(CREATE_UNMANAGED), + spec.getFlags().get(CREATE_UNMANAGED.getName()) + ); + if (createUnmanaged) { + spec.removeConfig(CREATE_UNMANAGED); + } + T loc = locationFactory.createLocation(spec); + if (!createUnmanaged) { + manage(loc); + } else { + // remove references + Location parent = loc.getParent(); + if (parent != null) { + ((AbstractLocation) parent).removeChild(loc); + } + preRegisteredLocationsById.remove(loc.getId()); + } + return loc; } + }; - T loc = locationFactory.createLocation(spec); - if (!createUnmanaged) { - manage(loc); + try { + // Only create a new task if we're not in the context of one already. + if (Tasks.current() == null) { + return executionContext.submit(performCreation).get(); } else { - // remove references - Location parent = loc.getParent(); - if (parent!=null) { - ((AbstractLocation)parent).removeChild(loc); - } - preRegisteredLocationsById.remove(loc.getId()); + return performCreation.call(); } - - return loc; + } catch (Throwable e) { log.warn("Failed to create location using spec "+spec+" (rethrowing)", e); throw Exceptions.propagate(e); From c9c7c6f23fc6eebb5c63ece64fd167b92fdef6f1 Mon Sep 17 00:00:00 2001 From: Alasdair Hodge Date: Tue, 8 Dec 2015 10:22:59 +0000 Subject: [PATCH 3/4] Fix test assertions in response to task-based location creation. --- .../core/location/LocationExtensionsTest.java | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/core/src/test/java/org/apache/brooklyn/core/location/LocationExtensionsTest.java b/core/src/test/java/org/apache/brooklyn/core/location/LocationExtensionsTest.java index dc25b9c6bd..2530d35f08 100644 --- a/core/src/test/java/org/apache/brooklyn/core/location/LocationExtensionsTest.java +++ b/core/src/test/java/org/apache/brooklyn/core/location/LocationExtensionsTest.java @@ -29,6 +29,8 @@ import org.apache.brooklyn.core.entity.Entities; import org.apache.brooklyn.core.location.AbstractLocation; import org.apache.brooklyn.core.test.entity.LocalManagementContextForTests; +import org.apache.brooklyn.util.exceptions.Exceptions; +import org.apache.brooklyn.util.exceptions.PropagatedRuntimeException; import org.testng.annotations.AfterMethod; import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; @@ -164,21 +166,24 @@ public void testAddExtensionThroughLocationSpecIllegally() { try { Location loc = createConcrete(MyExtension.class, "not an extension"); fail("loc="+loc); - } catch (IllegalArgumentException e) { + } catch (Exception e) { + assertTrue(Exceptions.getFirstInteresting(e) instanceof IllegalArgumentException); // success } try { Location loc = createConcrete(MyExtension.class, null); fail("loc="+loc); - } catch (NullPointerException e) { + } catch (Exception e) { + assertTrue(Exceptions.getFirstInteresting(e) instanceof NullPointerException); // success } try { Location loc = createConcrete(null, extension); fail("loc="+loc); - } catch (NullPointerException e) { + } catch (Exception e) { + assertTrue(Exceptions.getFirstInteresting(e) instanceof NullPointerException); // success } } From 7b7e0e0dddbb1738054b25f99a8f32d8d38a5a06 Mon Sep 17 00:00:00 2001 From: Alasdair Hodge Date: Mon, 7 Dec 2015 12:46:12 +0000 Subject: [PATCH 4/4] Fix JcloudsLocation to use type-coercing get() method. --- .../apache/brooklyn/location/jclouds/JcloudsLocation.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java b/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java index 5afc3b3545..f3efcd5a5d 100644 --- a/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java +++ b/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java @@ -2067,9 +2067,9 @@ private static class RebindToMachinePredicate implements Predicate