You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by mikezaccardo <gi...@git.apache.org> on 2015/11/20 07:31:29 UTC
[GitHub] incubator-brooklyn pull request: Add all.members.up sensor to Dyna...
GitHub user mikezaccardo opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/1050
Add all.members.up sensor to DynamicCluster
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mikezaccardo/incubator-brooklyn all-members-up-sensor
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/1050.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 #1050
----
commit 5f6a621801638424a899e015156713b7375f8d6e
Author: Mike Zaccardo <mi...@cloudsoftcorp.com>
Date: 2015-11-20T06:31:06Z
Add all.members.up sensor to DynamicCluster
----
---
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 all.members.up sensor to Dyna...
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1050#issuecomment-158578735
Thanks for reviewing, @sjcorbett. You're definitely right about the `null` case in my `FALSE` comparison -- I've switched to checking `!TRUE`.
As for your other comments, I think I need to elaborate on my intended use for this sensor; perhaps it is not properly named. I want to be able to use this as a launch latch / attribute when ready sensor.
If the emptiness check is removed then this sensor will resolve to true immediately when the cluster is created (before the children are added) which is too early. I could remove this check but then I'd need to leave in the check for the running lifecycle state -- that way even though the cluster is empty in the beginning, this sensor would not resolve to true because it is not yet running at that point.
tl;dr -- I need one or both of the checks that you flagged to remain in order for the sensor to behave the way I'd like it to. WDYT?
---
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 all.members.up sensor to Dyna...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/1050
---
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 all.members.up sensor to Dyna...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1050#issuecomment-158955208
I think you're right that this can just come down to nomenclature. ClusterAndMembersUpCallable? Though that doesn't indicate that it expects at least one member.
To be honest I think it would be clearer to keep the three checks separate (cluster up, members up, members.size > 0) and get your launch latch by combining them with a reducer that ANDs them all together. (Not that I know how to express this in YAML, of course!)
---
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 all.members.up sensor to Dyna...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1050#issuecomment-163553359
changing sensor name to `CLUSTER_ONE_AND_ALL_MEMBERS_UP` `cluster.one_and_all.members.up` for clarity, and merging
---
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 all.members.up sensor to Dyna...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1050#discussion_r45455633
--- Diff: core/src/main/java/org/apache/brooklyn/entity/group/DynamicClusterImpl.java ---
@@ -181,6 +189,34 @@ private void initialiseMemberId() {
}
}
+ private void connectAllMembersUp() {
+ allMembersUp = FunctionFeed.builder()
+ .entity(this)
+ .period(Duration.FIVE_SECONDS)
+ .poll(new FunctionPollConfig<Boolean, Boolean>(ALL_MEMBERS_UP)
+ .onException(Functions.constant(Boolean.FALSE))
+ .callable(new AllMembersUpCallable()))
+ .build();
+ }
+
+ private class AllMembersUpCallable implements Callable<Boolean> {
+
+ @Override
+ public Boolean call() throws Exception {
+ if (DynamicClusterImpl.this.getMembers().isEmpty())
+ return false;
+
+ if (Lifecycle.RUNNING != DynamicClusterImpl.this.sensors().get(SERVICE_STATE_ACTUAL))
+ return false;
+
+ for (Entity member : DynamicClusterImpl.this.getMembers())
+ if (Boolean.FALSE.equals(member.sensors().get(SERVICE_UP)))
--- End diff --
The callable is named `AllMembersUp` but this is checking that all members are not not up. It will return true if a member's `SERVICE_UP` is null. I think you should swap the if condition for `if (!Boolean.TRUE.equals(...))`.
---
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 all.members.up sensor to Dyna...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1050#issuecomment-162571760
@mikezaccardo What do you want to do with this now that #1037 is merged?
---
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 all.members.up sensor to Dyna...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1050#discussion_r45455255
--- Diff: core/src/main/java/org/apache/brooklyn/entity/group/DynamicClusterImpl.java ---
@@ -181,6 +189,34 @@ private void initialiseMemberId() {
}
}
+ private void connectAllMembersUp() {
+ allMembersUp = FunctionFeed.builder()
+ .entity(this)
+ .period(Duration.FIVE_SECONDS)
+ .poll(new FunctionPollConfig<Boolean, Boolean>(ALL_MEMBERS_UP)
+ .onException(Functions.constant(Boolean.FALSE))
+ .callable(new AllMembersUpCallable()))
+ .build();
+ }
+
+ private class AllMembersUpCallable implements Callable<Boolean> {
+
+ @Override
+ public Boolean call() throws Exception {
+ if (DynamicClusterImpl.this.getMembers().isEmpty())
+ return false;
--- End diff --
Wouldn't no members mean that all the members are up?
---
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 all.members.up sensor to Dyna...
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1050#issuecomment-162957168
@sjcorbett I still think this addition has value because it simplifies YAML blueprints (vs. having to add an aggregator from #1037), and it also better handles changes in cluster size when invoking a positive resize effector on a `DynamicCluster`. I'd like to see it merged -- maybe a name change could assuage the concerns you raised about an empty cluster having a `false` value?
---
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 all.members.up sensor to Dyna...
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1050#issuecomment-158986371
@sjcorbett I believe we could actually achieve the same functionality in YAML using this feature: https://github.com/apache/incubator-brooklyn/pull/1037
---
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 all.members.up sensor to Dyna...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1050#discussion_r45455745
--- Diff: core/src/main/java/org/apache/brooklyn/entity/group/DynamicClusterImpl.java ---
@@ -181,6 +189,34 @@ private void initialiseMemberId() {
}
}
+ private void connectAllMembersUp() {
+ allMembersUp = FunctionFeed.builder()
+ .entity(this)
+ .period(Duration.FIVE_SECONDS)
+ .poll(new FunctionPollConfig<Boolean, Boolean>(ALL_MEMBERS_UP)
+ .onException(Functions.constant(Boolean.FALSE))
+ .callable(new AllMembersUpCallable()))
+ .build();
+ }
+
+ private class AllMembersUpCallable implements Callable<Boolean> {
+
+ @Override
+ public Boolean call() throws Exception {
+ if (DynamicClusterImpl.this.getMembers().isEmpty())
+ return false;
+
+ if (Lifecycle.RUNNING != DynamicClusterImpl.this.sensors().get(SERVICE_STATE_ACTUAL))
--- End diff --
Why take the state of the cluster into account when determining whether members are up?
---
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.
---