brooklyn-server icon indicating copy to clipboard operation
brooklyn-server copied to clipboard

[WIP] deprecate Task as values in config and getImmediately(Task) in favour of TaskFactory

Open ahgittin opened this issue 8 years ago • 1 comments

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 Task is set as a config value: attempts to read it change the behaviour for subsequent reads. In particular a call to getImmediately (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) and getImmediately(task) -- to take TaskFactory instances 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 Task being used where a TaskFactory is now wanted. In particular DependentConfiguration returns Task instances! (There is a long-standing comment that DslComponent should 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-returning DependentConfig with TaskFactory alternatives; then in a subsequent version we can switch the deprecated methods to return TaskFactory: this means code such as config().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 .

ahgittin avatar Oct 04 '17 15:10 ahgittin

@ahgittin Can you take a look at this please to see if it is still relevant, and address the comments above if appropriate

Thanks

nakomis avatar Nov 26 '19 11:11 nakomis