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/07/31 01:34:46 UTC
[GitHub] incubator-brooklyn pull request: Add basic sensors to RedisCluster
GitHub user mikezaccardo opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/782
Add basic sensors to RedisCluster
RedisCluster did not expose any sensors, so this adds basic information about the master node like host address and Redis port.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mikezaccardo/incubator-brooklyn redis-cluster-sensors
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/782.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 #782
----
commit d5936fcb050442854ccc27717d9a4562c9274aae
Author: Mike Zaccardo <mi...@cloudsoftcorp.com>
Date: 2015-07-30T23:28:20Z
Add basic sensors to RedisCluster
----
---
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 basic sensors to RedisCluster
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/782#issuecomment-126795682
@mikezaccardo when starting for a second time use `None` location, meaning that it should use the existing one.
---
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 basic sensors to RedisCluster
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/782#issuecomment-126797249
Looks good once `getUnchecked` is used.
---
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 basic sensors to RedisCluster
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/782#discussion_r36251169
--- Diff: software/nosql/src/main/java/brooklyn/entity/nosql/redis/RedisClusterImpl.java ---
@@ -20,64 +20,111 @@
import java.util.Collection;
-import brooklyn.entity.Entity;
+import brooklyn.enricher.Enrichers;
import brooklyn.entity.basic.AbstractEntity;
-import brooklyn.entity.basic.Entities;
+import brooklyn.entity.basic.Lifecycle;
+import brooklyn.entity.basic.ServiceStateLogic;
+import brooklyn.entity.basic.ServiceStateLogic.ComputeServiceIndicatorsFromChildrenAndMembers;
+import brooklyn.entity.basic.ServiceStateLogic.ServiceProblemsLogic;
import brooklyn.entity.group.DynamicCluster;
import brooklyn.entity.proxying.EntitySpec;
-import brooklyn.entity.trait.Startable;
+import brooklyn.event.AttributeSensor;
+import brooklyn.event.basic.Sensors;
import brooklyn.location.Location;
+import brooklyn.util.collections.QuorumCheck.QuorumChecks;
+import brooklyn.util.exceptions.Exceptions;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
public class RedisClusterImpl extends AbstractEntity implements RedisCluster {
- protected RedisStore master;
- protected DynamicCluster slaves;
+ private static AttributeSensor<RedisStore> MASTER = Sensors.newSensor(RedisStore.class, "redis.master");
+ private static AttributeSensor<DynamicCluster> SLAVES = Sensors.newSensor(DynamicCluster.class, "redis.slaves");
public RedisClusterImpl() {
}
@Override
public RedisStore getMaster() {
- return master;
+ return getAttribute(MASTER);
}
@Override
public DynamicCluster getSlaves() {
- return slaves;
+ return getAttribute(SLAVES);
+ }
+
+ @Override
+ public void init() {
+ super.init();
+
+ RedisStore master = addChild(EntitySpec.create(RedisStore.class));
+ setAttribute(MASTER, master);
+
+ DynamicCluster slaves = addChild(EntitySpec.create(DynamicCluster.class)
+ .configure(DynamicCluster.MEMBER_SPEC, EntitySpec.create(RedisSlave.class).configure(RedisSlave.MASTER, master)));
+ setAttribute(SLAVES, slaves);
+
+ addEnricher(Enrichers.builder()
+ .propagating(RedisStore.HOSTNAME, RedisStore.ADDRESS, RedisStore.SUBNET_HOSTNAME, RedisStore.SUBNET_ADDRESS, RedisStore.REDIS_PORT)
+ .from(master)
+ .build());
+ }
+
+ @Override
+ protected void initEnrichers() {
+ super.initEnrichers();
+ ServiceStateLogic.newEnricherFromChildrenUp().
+ checkChildrenOnly().
+ requireUpChildren(QuorumChecks.all()).
+ configure(ComputeServiceIndicatorsFromChildrenAndMembers.IGNORE_ENTITIES_WITH_THESE_SERVICE_STATES, ImmutableSet.<Lifecycle>of()).
+ addTo(this);
}
@Override
public void start(Collection<? extends Location> locations) {
- master = addChild(EntitySpec.create(RedisStore.class));
- Entities.manage(master);
- master.start(locations);
+ ServiceStateLogic.setExpectedState(this, Lifecycle.STARTING);
+ ServiceProblemsLogic.clearProblemsIndicator(this, START);
+ try {
+ doStart(locations);
+ ServiceStateLogic.setExpectedState(this, Lifecycle.RUNNING);
+ } catch (Exception e) {
+ ServiceProblemsLogic.updateProblemsIndicator(this, START, "Start failed with error: "+e);
+ ServiceStateLogic.setExpectedState(this, Lifecycle.ON_FIRE);
+ throw Exceptions.propagate(e);
+ }
+ }
- slaves = addChild(EntitySpec.create(DynamicCluster.class)
- .configure(DynamicCluster.MEMBER_SPEC, EntitySpec.create(RedisSlave.class).configure(RedisSlave.MASTER, master)));
- slaves.start(locations);
+ private void doStart(Collection<? extends Location> locations) {
+ RedisStore master = getMaster();
+ master.invoke(RedisStore.START, ImmutableMap.<String, Object>of("locations", ImmutableList.copyOf(locations))).getUnchecked();
- setAttribute(Startable.SERVICE_UP, calculateServiceUp());
+ DynamicCluster slaves = getSlaves();
+ slaves.invoke(DynamicCluster.START, ImmutableMap.<String, Object>of("locations", ImmutableList.copyOf(locations))).getUnchecked();
}
@Override
public void stop() {
- if (slaves != null) slaves.stop();
- if (master != null) master.stop();
+ ServiceStateLogic.setExpectedState(this, Lifecycle.STOPPING);
+ try {
+ doStop();
+ ServiceStateLogic.setExpectedState(this, Lifecycle.STOPPED);
+ } catch (Exception e) {
+ ServiceProblemsLogic.updateProblemsIndicator(this, STOP, "Stop failed with error: "+e);
+ ServiceStateLogic.setExpectedState(this, Lifecycle.ON_FIRE);
+ throw Exceptions.propagate(e);
+ }
+ }
- setAttribute(Startable.SERVICE_UP, false);
+ private void doStop() {
+ getSlaves().invoke(DynamicCluster.STOP, ImmutableMap.<String, Object>of()).getUnchecked();
--- End diff --
This is fine for the PR, but I suspect if there is an error stopping the slaves then the master won't be stopped either. Not sure off-hand what best approach is for that.
---
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 basic sensors to RedisCluster
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/782#discussion_r36105600
--- Diff: software/nosql/src/main/java/brooklyn/entity/nosql/redis/RedisClusterImpl.java ---
@@ -72,12 +129,12 @@ public void restart() {
throw new UnsupportedOperationException();
}
- protected boolean calculateServiceUp() {
- boolean up = false;
- for (Entity member : slaves.getMembers()) {
- if (Boolean.TRUE.equals(member.getAttribute(SERVICE_UP))) up = true;
- }
- return up;
- }
+ private boolean calculateServiceUp() {
--- End diff --
Is this the appropriate method for calculating `SERVICE_UP`?
Before it would return `true` if one or more slave nodes were up, with no consideration of master.
Now it requires master and all slaves to be up. Perhaps that is too strict, so perhaps the correct way is somewhere in the middle?
---
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 basic sensors to RedisCluster
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/782#issuecomment-127781651
Looks good; 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 basic sensors to RedisCluster
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/782#discussion_r36250755
--- Diff: software/nosql/src/main/java/brooklyn/entity/nosql/redis/RedisClusterImpl.java ---
@@ -20,64 +20,111 @@
import java.util.Collection;
-import brooklyn.entity.Entity;
+import brooklyn.enricher.Enrichers;
import brooklyn.entity.basic.AbstractEntity;
-import brooklyn.entity.basic.Entities;
+import brooklyn.entity.basic.Lifecycle;
+import brooklyn.entity.basic.ServiceStateLogic;
+import brooklyn.entity.basic.ServiceStateLogic.ComputeServiceIndicatorsFromChildrenAndMembers;
+import brooklyn.entity.basic.ServiceStateLogic.ServiceProblemsLogic;
import brooklyn.entity.group.DynamicCluster;
import brooklyn.entity.proxying.EntitySpec;
-import brooklyn.entity.trait.Startable;
+import brooklyn.event.AttributeSensor;
+import brooklyn.event.basic.Sensors;
import brooklyn.location.Location;
+import brooklyn.util.collections.QuorumCheck.QuorumChecks;
+import brooklyn.util.exceptions.Exceptions;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
public class RedisClusterImpl extends AbstractEntity implements RedisCluster {
- protected RedisStore master;
- protected DynamicCluster slaves;
+ private static AttributeSensor<RedisStore> MASTER = Sensors.newSensor(RedisStore.class, "redis.master");
--- End diff --
I'd declare it `final` as well, given it is a constant.
---
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 basic sensors to RedisCluster
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/782#discussion_r36026316
--- Diff: software/nosql/src/main/java/brooklyn/entity/nosql/redis/RedisClusterImpl.java ---
@@ -20,49 +20,73 @@
import java.util.Collection;
+import brooklyn.enricher.Enrichers;
import brooklyn.entity.Entity;
import brooklyn.entity.basic.AbstractEntity;
import brooklyn.entity.basic.Entities;
import brooklyn.entity.group.DynamicCluster;
import brooklyn.entity.proxying.EntitySpec;
import brooklyn.entity.trait.Startable;
+import brooklyn.event.AttributeSensor;
+import brooklyn.event.basic.Sensors;
import brooklyn.location.Location;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+
public class RedisClusterImpl extends AbstractEntity implements RedisCluster {
- protected RedisStore master;
- protected DynamicCluster slaves;
+ private AttributeSensor<RedisStore> MASTER = Sensors.newSensor(RedisStore.class, "redis.master");
--- End diff --
We generally declare the config + sensors as `public static final`, and usually on the interface unless it is definitely just internal.
---
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 basic sensors to RedisCluster
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/782#issuecomment-126816930
Addressed
---
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 basic sensors to RedisCluster
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/782#discussion_r35936345
--- Diff: software/nosql/src/main/java/brooklyn/entity/nosql/redis/RedisClusterImpl.java ---
@@ -23,9 +23,13 @@
import brooklyn.entity.Entity;
import brooklyn.entity.basic.AbstractEntity;
import brooklyn.entity.basic.Entities;
+import brooklyn.entity.basic.EntityInternal;
import brooklyn.entity.group.DynamicCluster;
import brooklyn.entity.proxying.EntitySpec;
import brooklyn.entity.trait.Startable;
+import brooklyn.event.AttributeSensor;
+import brooklyn.event.SensorEvent;
+import brooklyn.event.SensorEventListener;
import brooklyn.location.Location;
public class RedisClusterImpl extends AbstractEntity implements RedisCluster {
--- End diff --
Also, looking at the existing `RedisClusterImpl` code, the use of fields for `master` and `slaves` is going to cause problems on Brooklyn restart (or HA failover). The fields won't get reset. Those need to be stored as sensors, or the getter methods need to look up the values by searching the children. Caching it in a field is fine, obviously.
Note that HA matters a lot to the customer who is driving this, so we need to test that before we give it to him for his lab.
---
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 basic sensors to RedisCluster
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/782#discussion_r36010043
--- Diff: software/nosql/src/main/java/brooklyn/entity/nosql/redis/RedisClusterImpl.java ---
@@ -20,49 +20,73 @@
import java.util.Collection;
+import brooklyn.enricher.Enrichers;
import brooklyn.entity.Entity;
import brooklyn.entity.basic.AbstractEntity;
import brooklyn.entity.basic.Entities;
import brooklyn.entity.group.DynamicCluster;
import brooklyn.entity.proxying.EntitySpec;
import brooklyn.entity.trait.Startable;
+import brooklyn.event.AttributeSensor;
+import brooklyn.event.basic.Sensors;
import brooklyn.location.Location;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+
public class RedisClusterImpl extends AbstractEntity implements RedisCluster {
- protected RedisStore master;
- protected DynamicCluster slaves;
+ private AttributeSensor<RedisStore> MASTER = Sensors.newSensor(RedisStore.class, "redis.master");
+ private AttributeSensor<DynamicCluster> SLAVES = Sensors.newSensor(DynamicCluster.class, "redis.slaves");
public RedisClusterImpl() {
}
@Override
public RedisStore getMaster() {
- return master;
+ return getAttribute(MASTER);
}
@Override
public DynamicCluster getSlaves() {
- return slaves;
+ return getAttribute(SLAVES);
}
@Override
public void start(Collection<? extends Location> locations) {
- master = addChild(EntitySpec.create(RedisStore.class));
+ RedisStore master = getMaster();
+ if (master == null) {
+ master = addChild(EntitySpec.create(RedisStore.class));
+ setAttribute(MASTER, master);
+ }
+
Entities.manage(master);
- master.start(locations);
+ master.invoke(RedisStore.START, ImmutableMap.<String, Object>of("locations", ImmutableList.copyOf(locations))).blockUntilEnded();
- slaves = addChild(EntitySpec.create(DynamicCluster.class)
- .configure(DynamicCluster.MEMBER_SPEC, EntitySpec.create(RedisSlave.class).configure(RedisSlave.MASTER, master)));
- slaves.start(locations);
+ DynamicCluster slaves = getSlaves();
+ if (slaves == null) {
+ slaves = addChild(EntitySpec.create(DynamicCluster.class)
+ .configure(DynamicCluster.MEMBER_SPEC, EntitySpec.create(RedisSlave.class).configure(RedisSlave.MASTER, master)));
+ setAttribute(SLAVES, slaves);
+ }
+
+ slaves.invoke(DynamicCluster.START, ImmutableMap.<String, Object>of("locations", ImmutableList.copyOf(locations))).blockUntilEnded();
setAttribute(Startable.SERVICE_UP, calculateServiceUp());
+
+ addEnricher(Enrichers.builder()
+ .propagating(RedisStore.HOSTNAME, RedisStore.ADDRESS, RedisStore.SUBNET_HOSTNAME, RedisStore.SUBNET_ADDRESS, RedisStore.REDIS_PORT)
+ .from(master)
+ .build());
}
@Override
public void stop() {
- if (slaves != null) slaves.stop();
- if (master != null) master.stop();
+ DynamicCluster slaves = getSlaves();
+ RedisStore master = getMaster();
+
+ if (slaves != null) slaves.invoke(DynamicCluster.STOP, ImmutableMap.<String, Object>of()).blockUntilEnded();
--- End diff --
Same here, you could invoke both effectors and then check for the return 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 basic sensors to RedisCluster
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/782#discussion_r36026516
--- Diff: software/nosql/src/main/java/brooklyn/entity/nosql/redis/RedisClusterImpl.java ---
@@ -20,49 +20,73 @@
import java.util.Collection;
+import brooklyn.enricher.Enrichers;
import brooklyn.entity.Entity;
import brooklyn.entity.basic.AbstractEntity;
import brooklyn.entity.basic.Entities;
import brooklyn.entity.group.DynamicCluster;
import brooklyn.entity.proxying.EntitySpec;
import brooklyn.entity.trait.Startable;
+import brooklyn.event.AttributeSensor;
+import brooklyn.event.basic.Sensors;
import brooklyn.location.Location;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+
public class RedisClusterImpl extends AbstractEntity implements RedisCluster {
- protected RedisStore master;
- protected DynamicCluster slaves;
+ private AttributeSensor<RedisStore> MASTER = Sensors.newSensor(RedisStore.class, "redis.master");
+ private AttributeSensor<DynamicCluster> SLAVES = Sensors.newSensor(DynamicCluster.class, "redis.slaves");
public RedisClusterImpl() {
}
@Override
public RedisStore getMaster() {
- return master;
+ return getAttribute(MASTER);
}
@Override
public DynamicCluster getSlaves() {
- return slaves;
+ return getAttribute(SLAVES);
}
@Override
public void start(Collection<? extends Location> locations) {
--- End diff --
If you go down that road of setting starting/running, then I'd probably also separate it out into the two methods - delegate to something like `doStart()` to actually invoke start on the entities.
I think you're free to instantiate the `slave` cluster before calling master.start. That will mean the web-console will show the master + slaves in the tree immediately, rather than waiting for the master to start before the cluster appears.
You could also move the instantiation of the master + slave entities into init. Then you wouldn't need to guard for null either (and don't need to call `Entities.manage()` when inside init - on completion of init, any child entities will immediately get managed).
---
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 basic sensors to RedisCluster
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/782#discussion_r35936178
--- Diff: software/nosql/src/main/java/brooklyn/entity/nosql/redis/RedisClusterImpl.java ---
@@ -80,4 +84,26 @@ protected boolean calculateServiceUp() {
return up;
}
+ @Override
+ public void init() {
+ super.init();
+
+ subscribe(master, RedisStore.HOSTNAME, new MasterAttributePropagator());
--- End diff --
However, at this point (in `init`), `master` will be null. The reason it seems to work is that `subscribe(null, ...)` is taken to mean a wild card to subscribe to all entities for that sensor! Therefore when master does have its sensor published, we receive it here.
Perhaps better to do this subscription / addEnricher in `start` where the `master` is being set.
---
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 basic sensors to RedisCluster
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/782#discussion_r36009934
--- Diff: software/nosql/src/main/java/brooklyn/entity/nosql/redis/RedisClusterImpl.java ---
@@ -20,49 +20,73 @@
import java.util.Collection;
+import brooklyn.enricher.Enrichers;
import brooklyn.entity.Entity;
import brooklyn.entity.basic.AbstractEntity;
import brooklyn.entity.basic.Entities;
import brooklyn.entity.group.DynamicCluster;
import brooklyn.entity.proxying.EntitySpec;
import brooklyn.entity.trait.Startable;
+import brooklyn.event.AttributeSensor;
+import brooklyn.event.basic.Sensors;
import brooklyn.location.Location;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+
public class RedisClusterImpl extends AbstractEntity implements RedisCluster {
- protected RedisStore master;
- protected DynamicCluster slaves;
+ private AttributeSensor<RedisStore> MASTER = Sensors.newSensor(RedisStore.class, "redis.master");
+ private AttributeSensor<DynamicCluster> SLAVES = Sensors.newSensor(DynamicCluster.class, "redis.slaves");
public RedisClusterImpl() {
}
@Override
public RedisStore getMaster() {
- return master;
+ return getAttribute(MASTER);
}
@Override
public DynamicCluster getSlaves() {
- return slaves;
+ return getAttribute(SLAVES);
}
@Override
public void start(Collection<? extends Location> locations) {
- master = addChild(EntitySpec.create(RedisStore.class));
+ RedisStore master = getMaster();
+ if (master == null) {
+ master = addChild(EntitySpec.create(RedisStore.class));
+ setAttribute(MASTER, master);
+ }
+
Entities.manage(master);
- master.start(locations);
+ master.invoke(RedisStore.START, ImmutableMap.<String, Object>of("locations", ImmutableList.copyOf(locations))).blockUntilEnded();
--- End diff --
@mikezaccardo I mislead you to use `blockUntilEnded`. It will not throw exceptions if there was an error while executing the effector. Better use `getUnchecked`.
---
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 basic sensors to RedisCluster
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/782#discussion_r36026395
--- Diff: software/nosql/src/main/java/brooklyn/entity/nosql/redis/RedisClusterImpl.java ---
@@ -20,49 +20,73 @@
import java.util.Collection;
+import brooklyn.enricher.Enrichers;
import brooklyn.entity.Entity;
import brooklyn.entity.basic.AbstractEntity;
import brooklyn.entity.basic.Entities;
import brooklyn.entity.group.DynamicCluster;
import brooklyn.entity.proxying.EntitySpec;
import brooklyn.entity.trait.Startable;
+import brooklyn.event.AttributeSensor;
+import brooklyn.event.basic.Sensors;
import brooklyn.location.Location;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+
public class RedisClusterImpl extends AbstractEntity implements RedisCluster {
- protected RedisStore master;
- protected DynamicCluster slaves;
+ private AttributeSensor<RedisStore> MASTER = Sensors.newSensor(RedisStore.class, "redis.master");
+ private AttributeSensor<DynamicCluster> SLAVES = Sensors.newSensor(DynamicCluster.class, "redis.slaves");
public RedisClusterImpl() {
}
@Override
public RedisStore getMaster() {
- return master;
+ return getAttribute(MASTER);
}
@Override
public DynamicCluster getSlaves() {
- return slaves;
+ return getAttribute(SLAVES);
}
@Override
public void start(Collection<? extends Location> locations) {
--- End diff --
Also see the code in things like `DynamicCluster.start(locations)`, which does things like `ServiceStateLogic.setExpectedState(this, Lifecycle.STARTING);`. That gives more visibility of what the entity is doing and better error reporting. The finally block also ensures that we'll correctly report failures, and will stop the green-spinning in the web-console that was shown while starting.
---
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 basic sensors to RedisCluster
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/782#issuecomment-126807228
Thanks @neykov
Re: `None` location, I did try that before and saw that it worked, so I guess it's just a user experience issue, then. I thought it was odd that it was using the existing location when `None` was selected, but in this context it really determines which *additional* location.
---
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 basic sensors to RedisCluster
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/782#issuecomment-126772880
**This is not yet working properly!**
Stopping and starting the `DynamicCluster` of slaves fails. When testing on `localhost`, a `LocalhostMachineProvisioningLocation` is created (with an ID value we'll call X) upon the first time that the cluster is started. I then invoke the "stop" effector in the Brooklyn GUI which stops the master and slave cluster. I then invoke "start" in the GUI, select "localhost" when prompted for a location, and the slave cluster fails to start. This occurs because another `LocalhostMachineProvisioningLocation` is created with a new ID (we'll call Y). `DynamicClusterImpl` line 262 attempts to add this new location to the `locations` parent member variable. `addLocation()` checks to see if the supplied location already exists, but since the ID of the new localhost location does not match the old one (X != Y), it is added to `locations`. When the location to deploy to is attempted to be retrieved on `DynamicClusterImpl` line 264, an exception is thrown because more than one location n
ow exists.
@aledsage @neykov or anyone else, do you have any thoughts about how to fix this?
---
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 basic sensors to RedisCluster
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/782#issuecomment-126773530
I added a test which validates the error when performing [start / stop / start in new location] on `DynamicClusterImpl`.
---
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 basic sensors to RedisCluster
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/782#discussion_r36163212
--- Diff: software/nosql/src/main/java/brooklyn/entity/nosql/redis/RedisClusterImpl.java ---
@@ -72,12 +120,12 @@ public void restart() {
throw new UnsupportedOperationException();
}
- protected boolean calculateServiceUp() {
- boolean up = false;
- for (Entity member : slaves.getMembers()) {
- if (Boolean.TRUE.equals(member.getAttribute(SERVICE_UP))) up = true;
- }
- return up;
- }
+ private boolean calculateServiceUp() {
+ if (!Boolean.TRUE.equals(getMaster().getAttribute(SERVICE_UP))) return false;
+ for (Entity member : getSlaves().getMembers())
--- End diff --
You can check the status of the `getSlaves()` entity directly, it will update based on the members state. It's at [least one](https://github.com/apache/incubator-brooklyn/blob/master/core/src/main/java/brooklyn/entity/group/DynamicClusterImpl.java#L171) by default.
---
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 basic sensors to RedisCluster
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/782#discussion_r35936037
--- Diff: software/nosql/src/main/java/brooklyn/entity/nosql/redis/RedisClusterImpl.java ---
@@ -80,4 +84,26 @@ protected boolean calculateServiceUp() {
return up;
}
+ @Override
+ public void init() {
+ super.init();
+
+ subscribe(master, RedisStore.HOSTNAME, new MasterAttributePropagator());
--- End diff --
You could instead use:
addEnricher(Enrichers.builder()
.propagating(RedisStore.HOSTNAME, RedisStore.ADDRESS, RedisStore.SUBNET_HOSTNAME, RedisStore.SUBNET_ADDRESS, RedisStore.REDIS_PORT)
.from(master)
.build());
---
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 basic sensors to RedisCluster
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/782#discussion_r36106153
--- Diff: software/nosql/src/main/java/brooklyn/entity/nosql/redis/RedisClusterImpl.java ---
@@ -20,49 +20,73 @@
import java.util.Collection;
+import brooklyn.enricher.Enrichers;
import brooklyn.entity.Entity;
import brooklyn.entity.basic.AbstractEntity;
import brooklyn.entity.basic.Entities;
import brooklyn.entity.group.DynamicCluster;
import brooklyn.entity.proxying.EntitySpec;
import brooklyn.entity.trait.Startable;
+import brooklyn.event.AttributeSensor;
+import brooklyn.event.basic.Sensors;
import brooklyn.location.Location;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+
public class RedisClusterImpl extends AbstractEntity implements RedisCluster {
- protected RedisStore master;
- protected DynamicCluster slaves;
+ private AttributeSensor<RedisStore> MASTER = Sensors.newSensor(RedisStore.class, "redis.master");
--- End diff --
In this case the attributes are used exclusively internally in lieu of storing master and slaves as non-persistent member variables, so I have left them as `private static`. However, if you feel that they should be `public` anyway, I will make that change.
---
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 basic sensors to RedisCluster
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/782
---
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 basic sensors to RedisCluster
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/782#discussion_r36163604
--- Diff: software/nosql/src/main/java/brooklyn/entity/nosql/redis/RedisClusterImpl.java ---
@@ -72,12 +120,12 @@ public void restart() {
throw new UnsupportedOperationException();
}
- protected boolean calculateServiceUp() {
- boolean up = false;
- for (Entity member : slaves.getMembers()) {
- if (Boolean.TRUE.equals(member.getAttribute(SERVICE_UP))) up = true;
- }
- return up;
- }
+ private boolean calculateServiceUp() {
+ if (!Boolean.TRUE.equals(getMaster().getAttribute(SERVICE_UP))) return false;
+ for (Entity member : getSlaves().getMembers())
--- End diff --
Also can replace the manual checks with enricher (in `initEnrichers`)
```
ServiceStateLogic.newEnricherFromChildrenUp().checkChildrenOnly().requireUpChildren(QuorumChecks.all()).addTo(this);
```
Where `QuorumChecks` is in `brooklyn.util.collections.QuorumCheck`.
---
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 basic sensors to RedisCluster
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/782#discussion_r36026553
--- Diff: software/nosql/src/main/java/brooklyn/entity/nosql/redis/RedisClusterImpl.java ---
@@ -20,49 +20,73 @@
import java.util.Collection;
+import brooklyn.enricher.Enrichers;
import brooklyn.entity.Entity;
import brooklyn.entity.basic.AbstractEntity;
import brooklyn.entity.basic.Entities;
import brooklyn.entity.group.DynamicCluster;
import brooklyn.entity.proxying.EntitySpec;
import brooklyn.entity.trait.Startable;
+import brooklyn.event.AttributeSensor;
+import brooklyn.event.basic.Sensors;
import brooklyn.location.Location;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+
public class RedisClusterImpl extends AbstractEntity implements RedisCluster {
- protected RedisStore master;
- protected DynamicCluster slaves;
+ private AttributeSensor<RedisStore> MASTER = Sensors.newSensor(RedisStore.class, "redis.master");
+ private AttributeSensor<DynamicCluster> SLAVES = Sensors.newSensor(DynamicCluster.class, "redis.slaves");
public RedisClusterImpl() {
}
@Override
public RedisStore getMaster() {
- return master;
+ return getAttribute(MASTER);
}
@Override
public DynamicCluster getSlaves() {
- return slaves;
+ return getAttribute(SLAVES);
}
@Override
public void start(Collection<? extends Location> locations) {
- master = addChild(EntitySpec.create(RedisStore.class));
+ RedisStore master = getMaster();
+ if (master == null) {
+ master = addChild(EntitySpec.create(RedisStore.class));
+ setAttribute(MASTER, master);
+ }
+
Entities.manage(master);
- master.start(locations);
+ master.invoke(RedisStore.START, ImmutableMap.<String, Object>of("locations", ImmutableList.copyOf(locations))).getUnchecked();
- slaves = addChild(EntitySpec.create(DynamicCluster.class)
- .configure(DynamicCluster.MEMBER_SPEC, EntitySpec.create(RedisSlave.class).configure(RedisSlave.MASTER, master)));
- slaves.start(locations);
+ DynamicCluster slaves = getSlaves();
+ if (slaves == null) {
+ slaves = addChild(EntitySpec.create(DynamicCluster.class)
+ .configure(DynamicCluster.MEMBER_SPEC, EntitySpec.create(RedisSlave.class).configure(RedisSlave.MASTER, master)));
+ setAttribute(SLAVES, slaves);
+ }
+
+ slaves.invoke(DynamicCluster.START, ImmutableMap.<String, Object>of("locations", ImmutableList.copyOf(locations))).getUnchecked();
setAttribute(Startable.SERVICE_UP, calculateServiceUp());
+
+ addEnricher(Enrichers.builder()
+ .propagating(RedisStore.HOSTNAME, RedisStore.ADDRESS, RedisStore.SUBNET_HOSTNAME, RedisStore.SUBNET_ADDRESS, RedisStore.REDIS_PORT)
+ .from(master)
+ .build());
}
@Override
public void stop() {
--- End diff --
See the pattern used in `DynamicCluster.stop()`, for setting stopping/stopped/on_fire.
---
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.
---