[WIP] deprecate Task as values in config and getImmediately(Task) in favour of TaskFactory #853
Conversation
but warns and suggests deprecation for many things which aren't yet modernized
aledsage
left a comment
aledsage
left a comment
There was a problem hiding this comment.
Looks like a good start. I think we really can deprecate things like config().set(ConfigKey, Task) now. Looking at where it's called with such a strongly typed value of type Task, there is only one place (in CassandraNodeSshDriver) plus tests. Not sure how many other places would give us warnings from loosely typed stuff though!
| <T> Maybe<T> getImmediately(TaskAdaptable<T> callableOrSupplierOrTask); | ||
| /** As {@link #getImmediately(Object)} but strongly typed for a task factory. */ | ||
| @Beta | ||
| <T> Maybe<T> getImmediately(TaskFactory<Task<T>> callableOrSupplierOrTask); |
| * @see {@link #setConfig(ConfigKey, Object)} | ||
| */ | ||
| <T> T set(ConfigKey<T> key, Task<T> val); | ||
| <T> T set(ConfigKey<T> key, TaskAdaptable<T> val); |
There was a problem hiding this comment.
Add TODO Deprecate here as well.
Some power-users might implement ConfigurationSupport, which this will impact, but that's fine: it's marked @Beta.
| private static final Logger log = LoggerFactory.getLogger(EffectorTasks.class); | ||
|
|
||
| public interface EffectorTaskFactory<T> { | ||
| // TODO deprecate |
There was a problem hiding this comment.
How does this relate to config of type Task? I thought that EffectorTaskFactory would be used in an entirely different context.
| // 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 { |
There was a problem hiding this comment.
In DslComponent, it calls to things like DependentConfiguration.attributeWhenReady(targetEntity, (AttributeSensor<?>)targetSensor) for its implementation of newTask.
I think we want to keep this code, but for people to not use it directly! I presume the way to do that (sticking to our deprecation policy) would be to create a new internal class (e.g. that delegates to it), and to deprecate this class.
I've always wondered about our DslComponent class. It feels like there's a big chunk of code in brooklyn-camp that could live in core. Things like the DslComponent.AttributeWhenReady class no longer has anything to do with the original yaml. We could move that to core (preserving backwards compatibility with persisted state obviously).
I don't think there's a need for any other new code that returns TaskFactory instances - we should re-use the existing DslComponent code as much as possible.
| public <T> Maybe<T> getImmediately(TaskAdaptable<T> callableOrSupplier) { | ||
| if (!(callableOrSupplier instanceof TaskFactory)) { | ||
| Task<T> t = callableOrSupplier.asTask(); | ||
| if (!t.isSubmitted()) { |
There was a problem hiding this comment.
I'd like us to go further and say that all calls that pass in a task are deprecated. But this is a good first step.
I'd also want us to warn about deprecated usage if someone called getImmediately((Object)task)
| * given a task) set <i>temporarily</i> in the current thread context */ | ||
| @SuppressWarnings("unchecked") | ||
| @Override | ||
| public <T> Maybe<T> getImmediately(Object callableOrSupplier) { |
There was a problem hiding this comment.
Would like to say that passing in a Task is deprecated.
|
@ahgittin Can you take a look at this please to see if it is still relevant, and address the comments above if appropriate Thanks |
3 participants
As discussed in #816 and suggested to be fixed in #835 but pushed out to here:
There are bad things that can happen if an unsubmitted
Taskis set as a config value: attempts to read it change the behaviour for subsequent reads. In particular a call togetImmediately(which can be done during validation, or maybe even during a REST read) can cause it to be submitted in an interrupted state, which damages it for all future use. But more generally, once it has been evaluated it will always and only return that value, where a user may sensibly expect it to return the current value on each read.Changing those methods --
config().set(key, task)andgetImmediately(task)-- to takeTaskFactoryinstances instead removes this confusion and problems, making it clear that a new task is run on each execution.However this is a big job due to the prevalence of
Taskbeing used where aTaskFactoryis now wanted. In particularDependentConfigurationreturnsTaskinstances! (There is a long-standing comment thatDslComponentshould be used instead, as that generates factories internal, and YAML use behaves as proposed to implement here across the board, so it is mainly a concern internally and for people using the Java API. Still it is a clean-up worth doing.)I've committed one set of changes so far (it is just one commit beyond the merge of #835), signposting the direction this will go and logging warnings in the deprecated code paths. Next I will actually deprecate these and update usages.
In particular I will likely deprecate all
Task-returningDependentConfigwithTaskFactoryalternatives; then in a subsequent version we can switch the deprecated methods to returnTaskFactory: this means code such asconfig().set(key, attributeWhenReady(...))although it will be deprecated for one release will be migrated with no effort from a user to the new behaviour.Comments at this early stage very welcome cc @aledsage @sjcorbett @grkvlt .