Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/816
For `executionContext.getImmediately()` if given an unsubmitted task, I
think it is probably better to throw an `ImmediateUnsupportedException` rather
than break the task.
There are two main places we use `getImmediately`, as far as i recall: one
is the UI to show config values; the second is in an enricher that is
triggered, where it's target value is set as something like
`$brooklyn:formatString("...%s", $brooklyn:config("mykey"))`. In both cases,
it's better to not get the value than to break all subsequent use of that
config key (because the task is cancelled, so its value will never be
available).
---
Not sure what you mean by the `allowImmediateExecution` in `ValueResolver`.
That's just a variable that is true if `v instanceof ImmediateSupplier || v
instanceof DeferredSupplier || v instanceof TaskFactory`.
I think your code in `ValueResolver` is fine now (with it calling `submit`
if immediately evaluating a task that was passed in). Hopefully that won't lead
to any "one-off jobs (eg config validation) that were leaking". I don't recall
the details of exactly why those leaked.
I suspect the number of times we pass a `Task` to resolve immediately is
either zero or close to zero, so it's fine.
---
Regarding ranting and improving it by centralising the logic: agreed :-)
I stand by the "accidental", but agree it's not about this PR and it's more
complicated. We (i.e. brooklyn committers) sometimes add a better way of doing
something, or we just stop using some old functionality because the use-case
turned out to be of less interest than we thought. Sometimes that old code just
sits around - which leads to a bit of confusion for new contributors. Sometimes
it gets deprecated - which is not too bad. Sometimes it is worse in that the
code paths of that original code are deep within the code-base. We continue to
support those low-level code paths because we don't want to break things,
including when refactoring and adding more things.
My suspicion is that support for `getImmediately` on a task is one of those
latter examples. Going further, do we even want to support a config value of
type `Task` (which is not really persistable - we only persist it right if the
task has completed, where we switch it out for the result value, if I recall
correctly!)? I suspect that code path only gets used when one calls things like
the `DependentConfiguration.*` methods directly. A long time ago we used to set
config values directly with
`DependentConfiguration.attributePostProcessedWhenReady` but hopefully we never
do that any more!
That's one for the mailing list: should we fail-fast if you try to set
config value that is a `Task`. And it's one to experiment with: if we disallow
it, what side effects does it have and do our QA runs still work.
---
