You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by sjcorbett <gi...@git.apache.org> on 2015/07/16 16:57:54 UTC
[GitHub] incubator-brooklyn pull request: Add EntityDynamicType.removeEffec...
GitHub user sjcorbett opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/749
Add EntityDynamicType.removeEffector(Effector)
Not sure if this is controversial.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sjcorbett/incubator-brooklyn feature/remove-effector
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/749.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 #749
----
commit 98241a8c5dccc3ccc8b2410f2dd5cc1fea809bc8
Author: Sam Corbett <sa...@cloudsoftcorp.com>
Date: 2015-07-15T17:03:27Z
Add EntityDynamicType.removeEffector(Effector)
----
---
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] incubator-brooklyn pull request: Add EntityDynamicType.removeEffec...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/749#issuecomment-122852244
@alasdairhodge Thanks for comments. Would you cast your eye over a second time? Note the comment on `removeEffector` and the assertions of the new test. Brooklyn will still invoke removed effectors because it knows better than you and gets Groovy to look up the method dynamically.
---
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] incubator-brooklyn pull request: Add EntityDynamicType.removeEffec...
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/749#issuecomment-124008233
Interesting example, @svet. Just out of curiosity: this specific usage feels like a band-aid for lacking the right place to define a (static) `executeCommand` effector. Would you dynamically add the mixin effector if an explicit `SshMachine` entity was present in the mgmt tree? (Not that I'm wanting to reopen the "locations as entities" discussion...)
---
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] incubator-brooklyn pull request: Add EntityDynamicType.removeEffec...
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/749#issuecomment-122257710
Awesome. Not controversial, IMO; the symmetry makes perfect sense.
---
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] incubator-brooklyn pull request: Add EntityDynamicType.removeEffec...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/749
---
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] incubator-brooklyn pull request: Add EntityDynamicType.removeEffec...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/749#issuecomment-124024954
Removing effectors could still be useful for UI reasons.
---
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] incubator-brooklyn pull request: Add EntityDynamicType.removeEffec...
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/749#issuecomment-124021792
@neykov: Hmm, interesting and powerful. The (undeclared) "cross-cutting concern" effectors won't be shown in the UI, right? And can't be invoked via REST?
---
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] incubator-brooklyn pull request: Add EntityDynamicType.removeEffec...
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/749#issuecomment-122259733
Controversial lack of a unit test, however.
---
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] incubator-brooklyn pull request: Add EntityDynamicType.removeEffec...
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/749#issuecomment-123787647
Unfortunate behaviour in `entity.invoke()`; I'd call that a bug, and suggest a fix in the form of an explicit check in `Effectors.invocation()`, which already throws `UnsupportedOperationException` for missing effectors.
---
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] incubator-brooklyn pull request: Add EntityDynamicType.removeEffec...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/749#issuecomment-123797110
Is it a bug? We can remove the effector but for example `Startable` entities are still going to have a `stop` method the can be invoked programatically. Brooklyn turns those method calls into effector invocations. Should we not do this if the related effector has been removed?
I'm wary of modifying `Effectors.invocation` (see https://github.com/apache/incubator-brooklyn/blob/master/core/src/main/java/brooklyn/entity/effector/Effectors.java#L153-159) without looking more into what is relying on this at the moment.
---
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] incubator-brooklyn pull request: Add EntityDynamicType.removeEffec...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/749#issuecomment-124024471
> The (undeclared) "cross-cutting concern" effectors won't be shown in the UI, right? And can't be invoked via REST?
That's right. We could add functionality to invoke undeclared effectors to the REST API but it's not supported now.
On the other hand the effector task will appear in the entity's Activity tab for inspection.
---
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] incubator-brooklyn pull request: Add EntityDynamicType.removeEffec...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/749#issuecomment-124334627
Thanks all - merging now.
---
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] incubator-brooklyn pull request: Add EntityDynamicType.removeEffec...
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/749#issuecomment-124022543
@sjcorbett: this thread has been a real eye-opener. I'm coming round to the elegance of "unattached" effectors for cross-cutting concerns, but this does call into question the notion of removing effectors, which is what this PR is all about.
In our specific case, can we instead prevent the unwanted effector being added in the first place?
---
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] incubator-brooklyn pull request: Add EntityDynamicType.removeEffec...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/749#issuecomment-123816219
I don't see it as a bug. Effectors are like enrichers - the entity might not be aware of it at all. The entity could override an effector body by adding it to the `entityType`, but otherwise the caller's implementation will be used.
Take for example an `SshEffector` which executes a shell command on the entity's machine. It can be executed on any entity which uses `SshMachineLocation`, but it's not feasible to add it to all such entities.
---
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] incubator-brooklyn pull request: Add EntityDynamicType.removeEffec...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/749#issuecomment-124020763
Well I wouldn't add it dynamically at all, just invoke it on the entity (not sure if I got your question right).
There are other examples of cross-cutting concerns which would work on a large number of entities, but none would add them explicitly to the "favoured" list. Like an effector which reports for potential rebind problems on running entities.
---
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] incubator-brooklyn pull request: Add EntityDynamicType.removeEffec...
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/749#discussion_r35231442
--- Diff: core/src/main/java/brooklyn/entity/basic/EntityDynamicType.java ---
@@ -130,17 +130,38 @@ public void addEffector(Effector<?> newEffector) {
instance.emit(AbstractEntity.EFFECTOR_ADDED, newEffector.getName());
}
- /** Adds an effector with an explicit body */
+ /**
+ * Adds an effector with an explicit body to this entity.
+ */
@Beta
public <T> void addEffector(Effector<T> effector, EffectorTaskFactory<T> body) {
addEffector(new EffectorAndBody<T>(effector, body));
}
- /** Adds an effector with an explicit body */
+
+ /**
+ * Adds an effector with an explicit body to this entity.
+ */
@Beta
public <T> void addEffector(Effector<T> effector, EffectorBody<T> body) {
addEffector(effector, new EffectorBodyTaskFactory<T>(body));
}
+ /**
+ * Removes the given {@link Effector} from this entity.
+ * <p>
+ * Note that if the argument is an instance of {@link EffectorWithBody} it will
+ * still be possible to invoke the effector on the entity by calling
+ * <code>entity.invoke(effector, argumentsMap)</code>.
+ */
--- End diff --
The ability to invoke removed efectors is a bug, this comment should reflect that. Is it trivial to fix `entity.invoke()`?
---
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] incubator-brooklyn pull request: Add EntityDynamicType.removeEffec...
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/749#issuecomment-124005744
Good points, @sjcorbett and @neykov, thanks. I agree that "fixing" `Effectors` is too risky to consider here. I would still consider it a bug if effectors that have been *explicitly removed in code* can still be invoked.
---
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.
---