DSL deferred execution #482

neykov · 2016-12-08T15:53:35Z

Allows DSL to be applied on the return value of attributeWhenReady and config calls. Before the change the DSL calls are made on the AttributeWhenReady and DslConfigSupplier objects correspondingly. After the changes the value will first be resolved and then the DSL evaluated on result. DslComponent is a special case where it will not be resolved before evaluating the DSL as that's the expected behaviour.

Allows us to call custom DSL methods on any object we are interested in without the object knowing about DSL. Also lets us do chaining.
For example:

attributeWhenReady("targetSensor").attributeWhenReady("mySensor")
attributeWhenReady("anyBrooklynObject").config("myConfig")

Builds on #482

Introduces DslAccessible annotation used to white-list methods which are allowed to be called by DSL, replacing the current approach of expecting them to be under the org.apache.brooklyn.camp.brooklyn.spi.dsl.methods package.

neykov · 2016-12-12T10:21:33Z

camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/DslCallable.java

+ * Marker interface so the evaluator can tell apart objects which are {@link DeferredSupplier}
+ * but which expect DSL methods called on them instead of the value they supply.
+ */
+public interface DslCallable {


I'm considering changing this to an annotation. When applied on:

A class will work as now - let the DSL evaluator know to use the object as is, stop evaluation

On a method - white-list the method for DSL calls. Deprecate non-annotated method calls. This will make it very easy to inspect what is executable by the DSL.

I like this idea. Do you want to make it part of this PR or do it subsequently?

This one. Don't want to introduce APIs that will go away in master.

After spiking the changes decided to keep DslCallable interface for first case and introduce a DslAccessible annotation for second. They have different use-cases, better not to mix them in the same annotation.

Will send an email to dev@brooklyn.apache.org discussing the DslAccessible change and implement it in a separate PR.

…RegexReplacer

Lets us call methods on DslCallable methods and at the same time resolve the object if it's the last of the chain.

Fixes tests which expect the DSL evaluated objects to be in the correct package. Adds tests to verify that objects outside of the package scope are inaccessible to DSL.

Gives more control over what method gets executed.

ahgittin · 2016-12-13T09:38:21Z

...rooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/DslDeferredFunctionCall.java

+        if (object instanceof DslCallable || object == null) {
+            return Maybe.of(object);
+        }
+        Maybe<?> resultMaybe = Tasks.resolving(object, Object.class)


How is this code reachable? This class is only constructed when object is DslCallable in which case the above branch applies, isn't it?

nvm - missed the !

ahgittin · 2016-12-13T09:53:22Z

makes sense although something niggles...

quick things:

definitely @DslAccessible is clearer for methods
maybe DslFunctionSource is a better name than DslCallable ... though feels like this interface shouldn't be needed; we simply skip evaluation in DslDeferredFunctionCall if object is already a valid source for invoking fn eg object.getClass().getMethod(fn) exists and is @DslAccessible (or maybe we always evaluate, given my re-reading of code, DDFC is only created when object is not a DslCallable ... or in an annotation world it would only be created when object.fn() is @DslAccessible)

my gut says a cleaner solution would be methods of the form attributeWhenReady(Entity source, String sensorName) and we do "C++ on C" style caller injection into the first argument. this would solve the issue you mention where e.g. attributeWhenReady("target.cluster.entity").attributeWhenReady("size") isn't supported. it's a bigger change perhaps than wanted at this point so happy to defer but let's keep that in mind and any interfaces or annotations we introduce along the way probably mark beta.

Allows us to call custom DSL methods on any object we are interested in without the object knowing about DSL.

neykov · 2016-12-13T10:19:16Z

Good idea about stopping once the function is available, will think about it.
The DDFC chaining is started on the first non-DslCallable DeferredSupplier. But said supplier could return a DslCallable after an unspecified number of resolve iterations. And it's known only at resolve time. Once you start chaining you can't stop as all subsequent calls depend on the return value of the previous which is already deferred.

Re C++ on C see #486. Still think its orthogonal to DDFC.

neykov · 2016-12-13T11:39:48Z

Did the removal of DslCallable locally. Works fine, but turns out we are losing one important feature of the current approach - if you misspell a method on DslComponent you won't find until executing the blueprint. Now it will complain when you try to deploy (at least until we switch to DDFC). The reason is that if it doesn't find the method on the DslComponent it will defer evaluation to get the underlying Entity.
Wdyt @ahgittin, is it worth keeping DslCallable/DslFunctionSource to get the (now limited) early warning?

ahgittin · 2016-12-13T11:56:36Z

nice. and good spot re errors. probably is worth keeping, not just for errors, but also so that we have some hope of being able to propose autocompletions / populate graphical editor. fine by me either way.

geomacy · 2016-12-13T12:16:15Z

camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/DslYamlTest.java

+        assertEquals(getConfigEventually(app, DEST), "myvalue");
+    }
+
+    @Test(groups="WIP") // config accepts strings only, no suppliers


Cut-n-paste - think this comment should be attributeWhenReady accepts strings only, no suppliers, also on next method

geomacy · 2016-12-13T12:36:53Z

camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/DslYamlTest.java

+        Task<T> result = ((EntityInternal)entity).getExecutionContext().submit(new Callable<T>() {
+            @Override
+            public T call() throws Exception {
+                // TODO Move the getNonBlocking call out of the task after #280 is merged.


Do you mean #480?

Addresses review comments.

Applying the DslAccessible annotation on any method allows it to be called by DSL

neykov · 2016-12-13T15:34:12Z

Decided to stick with DslFunctionSource. Addressed all other comments. Merged with #486.

geomacy · 2016-12-13T15:48:51Z

...rooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/DslDeferredFunctionCall.java

+                    clazz.getPackage().getName().startsWith(whiteListPackage.getName()));
+        if (isPackageAllowed) {
+            if (DEPRECATED_ACCESS_WARNINGS.add(m)) {
+                log.warn("Deprecated since 0.11.0. The method '" + m.toString() + "' called by DSL should be white listed using the " + DslAccessible.class.getSimpleName() + " annotation. Support for DSL callable methods under the " + whiteListPackage + " will be fremoved in a future release.");


typo "fremoved". Would be nice to break this line up over several (currently 297 characters wide!)

geomacy · 2016-12-13T15:52:11Z

camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/DslAccessible.java

+
+@Retention(RUNTIME)
+@Target(METHOD)
+public @interface DslAccessible {


It would be nice to add some javadoc around this to explain what it's for.

geomacy · 2016-12-13T17:03:41Z

...p-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/methods/DslComponent.java

        }
    }

+    @DslAccessible


How is it that attributeWhenReady() and config() are able to be @DslAccessible when a few commits ago they did not return a value that implemented DslCallable?

(i.e. I thought the difference was that e.g. attributeWhenReady() and config() couldn't be invoked within BrooklynDslInterpreter.evaluateOn() using DslDeferredFunctionCall.invoke() and so for them a DslDeferredFunctionCall object gets created, whereas general DslComponents get evaluated with an immediate DslDeferredFunctionCall.invoke(). Now, however, both general DslComponents like parent() and also e.g. attributeWhenReady()/config() are marked as @DslAccessible.

(does the comment on the PR header, "DslComponent is a special case where it will not be resolved before evaluating the DSL as that's the expected behaviour.", still apply?)

ahgittin · 2016-12-14T13:42:59Z

LGTM, once @geomacy 's comments addressed

geomacy · 2016-12-20T11:04:50Z

Actually I think my comments above are not worth holding this up for, and I answered my question above myself with a bit of further inspection and running tests, so happy to merge. LGTM.

neykov changed the title ~~Dsl deferred execution~~ DSL deferred execution Dec 12, 2016

neykov commented Dec 12, 2016

View reviewed changes

neykov changed the title ~~DSL deferred execution~~ [WIP] DSL deferred execution Dec 13, 2016

neykov force-pushed the dsl-deferred-execution branch from a0f3749 to e634760 Compare December 13, 2016 07:04

neykov changed the title ~~[WIP] DSL deferred execution~~ DSL deferred execution Dec 13, 2016

neykov added 7 commits December 13, 2016 09:15

Basic coverage of DSL YAML parsing and evaluation

2e0ee18

Deferred DSL evaluation, allowing for chaining

d71141e

Deprecate BDC.Functions.RegexReplacer - duplicate of StringFunctions.…

498a58c

…RegexReplacer

Tighten DSL function origin check, apply on deferred

ddfe733

Non-recursive ValueResolver

42bc7c1

Stop recursive resolving of DSL results on DslCallable

e831d75

Lets us call methods on DslCallable methods and at the same time resolve the object if it's the last of the chain.

Move test objects in the allowed package scope

6607ccb

Fixes tests which expect the DSL evaluated objects to be in the correct package. Adds tests to verify that objects outside of the package scope are inaccessible to DSL.

neykov force-pushed the dsl-deferred-execution branch from e634760 to 6607ccb Compare December 13, 2016 07:30

Split Reflections.invokeMethodArgs into Method find and Method invoke

eb4ac4c

Gives more control over what method gets executed.

ahgittin reviewed Dec 13, 2016

View reviewed changes

Provide mixins/facade DSL methods

b75b45c

Allows us to call custom DSL methods on any object we are interested in without the object knowing about DSL.

neykov mentioned this pull request Dec 13, 2016

DSL mixins/facade wrappers #486

Closed

geomacy reviewed Dec 13, 2016

View reviewed changes

neykov added 2 commits December 13, 2016 16:42

Rename DslCallable to DslFunctionSource and some cleanup

60c5c6f

Addresses review comments.

DslAccessible replaces hard-coded package DSL white-listing

f564aa8

Applying the DslAccessible annotation on any method allows it to be called by DSL

geomacy reviewed Dec 13, 2016

View reviewed changes

asfgit merged commit f564aa8 into apache:master Dec 20, 2016

asfgit pushed a commit that referenced this pull request Dec 20, 2016

This closes #482 DSL deferred execution

58b1de7

neykov deleted the dsl-deferred-execution branch January 4, 2017 14:19

DSL deferred execution #482

DSL deferred execution #482

Uh oh!

Conversation

neykov commented Dec 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahgittin commented Dec 13, 2016

Uh oh!

neykov commented Dec 13, 2016

Uh oh!

neykov commented Dec 13, 2016

Uh oh!

ahgittin commented Dec 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neykov commented Dec 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahgittin commented Dec 14, 2016

Uh oh!

geomacy commented Dec 20, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

neykov commented Dec 8, 2016 •

edited

Loading

neykov commented Dec 13, 2016 •

edited

Loading