[WIP] Add DSL support for calling effectors in YAML #155
Conversation
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object obj) { |
There was a problem hiding this comment.
Should equals include the args? Not sure when equals would actually be used in anger.
|
You said: "It is hard to determine when the effector will be executed". But it will follow the same rules as for other deferred config: when someone attempts to retrieve the config value, then it will be executed. So we can use all the standard |
|
I'm not sure I follow when you say: "This would require a version of the function that returned a Task<?> rather than a value directly, perhaps via another new function like $brooklyn:task()?" Can you elaborate on that use-case, and why you'd need this? |
|
I was thinking we might configure this with: You could imagine (in the future) supporting more options such as timeout, non-blocking, on-error, etc. Putting the arguments inside "args" gives us better extensibility for the future, I think. |
|
@aledsage I'll post a message to the Brooklyn dev list that answers your questions more completely. I like the idea for combining the arguments in an |
|
I have pulled this and was doing some tests around it. I noticed that given the test YAML and a test that does a getConfig, which should evaluate the DSL and invoke the effector: the expectation is that the call history on the test entity should contain one call to “myEffector”. In fact it contains two, because the config item is also evaluated during validation in construction of the entity, in a task kicked off from stack trace I've been looking at updating the ValueResolver to be a bit smarter about this but need to do a bit more testing on it before doing a PR. |
|
I've PRed my changes to Andrew's branch but can re-do this to master if preferred, once Andrew's is merged. |
|
@geomacy I have merged your changes and rebased to master |
|
Just noting the issues with this that we observed earlier, to do with effector invocations with parameters that are DSL expressions: At present there is no ability to have multi line effector invocations, which would enable simple writing of effector calls where the parameters were themselves DSL expressions. See disabled test "testEffectorMultiLine" in EffectorsYamlTest.java. Also when doing such invocations on one line there are problems. The test EffectorsYamlIntegrationTest fails. This appears to be a byproduct of 8bfff1635c Fix config().getNonBlocking for MapConfigKey. It is cancelling the subtask for the effector call during the validation phase of the blueprint (per attached), before it gets evaluated when it’s meant to be. See attached stack trace. |
|
@aledsage iirc I think this can be merged in its present state but it would be worth recording a todo item for the points noted in #155 (comment). |
| if (targetEffector.isAbsentOrNull()) { | ||
| throw new IllegalArgumentException("Effector " + effectorName + " not found on entity: " + targetEntity); | ||
| } | ||
| if (cachedTask == null) { |
There was a problem hiding this comment.
cachedTask is accessed from multiple threads. Should we use some locking to make sure we are creating it only once?
this includes the following changes: An "avoidSideEffects" capability in ValueResolver, to allow callers to specify that any tasks which are marked as having side effects (a new marker Interface) should be ignored. The use case for this is illustrated by its use here in AbstractConfigurationSupportInternal in the getNonBlocking method invoked by ConfigConstraints.validateAll. This is called during validation of the YAML, and evaluates all config key values, which without this change would result in unwanted repeat executions of effectors (and at a point when their target entities have not yet been started). Similarly it is called during calculation of a hash for the extra salt for VanillaSoftwareProcessSshDriver, again a situation where we do not want effectors being called. An additional effector() method on BrooklynDslCommon, to add support for varArgs passing of the effector arguments, and a matching new method on BrooklynDslDeferredSupplier. The task created by ExecuteEffector is cached, in order to avoid unwanted repeated executor invocations, as illustrated in the test testEffectorCalledOncePerConfigKey in EffectorsYamlTest. test-app-with-effectors.yaml is removed as it's easier to see what's going on with the YAML inline within the tests. New tests are added in EffectorsYamlTest. A new 'sequenceEffector' is added, just added directly to TestEntity, in case it happens to be useful for anyone else.
Also adds a formatString test that was used to get the right structure for the entity test, may as well include it now that it is written.
|
@drigodwin I had to pull in grkvlt#2 manually but it is there in 70d9aca so could you have a look please? |
|
Ouch -- I don't think this is a good idea. Apologies, I hadn't reviewed it because it was marked
And there is a minor but aggravating issue that there are other PRs I have being reviewed which clean up immediately semantics and this clobbers them. I think it will be easier to put this on top of those if we decide we need it. |
|
@drigodwin @grkvlt @aledsage @neykov WDYT? ^ could we revert? |
|
I agree this should probably not have been marked as a WIP @ahgittin . This initially came about through work on the CA entity in clocker, there was no good way to share keys between entities. I have no problem reverting it but I think it is a useful addition which we shouldn't throw away without discussion. |
|
right, so the use case is one entity wanting to get information from another. is there no way this can be accommodated using sensors? my philosophical objection is that we're introducing first-class support for a new class of dependency injection:
a widespread use of 2 scares me and it's worth avoiding this if at all possible. it also means lookups aren't idempotent (which is why the could your problem with the CA entity be solved another way, if not with waiting on a sensor, by the config pointing at the source entity rather than the key value itself, and whenever it is accessed there is code which invokes the effector to get the key on that source entity? |
|
@drigodwin @grkvlt Reply requested this morning. I'm going to revert at noon if I hear nothing. Something of this magnitude should have been discussed on list, and definitely should not have been merged while still apparently in a |
|
On balance, I agree with @ahgittin that we should revert this, and first merge #480 The changes in this PR were suggested on the mailing list as part of @grkvlt's message "[PROPOSAL] Enabling Effective Effectors" on 30/05/2016, but it wasn't properly discussed there. After reverting this, we need to kick off that discussion on the mailing list again. We need to more clearly describe the use-case that motivated this change. We can then discuss the philosophy behind it. For clarification, I believe the intent for the CA-server use-case was that the effector call happens only once; subsequent queries for the config key gets the value from that initial invocation (or blocks until the effector returns, if another thread is still executing it). But let's discuss that more on the mailing list. |
|
Okay, the semantics that The ideal way to do this I think would be to get on to the then wherever you need the "populate config on startup" behaviour you add the above as a child that starts in parallel and you set the desired config to |
|
+1 to reverting, but retaining the branch for possible future use after more discussion. |
|
reverted and pushed, with |
8 participants
Adds a new DSL function
$brooklyn:effectorthat takes two parameters, a string with the effector name and an optional map of the named effector arguments. Use in a blueprint wherever you can substitute a$brooklynfunction, like this:See #153 for a complementary mechanism to create effectors in YAML directly.
Issues
It is hard to determine when the effector will be executed, but lazy evaluation can be effected by assigning the result of an
attributeWhenReady()call to one of the effector arguments. It may be useful to have a version of the function that explicitly waits until some sensor has a specific value. Different predicates could be defined for start conditions, or implicit or explicit latches could be used. It would also be interesting to allow the tasks created when running an effector to be used in places like theinstall.commandconfiguration of aVanillaSoftwareProcessin place of the SSH command executing task. This would require a version of the function that returned aTask<?>rather than a value directly, perhaps via another new function like$brooklyn:task()?Further work is still required to complete this feature, including more tests and documentation.