diff --git a/api/src/main/java/org/apache/brooklyn/api/mgmt/ExecutionContext.java b/api/src/main/java/org/apache/brooklyn/api/mgmt/ExecutionContext.java index bb95ea9eed..607b4f12aa 100644 --- a/api/src/main/java/org/apache/brooklyn/api/mgmt/ExecutionContext.java +++ b/api/src/main/java/org/apache/brooklyn/api/mgmt/ExecutionContext.java @@ -97,8 +97,12 @@ public interface ExecutionContext extends Executor { @Beta Maybe getImmediately(Object callableOrSupplierOrTask); /** As {@link #getImmediately(Object)} but strongly typed for a task. */ + // TODO deprecate @Beta - Maybe getImmediately(Task callableOrSupplierOrTask); + Maybe getImmediately(TaskAdaptable callableOrSupplierOrTask); + /** As {@link #getImmediately(Object)} but strongly typed for a task factory. */ + @Beta + Maybe getImmediately(TaskFactory> callableOrSupplierOrTask); /** * Efficient implementation of common case when {@link #submit(TaskAdaptable)} is followed by an immediate {@link Task#get()}. diff --git a/api/src/main/java/org/apache/brooklyn/api/objs/Configurable.java b/api/src/main/java/org/apache/brooklyn/api/objs/Configurable.java index 40de655527..5208d0e286 100644 --- a/api/src/main/java/org/apache/brooklyn/api/objs/Configurable.java +++ b/api/src/main/java/org/apache/brooklyn/api/objs/Configurable.java @@ -20,7 +20,10 @@ import java.util.Set; +import org.apache.brooklyn.api.mgmt.ExecutionContext; import org.apache.brooklyn.api.mgmt.Task; +import org.apache.brooklyn.api.mgmt.TaskAdaptable; +import org.apache.brooklyn.api.mgmt.TaskFactory; import org.apache.brooklyn.config.ConfigKey; import org.apache.brooklyn.config.ConfigKey.HasConfigKey; import org.apache.brooklyn.config.ConfigMap; @@ -78,19 +81,39 @@ public interface ConfigurationSupport { T set(HasConfigKey key, T val); /** - * Sets the config to the value returned by the task. + * Sets the config to return the value returned by the task this generates, + * on demand when invoked. + *

+ * Returns immediately without blocking; subsequent calls to {@link #getConfig(ConfigKey)} + * will create and execute an instance of the task, blocking unless otherwise configured. * + * @see {@link #setConfig(ConfigKey, Object)} + */ + T set(ConfigKey key, TaskFactory> val); + + /** + * Sets the config to the value returned by the task. + *

+ * It is recommended to use {@link #set(ConfigKey, TaskFactory)} instead so that calls to validate + * items via {@link ExecutionContext#getImmediately(Object)} do not interrupt and wreck the task set here. + *

* Returns immediately without blocking; subsequent calls to {@link #getConfig(ConfigKey)} * will execute the task, and block until the task completes. * * @see {@link #setConfig(ConfigKey, Object)} */ - T set(ConfigKey key, Task val); + T set(ConfigKey key, TaskAdaptable val); + + /** + * @see {@link #setConfig(ConfigKey, TaskFactory)} + */ + T set(HasConfigKey key, TaskFactory> val); /** * @see {@link #setConfig(ConfigKey, Task)} */ - T set(HasConfigKey key, Task val); + // TODO deprecate + T set(HasConfigKey key, TaskAdaptable val); /** @deprecated since 0.11.0 see {@link ConfigMap#findKeys(Predicate)} */ @Deprecated diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ObjectsYamlTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ObjectsYamlTest.java index 85aed953a4..fc32f71721 100644 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ObjectsYamlTest.java +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ObjectsYamlTest.java @@ -27,7 +27,8 @@ import org.apache.brooklyn.api.entity.Entity; import org.apache.brooklyn.api.mgmt.ManagementContext; -import org.apache.brooklyn.api.mgmt.Task; +import org.apache.brooklyn.api.mgmt.TaskAdaptable; +import org.apache.brooklyn.api.mgmt.TaskFactory; import org.apache.brooklyn.api.objs.Configurable; import org.apache.brooklyn.config.ConfigKey; import org.apache.brooklyn.config.ConfigKey.HasConfigKey; @@ -177,12 +178,22 @@ public T set(HasConfigKey key, T val) { } @Override - public T set(ConfigKey key, Task val) { + public T set(ConfigKey key, TaskFactory> val) { throw new UnsupportedOperationException(); } @Override - public T set(HasConfigKey key, Task val) { + public T set(HasConfigKey key, TaskFactory> val) { + return set(key.getConfigKey(), val); + } + + @Override + public T set(ConfigKey key, TaskAdaptable val) { + throw new UnsupportedOperationException(); + } + + @Override + public T set(HasConfigKey key, TaskAdaptable val) { return set(key.getConfigKey(), val); } diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/DslTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/DslTest.java index 04b6bf124a..8f575d8ed8 100644 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/DslTest.java +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/DslTest.java @@ -209,7 +209,7 @@ public void testConfigUsesParameterDefaultValue() throws Exception { public void testConfigImmediatelyDoesNotBlock() throws Exception { ConfigKey configKey = ConfigKeys.newStringConfigKey("testConfig"); BrooklynDslDeferredSupplier attributeDsl = BrooklynDslCommon.attributeWhenReady(TestApplication.MY_ATTRIBUTE.getName()); - app.config().set((ConfigKey)configKey, attributeDsl); // ugly cast because val is DSL, resolving to a string + app.config().set((ConfigKey)configKey, (Object)attributeDsl); // ugly cast because val is DSL, resolving to a string BrooklynDslDeferredSupplier configDsl = BrooklynDslCommon.config(configKey.getName()); Maybe actualValue = execDslImmediately(configDsl, configKey.getType(), app, true); assertTrue(actualValue.isAbsent()); diff --git a/core/src/main/java/org/apache/brooklyn/core/effector/EffectorTasks.java b/core/src/main/java/org/apache/brooklyn/core/effector/EffectorTasks.java index db49227738..72ad94bdd4 100644 --- a/core/src/main/java/org/apache/brooklyn/core/effector/EffectorTasks.java +++ b/core/src/main/java/org/apache/brooklyn/core/effector/EffectorTasks.java @@ -27,6 +27,7 @@ import org.apache.brooklyn.api.entity.Entity; import org.apache.brooklyn.api.mgmt.Task; import org.apache.brooklyn.api.mgmt.TaskAdaptable; +import org.apache.brooklyn.api.mgmt.TaskFactory; import org.apache.brooklyn.config.ConfigKey; import org.apache.brooklyn.core.config.ConfigKeys; import org.apache.brooklyn.core.location.Machines; @@ -57,9 +58,14 @@ public class EffectorTasks { private static final Logger log = LoggerFactory.getLogger(EffectorTasks.class); public interface EffectorTaskFactory { + // TODO deprecate public abstract TaskAdaptable newTask(Entity entity, Effector effector, ConfigBag parameters); } + public interface EffectorTaskFactoryFactory { + public abstract TaskFactory> newTaskFactory(Entity entity, Effector effector, ConfigBag parameters); + } + /** wrapper for {@link EffectorBody} which simply runs that body on each invocation; * the body must be thread safe and ideally stateless */ public static class EffectorBodyTaskFactory implements EffectorTaskFactory { @@ -70,28 +76,52 @@ public EffectorBodyTaskFactory(EffectorBody effectorBody) { @Override public Task newTask(final Entity entity, final org.apache.brooklyn.api.effector.Effector effector, final ConfigBag parameters) { - final AtomicReference> dst = new AtomicReference>(); - - dst.set(new DynamicSequentialTask( - getFlagsForTaskInvocationAt(entity, effector, parameters), - new Callable() { - @Override - public T call() throws Exception { - try { - DynamicTasks.setTaskQueueingContext(dst.get()); - return effectorBody.call(parameters); - } finally { - DynamicTasks.removeTaskQueueingContext(); - } - } - }) { - @Override - public void handleException(Throwable throwable) throws Exception { - throw EffectorUtils.handleEffectorException(entity, effector, throwable); - } - }); - return dst.get(); + return newTaskFactory(entity, effector, parameters).newTask(); }; + + public TaskFactory> newTaskFactory(final Entity entity, final org.apache.brooklyn.api.effector.Effector effector, final ConfigBag parameters) { + return new RealTaskFactory(entity, effector, parameters, effectorBody, getFlagsForTaskInvocationAt(entity, effector, parameters)); + }; + + private static class RealTaskFactory implements TaskFactory> { + final Entity entity; + final Effector effector; + final ConfigBag parameters; + final EffectorBody effectorBody; + final Map invocationFlags; + public RealTaskFactory(Entity entity, Effector effector, ConfigBag parameters, EffectorBody effectorBody, Map invocationFlags) { + this.entity = entity; + this.effector = effector; + this.parameters = parameters; + this.effectorBody = effectorBody; + this.invocationFlags = invocationFlags; + } + + @Override + public Task newTask() { + final AtomicReference> dst = new AtomicReference>(); + + dst.set(new DynamicSequentialTask( + invocationFlags, + new Callable() { + @Override + public T call() throws Exception { + try { + DynamicTasks.setTaskQueueingContext(dst.get()); + return effectorBody.call(parameters); + } finally { + DynamicTasks.removeTaskQueueingContext(); + } + } + }) { + @Override + public void handleException(Throwable throwable) throws Exception { + throw EffectorUtils.handleEffectorException(entity, effector, throwable); + } + }); + return dst.get(); + } + } /** subclasses may override to add additional flags, but they should include the flags returned here * unless there is very good reason not to; default impl returns a MutableMap */ diff --git a/core/src/main/java/org/apache/brooklyn/core/objs/AbstractConfigurationSupportInternal.java b/core/src/main/java/org/apache/brooklyn/core/objs/AbstractConfigurationSupportInternal.java index f67f1f5dac..4c775084d7 100644 --- a/core/src/main/java/org/apache/brooklyn/core/objs/AbstractConfigurationSupportInternal.java +++ b/core/src/main/java/org/apache/brooklyn/core/objs/AbstractConfigurationSupportInternal.java @@ -27,6 +27,8 @@ import org.apache.brooklyn.api.mgmt.ExecutionContext; import org.apache.brooklyn.api.mgmt.Task; +import org.apache.brooklyn.api.mgmt.TaskAdaptable; +import org.apache.brooklyn.api.mgmt.TaskFactory; import org.apache.brooklyn.api.objs.BrooklynObject; import org.apache.brooklyn.config.ConfigKey; import org.apache.brooklyn.config.ConfigKey.HasConfigKey; @@ -141,7 +143,12 @@ protected Maybe getNonBlockingResolvingSimple(ConfigKey key) { } @Override - public T set(HasConfigKey key, Task val) { + public T set(HasConfigKey key, TaskFactory> val) { + return set(key.getConfigKey(), val); + } + + @Override + public T set(HasConfigKey key, TaskAdaptable val) { return set(key.getConfigKey(), val); } @@ -176,10 +183,19 @@ public T set(ConfigKey key, T val) { } @Override - public T set(ConfigKey key, Task val) { + public T set(ConfigKey key, TaskAdaptable val) { + LOG.warn("Setting a task as a config value is deprecated; use a TaskFactory instead, " + + "when setting "+getContainer()+" "+key+" to "+val); + LOG.debug("Trace for setting "+getContainer()+" "+key+" to task "+val, + new Exception("Trace for setting "+getContainer()+" "+key+" to task "+val)); return setConfigInternal(key, val); } + @Override + public T set(ConfigKey key, TaskFactory> val) { + return setConfigInternal(key, val); + } + @Override public ConfigBag getLocalBag() { return ConfigBag.newInstance(getConfigsInternal().getAllConfigLocalRaw()); diff --git a/core/src/main/java/org/apache/brooklyn/core/objs/BasicConfigurableObject.java b/core/src/main/java/org/apache/brooklyn/core/objs/BasicConfigurableObject.java index 3c28fc0e47..b9e13f01aa 100644 --- a/core/src/main/java/org/apache/brooklyn/core/objs/BasicConfigurableObject.java +++ b/core/src/main/java/org/apache/brooklyn/core/objs/BasicConfigurableObject.java @@ -22,6 +22,8 @@ import org.apache.brooklyn.api.mgmt.ManagementContext; import org.apache.brooklyn.api.mgmt.Task; +import org.apache.brooklyn.api.mgmt.TaskAdaptable; +import org.apache.brooklyn.api.mgmt.TaskFactory; import org.apache.brooklyn.api.objs.Configurable; import org.apache.brooklyn.api.objs.Identifiable; import org.apache.brooklyn.config.ConfigKey; @@ -109,12 +111,22 @@ public T set(HasConfigKey key, T val) { } @Override - public T set(ConfigKey key, Task val) { - throw new UnsupportedOperationException(); + public T set(ConfigKey key, TaskFactory> val) { + throw new UnsupportedOperationException("Setting tasks only supported on subtypes"); } @Override - public T set(HasConfigKey key, Task val) { + public T set(HasConfigKey key, TaskFactory> val) { + return set(key.getConfigKey(), val); + } + + @Override + public T set(ConfigKey key, TaskAdaptable val) { + throw new UnsupportedOperationException("Setting tasks only supported on subtypes"); + } + + @Override + public T set(HasConfigKey key, TaskAdaptable val) { return set(key.getConfigKey(), val); } diff --git a/core/src/main/java/org/apache/brooklyn/core/sensor/DependentConfiguration.java b/core/src/main/java/org/apache/brooklyn/core/sensor/DependentConfiguration.java index 6a42aa26a4..da63f55038 100644 --- a/core/src/main/java/org/apache/brooklyn/core/sensor/DependentConfiguration.java +++ b/core/src/main/java/org/apache/brooklyn/core/sensor/DependentConfiguration.java @@ -102,8 +102,15 @@ * *

* Note that these methods return one-time tasks. The DslComponent methods return long-lasting pointers - * and should now normally be used instead. + * and should now normally be used instead. If using these methods that return {@link Task} instances + * you should either ensure it is submitted or that validation and any other attempts to + * {@link ExecutionContext#getImmediately(TaskAdaptable)} on this task is blocked; + * it is all to easy otherwise for an innocuous immediate-get to render this task interrupted */ +// possibly even the REST API just looking at config could cancel? +// TODO should we deprecate these and provide variants that return TaskFactory instances? +// maybe even ones that are nicely persistable? i (alex) think so, 2017-10. +// see https://github.com/apache/brooklyn-server/pull/816#issuecomment-333858098 public class DependentConfiguration { private static final Logger LOG = LoggerFactory.getLogger(DependentConfiguration.class); diff --git a/core/src/main/java/org/apache/brooklyn/util/core/task/BasicExecutionContext.java b/core/src/main/java/org/apache/brooklyn/util/core/task/BasicExecutionContext.java index f81ad35b20..90043f0572 100644 --- a/core/src/main/java/org/apache/brooklyn/util/core/task/BasicExecutionContext.java +++ b/core/src/main/java/org/apache/brooklyn/util/core/task/BasicExecutionContext.java @@ -40,6 +40,7 @@ import org.apache.brooklyn.api.mgmt.HasTaskChildren; import org.apache.brooklyn.api.mgmt.Task; import org.apache.brooklyn.api.mgmt.TaskAdaptable; +import org.apache.brooklyn.api.mgmt.TaskFactory; import org.apache.brooklyn.api.mgmt.entitlement.EntitlementContext; import org.apache.brooklyn.core.entity.EntityInternal; import org.apache.brooklyn.core.mgmt.BrooklynTaskTags; @@ -244,20 +245,39 @@ private Maybe runInSameThread(final Task task, Callable> job) } @Override - public Maybe getImmediately(Task callableOrSupplier) { - return getImmediately((Object) callableOrSupplier); + public Maybe getImmediately(TaskFactory> callableOrSupplier) { + return getImmediatelyInternal(callableOrSupplier); + } + + @Override + public Maybe getImmediately(TaskAdaptable callableOrSupplier) { + if (!(callableOrSupplier instanceof TaskFactory)) { + Task t = callableOrSupplier.asTask(); + if (!t.isSubmitted()) { + log.warn("Deprecated call to getImmediately on unsubmitted task "+callableOrSupplier+"; " + + "this will run the task in a crippled state. Pass a TaskFactory instead or submit the task beforehand."); + log.debug("Trace for deprecated call to getImmediately("+callableOrSupplier+")", + new Exception("Trace for deprecated call to getImmediately("+callableOrSupplier+")")); + } + } + return getImmediatelyInternal(callableOrSupplier); } /** performs execution without spawning a new task thread, though it does temporarily set a fake task for the purpose of getting context; - * currently supports {@link Supplier}, {@link Callable}, {@link Runnable}, or {@link Task} instances; + * currently supports {@link Supplier}, {@link Callable}, {@link Runnable}, + * or {@link Task} (recommended to be {@link TaskFactory}) instances; * with tasks if it is submitted or in progress, * it fails if not completed; with unsubmitted, unqueued tasks, it gets the {@link Callable} job and * uses that; with such a job, or any other callable/supplier/runnable, it runs that * in an {@link InterruptingImmediateSupplier}, with as much metadata as possible (eg task name if * given a task) set temporarily in the current thread context */ - @SuppressWarnings("unchecked") @Override public Maybe getImmediately(Object callableOrSupplier) { + return getImmediatelyInternal(callableOrSupplier); + } + + @SuppressWarnings("unchecked") + private Maybe getImmediatelyInternal(Object callableOrSupplier) { BasicTask fakeTaskForContext; if (callableOrSupplier instanceof BasicTask) { fakeTaskForContext = (BasicTask)callableOrSupplier; @@ -269,8 +289,12 @@ public Maybe getImmediately(Object callableOrSupplier) { } } callableOrSupplier = fakeTaskForContext.getJob(); - } else if (callableOrSupplier instanceof TaskAdaptable) { - return getImmediately( ((TaskAdaptable)callableOrSupplier).asTask() ); + } else if (callableOrSupplier instanceof TaskFactory) { + return getImmediatelyInternal( ((TaskFactory>)callableOrSupplier).newTask() ); + } else if (callableOrSupplier instanceof TaskAdaptable && !(callableOrSupplier instanceof Task)) { + // if it's another form of task go into the next block where we will likely fail; + // not supported to get-immediate tasks that aren't basic tasks + return getImmediatelyInternal( ((TaskAdaptable)callableOrSupplier).asTask() ); } else { fakeTaskForContext = new BasicTask(MutableMap.of("displayName", "Immediate evaluation")); } diff --git a/core/src/test/java/org/apache/brooklyn/util/core/internal/FlagUtilsTest.java b/core/src/test/java/org/apache/brooklyn/util/core/internal/FlagUtilsTest.java index b4ace3afaf..f3406c8f95 100644 --- a/core/src/test/java/org/apache/brooklyn/util/core/internal/FlagUtilsTest.java +++ b/core/src/test/java/org/apache/brooklyn/util/core/internal/FlagUtilsTest.java @@ -29,7 +29,8 @@ import java.util.Map; import java.util.Set; -import org.apache.brooklyn.api.mgmt.Task; +import org.apache.brooklyn.api.mgmt.TaskAdaptable; +import org.apache.brooklyn.api.mgmt.TaskFactory; import org.apache.brooklyn.api.objs.Configurable; import org.apache.brooklyn.config.ConfigKey; import org.apache.brooklyn.config.ConfigKey.HasConfigKey; @@ -394,12 +395,22 @@ public T set(HasConfigKey key, T val) { } @Override - public T set(ConfigKey key, Task val) { + public T set(ConfigKey key, TaskFactory> val) { throw new UnsupportedOperationException(); } @Override - public T set(HasConfigKey key, Task val) { + public T set(HasConfigKey key, TaskFactory> val) { + return set(key.getConfigKey(), val); + } + + @Override + public T set(ConfigKey key, TaskAdaptable val) { + throw new UnsupportedOperationException(); + } + + @Override + public T set(HasConfigKey key, TaskAdaptable val) { return set(key.getConfigKey(), val); }