You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by ahgittin <gi...@git.apache.org> on 2017/01/16 20:37:22 UTC
[GitHub] brooklyn-server pull request #521: WIP - AddChildrenEffector has some unexpe...
GitHub user ahgittin opened a pull request:
https://github.com/apache/brooklyn-server/pull/521
WIP - AddChildrenEffector has some unexpected behaviour wrt DSL and params
so far just adds tests indicating the ugly behaviour;
next will:
* improve impl to avoid the ugly behaviour
* facilitate evaluation of DSL in configuration parameters
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ahgittin/brooklyn-server tidy-add-children-effector
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/521.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #521
----
commit c5806879dfed042a45567699144fc452f899d10d
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2017-01-16T20:34:44Z
tests indicating current behaviour, including things we don't like
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #521: fix unexpected AddChildrenEffector behavi...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/521#discussion_r100310271
--- Diff: camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/DslDeferredFunctionCall.java ---
@@ -80,14 +80,14 @@ public Object call() throws Exception {
Object instance = resolvedMaybe.get();
if (instance == null) {
- throw new IllegalArgumentException("Deferred function call, " + object +
- " evaluates to null (when calling " + fnName + "(" + toString(args) + "))");
+ throw new IllegalArgumentException("Deferred function call not found: " +
+ object + " evaluates to null (wanting to call " + fnName + args + "))");
--- End diff --
Miss a `(` before the `args`:
```java
object + " evaluates to null (wanting to call " + fnName + "(" + args + "))");
```
Unless you can you your `DslToStringHelpers.fn()` ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #521: fix unexpected AddChildrenEffector behaviour and...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on the issue:
https://github.com/apache/brooklyn-server/pull/521
@aledsage Did you try the blueprint I posted above? I couldn't make it work, I'm guessing I missed something obvious
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #521: fix unexpected AddChildrenEffector behavi...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/521#discussion_r100361187
--- Diff: camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/DslDeferredFunctionCall.java ---
@@ -99,48 +99,81 @@ public Object call() throws Exception {
}
protected static Maybe<Object> invokeOn(Object obj, String fnName, List<?> args) {
- Object instance = obj;
- List<?> instanceArgs = args;
- Maybe<Method> method = Reflections.getMethodFromArgs(instance, fnName, instanceArgs);
-
- if (method.isAbsent()) {
+ return new Invoker(obj, fnName, args).invoke();
+ }
+
+ protected static class Invoker {
+ final Object obj;
+ final String fnName;
+ final List<?> args;
+
+ Maybe<Method> method;
+ Object instance;
+ List<?> instanceArgs;
+
+ protected Invoker(Object obj, String fnName, List<?> args) {
+ this.fnName = fnName;
+ this.obj = obj;
+ this.args = args;
+ }
+
+ protected Maybe<Object> invoke() {
+ findMethod();
+
+ if (method.isPresent()) {
+ Method m = method.get();
+
+ checkCallAllowed(m);
+
+ try {
+ // Value is most likely another BrooklynDslDeferredSupplier - let the caller handle it,
+ return Maybe.of(Reflections.invokeMethodFromArgs(instance, m, instanceArgs));
+ } catch (IllegalArgumentException | IllegalAccessException | InvocationTargetException e) {
+ // If the method is there but not executable for whatever reason fail with a fatal error, don't return an absent.
+ throw Exceptions.propagate(new InvocationTargetException(e, "Error invoking '"+fnName+instanceArgs+"' on '"+instance+"'"));
--- End diff --
Minor: I don't think this should be an `InvocationTargetException` - `e` was not "thrown by an invoked method or constructor". If `e` is an IllegalArgumentException or IllegalAccessException, then it was caused by trying to invoke it; if `e` is an `InvocationTargetException` then it is already wrapped. We probably just want `Exceptions.propagateAnnotated("...", e)`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #521: fix unexpected AddChildrenEffector behaviour and...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/521
@tbouron Yes ... for once it gives a good error message: `requires either blueprint_type or blueprint_yaml`. You called it `brooklyn_yaml`!
@aledsage Good comment re `InvocationTargetException`, adjusted that
also fixed the parentheses (with local static methods).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #521: fix unexpected AddChildrenEffector behavi...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/521#discussion_r100310679
--- Diff: camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/DslDeferredFunctionCall.java ---
@@ -99,48 +99,81 @@ public Object call() throws Exception {
}
protected static Maybe<Object> invokeOn(Object obj, String fnName, List<?> args) {
- Object instance = obj;
- List<?> instanceArgs = args;
- Maybe<Method> method = Reflections.getMethodFromArgs(instance, fnName, instanceArgs);
-
- if (method.isAbsent()) {
+ return new Invoker(obj, fnName, args).invoke();
+ }
+
+ protected static class Invoker {
+ final Object obj;
+ final String fnName;
+ final List<?> args;
+
+ Maybe<Method> method;
+ Object instance;
+ List<?> instanceArgs;
+
+ protected Invoker(Object obj, String fnName, List<?> args) {
+ this.fnName = fnName;
+ this.obj = obj;
+ this.args = args;
+ }
+
+ protected Maybe<Object> invoke() {
+ findMethod();
+
+ if (method.isPresent()) {
+ Method m = method.get();
+
+ checkCallAllowed(m);
+
+ try {
+ // Value is most likely another BrooklynDslDeferredSupplier - let the caller handle it,
+ return Maybe.of(Reflections.invokeMethodFromArgs(instance, m, instanceArgs));
+ } catch (IllegalArgumentException | IllegalAccessException | InvocationTargetException e) {
+ // If the method is there but not executable for whatever reason fail with a fatal error, don't return an absent.
+ throw Exceptions.propagate(new InvocationTargetException(e, "Error invoking '"+fnName+instanceArgs+"' on '"+instance+"'"));
--- End diff --
Miss `(` and `)` around `instanceArgs`
Unless you can you your `DslToStringHelpers.fn()` ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #521: fix unexpected AddChildrenEffector behavi...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/521#discussion_r100394340
--- Diff: camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/DslDeferredFunctionCall.java ---
@@ -62,7 +62,7 @@ public DslDeferredFunctionCall(Object o, String fn, List<Object> args) {
@Override
public Task<Object> newTask() {
return Tasks.builder()
- .displayName("Deferred function call " + object + "." + fnName + "(" + toString(args) + ")")
+ .displayName("Deferred function call " + object + "." + fnName + args + ")")
--- End diff --
Is `args` guaranteed to be non-null here? You'd get `fnNamenull` if not.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #521: fix unexpected AddChildrenEffector behavi...
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/521#discussion_r100310194
--- Diff: camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/DslDeferredFunctionCall.java ---
@@ -62,7 +62,7 @@ public DslDeferredFunctionCall(Object o, String fn, List<Object> args) {
@Override
public Task<Object> newTask() {
return Tasks.builder()
- .displayName("Deferred function call " + object + "." + fnName + "(" + toString(args) + ")")
+ .displayName("Deferred function call " + object + "." + fnName + args + ")")
--- End diff --
Miss a `(` before the `args`:
```java
.displayName("Deferred function call " + object + "." + fnName + "(" + args + ")")
```
Unless you can you your `DslToStringHelpers.fn()` ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #521: fix unexpected AddChildrenEffector behaviour and...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/521
*ready for review*
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #521: fix unexpected AddChildrenEffector behavi...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/521
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #521: fix unexpected AddChildrenEffector behaviour and...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/521
in the first commit, noticed that some of the DSL `toString()` methods returned non-re-parseable output. i've tidied most of the `DslComponent.toString()` impls to be consistent, and cleaner.
also added nice newer DSL tests, and support for `entity("ID")` to look at internal ID's
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---