From 4d824127a31b56a411c29a7c4f941471ea436eed Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Mon, 24 Jul 2017 16:49:58 +0100 Subject: [PATCH 1/4] allow simple constant quora to be set as a constant (no need for complex range) --- .../util/collections/QuorumCheck.java | 21 +++++++++++++++++-- .../coerce/CommonAdaptorTypeCoercions.java | 12 +++++++++++ .../util/collections/QuorumChecksTest.java | 10 ++++++--- 3 files changed, 38 insertions(+), 5 deletions(-) diff --git a/utils/common/src/main/java/org/apache/brooklyn/util/collections/QuorumCheck.java b/utils/common/src/main/java/org/apache/brooklyn/util/collections/QuorumCheck.java index bc96763ec4..c150f4718d 100644 --- a/utils/common/src/main/java/org/apache/brooklyn/util/collections/QuorumCheck.java +++ b/utils/common/src/main/java/org/apache/brooklyn/util/collections/QuorumCheck.java @@ -19,6 +19,7 @@ package org.apache.brooklyn.util.collections; import java.io.Serializable; +import java.util.Collection; import java.util.Iterator; import java.util.List; @@ -112,7 +113,9 @@ public static QuorumCheck newLinearRange(String range) { */ @SuppressWarnings({ "rawtypes", "unchecked" }) public static QuorumCheck newLinearRange(String range, String name) { - return LinearRangeQuorumCheck.of(name, (Iterable)Iterables.getOnlyElement( Yamls.parseAll(range) )); + Object input = Iterables.getOnlyElement( Yamls.parseAll(range) ); + if (input instanceof Iterable) return LinearRangeQuorumCheck.of(name, (Iterable)input); + throw new IllegalArgumentException("Invalid input to linear range quorum check; should be a list of points (not '"+range+"')"); } private static final List NAMED_CHECKS = MutableList @@ -126,7 +129,21 @@ public static QuorumCheck of(String nameOrRange) { return qc; } } - return newLinearRange(nameOrRange); + // parse YAML + Object input = Iterables.getOnlyElement( Yamls.parseAll(nameOrRange) ); + if (input instanceof Collection) return of((Collection)input); + if (input instanceof Integer || input instanceof Long) return of((int)((Number)input)); + // TODO also accept "50%", and "50%,1" + throw new IllegalArgumentException("Unknown quorum check format '"+input+"'"); + } + + @SuppressWarnings({ "unchecked", "rawtypes" }) + public static QuorumCheck of(Collection pointsForLinearRange) { + return LinearRangeQuorumCheck.of(null, (Iterable)pointsForLinearRange); + } + + public static QuorumCheck of(Integer constant) { + return newInstance(constant, 0.0, constant>0); } } diff --git a/utils/common/src/main/java/org/apache/brooklyn/util/javalang/coerce/CommonAdaptorTypeCoercions.java b/utils/common/src/main/java/org/apache/brooklyn/util/javalang/coerce/CommonAdaptorTypeCoercions.java index ae4c8b3e88..8d6e92494a 100644 --- a/utils/common/src/main/java/org/apache/brooklyn/util/javalang/coerce/CommonAdaptorTypeCoercions.java +++ b/utils/common/src/main/java/org/apache/brooklyn/util/javalang/coerce/CommonAdaptorTypeCoercions.java @@ -257,6 +257,18 @@ public QuorumCheck apply(final String input) { return QuorumChecks.of(input); } }); + registerAdapter(Integer.class, QuorumCheck.class, new Function() { + @Override + public QuorumCheck apply(final Integer input) { + return QuorumChecks.of(input); + } + }); + registerAdapter(Collection.class, QuorumCheck.class, new Function() { + @Override + public QuorumCheck apply(final Collection input) { + return QuorumChecks.of(input); + } + }); registerAdapter(String.class, TimeZone.class, new Function() { @Override public TimeZone apply(final String input) { diff --git a/utils/common/src/test/java/org/apache/brooklyn/util/collections/QuorumChecksTest.java b/utils/common/src/test/java/org/apache/brooklyn/util/collections/QuorumChecksTest.java index 421092142f..0d996e8eeb 100644 --- a/utils/common/src/test/java/org/apache/brooklyn/util/collections/QuorumChecksTest.java +++ b/utils/common/src/test/java/org/apache/brooklyn/util/collections/QuorumChecksTest.java @@ -99,7 +99,11 @@ public void testLinearNeedHalfToTenAndTenPercentAtHundred() { Assert.assertTrue(q.isQuorate(31, 300)); } - - - + @Test + public void testConstantQuorum() { + QuorumCheck q = QuorumChecks.of("2"); + Assert.assertTrue(q.isQuorate(2, 2)); + Assert.assertTrue(q.isQuorate(2, 10)); + Assert.assertFalse(q.isQuorate(1, 1)); + } } From 14565cd13e74082f8e6494deff40683d51b5978e Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Mon, 24 Jul 2017 16:50:29 +0100 Subject: [PATCH 2/4] display nicer message when required child/quorum not healthy --- .../core/entity/lifecycle/ServiceStateLogic.java | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/apache/brooklyn/core/entity/lifecycle/ServiceStateLogic.java b/core/src/main/java/org/apache/brooklyn/core/entity/lifecycle/ServiceStateLogic.java index ba08a443ed..0ce9169dcc 100644 --- a/core/src/main/java/org/apache/brooklyn/core/entity/lifecycle/ServiceStateLogic.java +++ b/core/src/main/java/org/apache/brooklyn/core/entity/lifecycle/ServiceStateLogic.java @@ -611,8 +611,20 @@ else if (!ignoreStates.contains(state.getValue())) } return "Required entit"+Strings.ies(onesNotHealthy.size())+" not healthy: "+ - (onesNotHealthy.size()>3 ? onesNotHealthy.get(0)+" and "+(onesNotHealthy.size()-1)+" others" - : Strings.join(onesNotHealthy, ", ")); + (onesNotHealthy.size()>3 ? nameOfEntity(onesNotHealthy.get(0))+" and "+(onesNotHealthy.size()-1)+" others" + : Strings.join(nameOfEntity(onesNotHealthy), ", ")); + } + + private List nameOfEntity(List entities) { + List result = MutableList.of(); + for (Entity e: entities) result.add(nameOfEntity(e)); + return result; + } + + private String nameOfEntity(Entity entity) { + String name = entity.getDisplayName(); + if (name.contains(entity.getId())) return name; + else return name + " ("+entity.getId()+")"; } protected void updateMapSensor(AttributeSensor> sensor, Object value) { From b2251f48c46cdf88d375f8a593990f2edf860011 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Mon, 24 Jul 2017 16:51:53 +0100 Subject: [PATCH 3/4] fix errors with DynamicMultiGroup minor things: * javadoc * quorum checks should only look at children (the buckets); the members are observed by the children * if there is a problem, fail more gracefully (otherwise it risks adding nodes ad infinitum) --- .../entity/group/DynamicMultiGroupImpl.java | 37 ++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/apache/brooklyn/entity/group/DynamicMultiGroupImpl.java b/core/src/main/java/org/apache/brooklyn/entity/group/DynamicMultiGroupImpl.java index f80194d0c7..b22566a396 100644 --- a/core/src/main/java/org/apache/brooklyn/entity/group/DynamicMultiGroupImpl.java +++ b/core/src/main/java/org/apache/brooklyn/entity/group/DynamicMultiGroupImpl.java @@ -18,6 +18,7 @@ */ package org.apache.brooklyn.entity.group; +import java.util.Collection; import java.util.Map; import java.util.Set; import java.util.concurrent.Callable; @@ -30,9 +31,13 @@ import org.apache.brooklyn.api.entity.Group; import org.apache.brooklyn.api.sensor.AttributeSensor; import org.apache.brooklyn.core.entity.Entities; +import org.apache.brooklyn.core.entity.lifecycle.ServiceStateLogic; +import org.apache.brooklyn.core.entity.lifecycle.ServiceStateLogic.ServiceProblemsLogic; import org.apache.brooklyn.feed.function.FunctionFeed; import org.apache.brooklyn.feed.function.FunctionPollConfig; import org.apache.brooklyn.util.collections.MutableMap; +import org.apache.brooklyn.util.collections.MutableSet; +import org.apache.brooklyn.util.exceptions.Exceptions; import org.apache.brooklyn.util.time.Time; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -101,6 +106,22 @@ public void init() { connectScanner(); } + @Override + protected void initEnrichers() { + super.initEnrichers(); + // check states and upness separately so they can be individually replaced if desired + // problem if any children or members are on fire + enrichers().add(ServiceStateLogic.newEnricherFromChildrenState() + .checkChildrenOnly() + .requireRunningChildren(getConfig(RUNNING_QUORUM_CHECK)) + .suppressDuplicates(true)); + // defaults to requiring at least one member or child who is up + enrichers().add(ServiceStateLogic.newEnricherFromChildrenUp() + .checkChildrenOnly() + .requireUpChildren(getConfig(UP_QUORUM_CHECK)) + .suppressDuplicates(true)); + } + private void connectScanner() { Long interval = getConfig(RESCAN_INTERVAL); if (interval != null && interval > 0L) { @@ -211,10 +232,24 @@ public void distributeEntities() { Iterables.filter(getMembers(), Predicates.compose(Predicates.notNull(), bucketFunction)), bucketFunction); // Now fill the buckets + Collection oldChildren = getChildren(); for (String name : entityMapping.keySet()) { BasicGroup bucket = buckets.get(name); if (bucket == null) { - bucket = addChild(EntitySpec.create(bucketSpec).displayName(name)); + try { + bucket = addChild(EntitySpec.create(bucketSpec).displayName(name)); + } catch (Exception e) { + Exceptions.propagateIfFatal(e); + ServiceProblemsLogic.updateProblemsIndicator(this, "children", "Could not add child; removing all new children for now: "+Exceptions.collapseText(e)); + // if we don't do this, they get added infinitely often + MutableSet newChildren = MutableSet.copyOf(getChildren()); + newChildren.removeAll(oldChildren); + for (Entity child: newChildren) { + removeChild(child); + } + throw e; + } + ServiceProblemsLogic.clearProblemsIndicator(this, "children"); buckets.put(name, bucket); } bucket.setMembers(entityMapping.get(name)); From e8d25ca95a43af52c0c8a97e2da1c707f9d7fa68 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Mon, 24 Jul 2017 17:01:51 +0100 Subject: [PATCH 4/4] remove FIRST_MEMBER sensor, it doesn't work, and deprecate FIRST both are a bit flaky, but FIRST_MEMBER especially so: it would never get cleared, and it could cause deadlock (as parent attempts to update child's sensor while holding members lock, meanwhile child might publish a sensor which causes parent subscriber to try to getMembers). additionally we no longer set FIRST=(other entity) on the child, for the same reasons, but also it causes conflict if the child is also a group (so FIRST might be the parent's FIRST or its own FIRST). the new "primary" model is a much better one to use instead when you need notification of something being promoted, or an enricher if you need the parent's first injected locally (but you should always be able to say `$brooklyn:parent().attributeWhenReady("first"))`) --- .../brooklyn/entity/group/AbstractGroup.java | 14 +++++++++----- .../brooklyn/entity/group/AbstractGroupImpl.java | 15 +++------------ .../brooklyn/entity/group/DynamicClusterImpl.java | 2 +- .../brooklyn/entity/group/DynamicClusterTest.java | 2 +- 4 files changed, 14 insertions(+), 19 deletions(-) diff --git a/core/src/main/java/org/apache/brooklyn/entity/group/AbstractGroup.java b/core/src/main/java/org/apache/brooklyn/entity/group/AbstractGroup.java index 625d9810a7..66d2d897c5 100644 --- a/core/src/main/java/org/apache/brooklyn/entity/group/AbstractGroup.java +++ b/core/src/main/java/org/apache/brooklyn/entity/group/AbstractGroup.java @@ -50,11 +50,9 @@ public interface AbstractGroup extends Entity, Group, Changeable { AttributeSensor> GROUP_MEMBERS = Sensors.newSensor( new TypeToken>() { }, "group.members", "Members of the group"); - // FIXME should definitely remove this, it is ambiguous if an entity is in multiple clusters. also should be "is_first" or something to indicate boolean. - AttributeSensor FIRST_MEMBER = Sensors.newBooleanSensor( - "cluster.first", "Set on an entity if it is the first member of a cluster"); - - // FIXME can we remove this too? + /** @deprecated since 0.12.0 use AbstractGroup.getFirst(Group) if required, + * or better use an external enricher or policy to define the primary. */ + @Deprecated AttributeSensor FIRST = Sensors.newSensor(Entity.class, "cluster.first.entity", "The first member of the cluster"); @@ -87,4 +85,10 @@ public interface AbstractGroup extends Entity, Group, Changeable { // FIXME Do we really want this method? "setMembers" is a misleading name void setMembers(Collection mm, Predicate filter); + public static Entity getFirst(Group g) { + Collection members = g.sensors().get(AbstractGroup.GROUP_MEMBERS); + if (!members.isEmpty()) return members.iterator().next(); + return null; + } + } diff --git a/core/src/main/java/org/apache/brooklyn/entity/group/AbstractGroupImpl.java b/core/src/main/java/org/apache/brooklyn/entity/group/AbstractGroupImpl.java index 23fb921db1..81f6da4f25 100644 --- a/core/src/main/java/org/apache/brooklyn/entity/group/AbstractGroupImpl.java +++ b/core/src/main/java/org/apache/brooklyn/entity/group/AbstractGroupImpl.java @@ -107,16 +107,7 @@ public boolean addMember(Entity member) { // FIXME do not set sensors on members; possibly we don't need FIRST at all, just look at the first in MEMBERS, and take care to guarantee order there Entity first = getAttribute(FIRST); if (first == null) { - member.sensors().set(FIRST_MEMBER, true); - member.sensors().set(FIRST, member); sensors().set(FIRST, member); - } else { - if (first.equals(member) || first.equals(member.getAttribute(FIRST))) { - // do nothing (rebinding) - } else { - member.sensors().set(FIRST_MEMBER, false); - member.sensors().set(FIRST, first); - } } ((EntityInternal)member).groups().add((Group)getProxyIfAvailable()); @@ -165,10 +156,10 @@ public boolean removeMember(final Entity member) { log.debug("Group {} lost member {}", this, member); // TODO ideally the following are all synched sensors().set(GROUP_SIZE, getCurrentSize()); - sensors().set(GROUP_MEMBERS, getMembers()); + Collection membersNow = getMembers(); + sensors().set(GROUP_MEMBERS, membersNow); if (member.equals(getAttribute(FIRST))) { - // TODO should we elect a new FIRST ? as is the *next* will become first. could we do away with FIRST altogether? - sensors().set(FIRST, null); + sensors().set(FIRST, membersNow.isEmpty() ? null : membersNow.iterator().next()); } // emit after the above so listeners can use getMembers() and getCurrentSize() sensors().emit(MEMBER_REMOVED, member); diff --git a/core/src/main/java/org/apache/brooklyn/entity/group/DynamicClusterImpl.java b/core/src/main/java/org/apache/brooklyn/entity/group/DynamicClusterImpl.java index 03eec38db2..0b2ec02078 100644 --- a/core/src/main/java/org/apache/brooklyn/entity/group/DynamicClusterImpl.java +++ b/core/src/main/java/org/apache/brooklyn/entity/group/DynamicClusterImpl.java @@ -847,7 +847,7 @@ protected ReferenceWithError> addInEachLocation(Iterable args = ImmutableMap.of("locations", MutableList.builder().addIfNotNull(loc).buildImmutable()); Task task = newThrottledEffectorTask(entity, Startable.START, args, privileged); tasks.put(entity, task); diff --git a/core/src/test/java/org/apache/brooklyn/entity/group/DynamicClusterTest.java b/core/src/test/java/org/apache/brooklyn/entity/group/DynamicClusterTest.java index 5b76bbcd33..f4a303db8e 100644 --- a/core/src/test/java/org/apache/brooklyn/entity/group/DynamicClusterTest.java +++ b/core/src/test/java/org/apache/brooklyn/entity/group/DynamicClusterTest.java @@ -1255,7 +1255,7 @@ public static class ThrowOnAsyncStartEntityImpl extends TestEntityImpl implement public void start(Collection locs) { int count = config().get(COUNTER).incrementAndGet(); try { - LOG.debug("{} starting (first={})", new Object[]{this, sensors().get(AbstractGroup.FIRST_MEMBER)}); + LOG.debug("{} starting (members={})", new Object[]{this, getParent().sensors().get(AbstractGroup.GROUP_MEMBERS)}); config().get(START_LATCH); // Throw if more than one entity is starting at the same time as this. assertTrue(count <= config().get(MAX_CONCURRENCY), "expected " + count + " <= " + config().get(MAX_CONCURRENCY));