From 94bc8925832649627f99a3162d471de7c05c95a2 Mon Sep 17 00:00:00 2001 From: Aled Sage Date: Fri, 5 Sep 2014 19:08:01 +0100 Subject: [PATCH 1/7] BROOKLYN-66: Fix deadlock in LocalEntityManager --- .../internal/LocalEntityManager.java | 17 ++++ .../entity/basic/DynamicGroupTest.java | 85 +++++++++++++++++++ 2 files changed, 102 insertions(+) diff --git a/core/src/main/java/brooklyn/management/internal/LocalEntityManager.java b/core/src/main/java/brooklyn/management/internal/LocalEntityManager.java index f03ce44a46..9f9e5e3445 100644 --- a/core/src/main/java/brooklyn/management/internal/LocalEntityManager.java +++ b/core/src/main/java/brooklyn/management/internal/LocalEntityManager.java @@ -398,6 +398,23 @@ private synchronized boolean manageNonRecursive(Entity e) { * Returns true if the entity has been removed from management; if it was not previously managed (anything else throws exception) */ private boolean unmanageNonRecursive(Entity e) { + /* + * When method is synchronized, hit deadlock: + * 1. thread called unmanage() on a member of a group, so we got the lock and called group.removeMember; + * this ties to synchronize on AbstractGroupImpl.members + * 2. another thread was doing AbstractGroupImpl.addMember, which is synchronized on AbstractGroupImpl.members; + * it tries to call Entities.manage(child) which calls LocalEntityManager.getEntity(), which is + * synchronized on this. + * + * We MUST NOT call alien code from within the management framework while holding locks. + * The AbstractGroup.removeMember is effectively alien because a user could override it, and because + * it is entity specific. + * + * TODO Does getting then removing from groups risk this entity being added to other groups while + * this is happening? Should abstractEntity.onManagementStopped or some such remove the entity + * from its groups? + */ + Collection groups = e.getGroups(); e.clearParent(); for (Group group : groups) { diff --git a/core/src/test/java/brooklyn/entity/basic/DynamicGroupTest.java b/core/src/test/java/brooklyn/entity/basic/DynamicGroupTest.java index b52332b2f8..f1af44fa7c 100644 --- a/core/src/test/java/brooklyn/entity/basic/DynamicGroupTest.java +++ b/core/src/test/java/brooklyn/entity/basic/DynamicGroupTest.java @@ -23,11 +23,13 @@ import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertTrue; +import java.util.Collection; import java.util.List; import java.util.Set; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import org.slf4j.Logger; @@ -39,6 +41,7 @@ import brooklyn.entity.Entity; import brooklyn.entity.proxying.EntitySpec; import brooklyn.event.AttributeSensor; +import brooklyn.event.Sensor; import brooklyn.event.SensorEvent; import brooklyn.event.SensorEventListener; import brooklyn.event.basic.Sensors; @@ -456,4 +459,86 @@ public void run() { assertEqualsIgnoringOrder(group2.getMembers(), ImmutableSet.of(e3)); }}); } + + // See deadlock in https://issues.apache.org/jira/browse/BROOKLYN-66 + @Test + public void testDoesNotDeadlockOnUnmanageWhileOtherMemberBeingAdded() throws Exception { + final CountDownLatch removingMemberReachedLatch = new CountDownLatch(1); + final CountDownLatch addingMemberReachedLatch = new CountDownLatch(1); + final CountDownLatch addingMemberContinueLatch = new CountDownLatch(1); + final AtomicBoolean addingMemberDoLatching = new AtomicBoolean(false); + final List membersAdded = new CopyOnWriteArrayList(); + + final DynamicGroupImpl group2 = new DynamicGroupImpl() { + @Override + public void emit(Sensor sensor, T val) { + // intercept inside AbstractGroup.addMember, while it still holds lock on members + if (sensor == AbstractGroup.MEMBER_ADDED && addingMemberDoLatching.get()) { + addingMemberReachedLatch.countDown(); + try { + addingMemberContinueLatch.await(); + } catch (InterruptedException e) { + throw Exceptions.propagate(e); + } + } + super.emit(sensor, val); + } + @Override + public boolean removeMember(final Entity member) { + removingMemberReachedLatch.countDown(); + return super.removeMember(member); + } + }; + group2.setConfig(DynamicGroup.MEMBER_DELEGATE_CHILDREN, true); + app.addChild(group2); + Entities.manage(group2); + + app.subscribe(group2, AbstractGroup.MEMBER_ADDED, new SensorEventListener() { + @Override public void onEvent(SensorEvent event) { + membersAdded.add(event.getValue()); + }}); + + final TestEntity e2 = app.createAndManageChild(EntitySpec.create(TestEntity.class)); + final TestEntity e3 = app.createAndManageChild(EntitySpec.create(TestEntity.class)); + group2.addMember(e2); + assertContainsEventually(membersAdded, e2); + addingMemberDoLatching.set(true); + + Thread t1 = new Thread(new Runnable() { + @Override public void run() { + try { + addingMemberReachedLatch.await(); + } catch (InterruptedException e) { + throw Exceptions.propagate(e); + } + Entities.unmanage(e2); + }}); + + Thread t2 = new Thread(new Runnable() { + @Override public void run() { + group2.addMember(e3); + }}); + + t1.start(); + t2.start(); + + try { + removingMemberReachedLatch.await(); + addingMemberContinueLatch.countDown(); + t1.join(TIMEOUT_MS); + t2.join(TIMEOUT_MS); + assertFalse(t1.isAlive()); + assertFalse(t2.isAlive()); + } finally { + t1.interrupt(); + t2.interrupt(); + } + } + + private void assertContainsEventually(final Collection vals, final T val) { + Asserts.succeedsEventually(new Runnable() { + public void run() { + assertTrue(vals.contains(val)); + }}); + } } From 13b7304f836bc2c4b69b28940a6e7d94290b0fba Mon Sep 17 00:00:00 2001 From: Aled Sage Date: Fri, 5 Sep 2014 19:22:32 +0100 Subject: [PATCH 2/7] Fix concurrency issue stopping cluster MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - when resizing to zero, we shrink by currentSize. If someone concurrently stops an entity, then don’t worry that we were only able to stop (currentSize-1) entities. Just continue. --- .../entity/group/DynamicClusterImpl.java | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/brooklyn/entity/group/DynamicClusterImpl.java b/core/src/main/java/brooklyn/entity/group/DynamicClusterImpl.java index be7724d2ad..3c1bc82934 100644 --- a/core/src/main/java/brooklyn/entity/group/DynamicClusterImpl.java +++ b/core/src/main/java/brooklyn/entity/group/DynamicClusterImpl.java @@ -786,7 +786,8 @@ protected List pickAndRemoveMembers(int delta) { return Lists.newArrayList(); if (delta == 1 && !isAvailabilityZoneEnabled()) { - return ImmutableList.of(pickAndRemoveMember()); // for backwards compatibility in sub-classes + Maybe member = tryPickAndRemoveMember(); + return (member.isPresent()) ? ImmutableList.of(member.get()) : ImmutableList.of(); } // TODO inefficient impl @@ -807,28 +808,29 @@ protected List pickAndRemoveMembers(int delta) { } else { List entities = Lists.newArrayList(); for (int i = 0; i < delta; i++) { - entities.add(pickAndRemoveMember()); + // don't assume we have enough members; e.g. if shrinking to zero and someone else concurrently stops a member, + // then just return what we were able to remove. + Maybe member = tryPickAndRemoveMember(); + if (member.isPresent()) entities.add(member.get()); } return entities; } } - /** - * @deprecated since 0.6.0; subclasses should instead override {@link #pickAndRemoveMembers(int)} if they really need to! - */ - protected Entity pickAndRemoveMember() { + private Maybe tryPickAndRemoveMember() { assert !isAvailabilityZoneEnabled() : "should instead call pickAndRemoveMembers(int) if using availability zones"; // TODO inefficient impl - Preconditions.checkState(getMembers().size() > 0, "Attempt to remove a node when members is empty, from cluster "+this); - if (LOG.isDebugEnabled()) LOG.debug("Removing a node from {}", this); + Collection members = getMembers(); + if (members.isEmpty()) return Maybe.absent(); - Entity entity = getRemovalStrategy().apply(getMembers()); + if (LOG.isDebugEnabled()) LOG.debug("Removing a node from {}", this); + Entity entity = getRemovalStrategy().apply(members); Preconditions.checkNotNull(entity, "No entity chosen for removal from "+getId()); Preconditions.checkState(entity instanceof Startable, "Chosen entity for removal not stoppable: cluster="+this+"; choice="+entity); removeMember(entity); - return entity; + return Maybe.of(entity); } protected void discardNode(Entity entity) { From 0fd94773569c932df0829b61d307be8ea391a378 Mon Sep 17 00:00:00 2001 From: Aled Sage Date: Fri, 5 Sep 2014 19:55:18 +0100 Subject: [PATCH 3/7] Fix failing ServiceReplacerTest.testSetsOnFireWhenFailToReplaceMember --- .../brooklyn/entity/basic/ServiceStateLogic.java | 2 +- .../brooklyn/policy/ha/ServiceReplacerTest.java | 15 +++++++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/brooklyn/entity/basic/ServiceStateLogic.java b/core/src/main/java/brooklyn/entity/basic/ServiceStateLogic.java index 95d5e57996..459723067a 100644 --- a/core/src/main/java/brooklyn/entity/basic/ServiceStateLogic.java +++ b/core/src/main/java/brooklyn/entity/basic/ServiceStateLogic.java @@ -386,7 +386,7 @@ protected Collection> getSourceSensors() { protected void onUpdated() { if (entity==null || !Entities.isManaged(entity)) { // either invoked during setup or entity has become unmanaged; just ignore - log.debug("Ignoring {} onUpdated when entity is not in valid state ({})", this, entity); + if (log.isDebugEnabled()) log.debug("Ignoring {} onUpdated when entity is not in valid state ({})", this, entity); return; } diff --git a/policy/src/test/java/brooklyn/policy/ha/ServiceReplacerTest.java b/policy/src/test/java/brooklyn/policy/ha/ServiceReplacerTest.java index 900405b2fb..f621536814 100644 --- a/policy/src/test/java/brooklyn/policy/ha/ServiceReplacerTest.java +++ b/policy/src/test/java/brooklyn/policy/ha/ServiceReplacerTest.java @@ -37,6 +37,8 @@ import brooklyn.entity.basic.Entities; import brooklyn.entity.basic.EntityInternal; import brooklyn.entity.basic.Lifecycle; +import brooklyn.entity.basic.QuorumCheck; +import brooklyn.entity.basic.ServiceStateLogic.ComputeServiceIndicatorsFromChildrenAndMembers; import brooklyn.entity.group.DynamicCluster; import brooklyn.entity.proxying.EntitySpec; import brooklyn.entity.trait.FailingEntity; @@ -116,6 +118,11 @@ public void testReplacesFailedMember() throws Exception { assertFalse(Entities.isManaged(e1)); }}); } + + @Test(invocationCount=100) + public void testSetsOnFireWhenFailToReplaceMemberManyTimes() throws Exception { + testSetsOnFireWhenFailToReplaceMember(); + } // fails the startup of the replacement entity (but not the original). @Test @@ -126,7 +133,9 @@ public void testSetsOnFireWhenFailToReplaceMember() throws Exception { .configure(DynamicCluster.MEMBER_SPEC, EntitySpec.create(FailingEntity.class) .configure(FailingEntity.FAIL_ON_START_CONDITION, predicateOnlyTrueForCallAtOrAfter(2))) .configure(DynamicCluster.INITIAL_SIZE, 1) - .configure(DynamicCluster.QUARANTINE_FAILED_ENTITIES, true)); + .configure(DynamicCluster.QUARANTINE_FAILED_ENTITIES, true) + .configure(ComputeServiceIndicatorsFromChildrenAndMembers.UP_QUORUM_CHECK, QuorumCheck.QuorumChecks.alwaysTrue()) + .configure(ComputeServiceIndicatorsFromChildrenAndMembers.RUNNING_QUORUM_CHECK, QuorumCheck.QuorumChecks.alwaysTrue())); app.start(ImmutableList.of(loc)); ServiceReplacer policy = new ServiceReplacer(new ConfigBag().configure(ServiceReplacer.FAILURE_SENSOR_TO_MONITOR, HASensors.ENTITY_FAILED)); @@ -138,9 +147,11 @@ public void testSetsOnFireWhenFailToReplaceMember() throws Exception { e1.emit(HASensors.ENTITY_FAILED, new FailureDescriptor(e1, "simulate failure")); // Expect cluster to go on-fire when fails to start replacement + // Note that we've set up-quorum and running-quorum to be "alwaysTrue" so that we don't get a transient onFire + // when the failed node fails to start (but before it has been removed from the group to be put in quarantine). EntityTestUtils.assertAttributeEqualsEventually(cluster, Attributes.SERVICE_STATE_ACTUAL, Lifecycle.ON_FIRE); - // And expect to have the second failed entity still kicking around as proof (in quarantine) + // Expect to have the second failed entity still kicking around as proof (in quarantine) Iterable members = Iterables.filter(managementContext.getEntityManager().getEntities(), Predicates.instanceOf(FailingEntity.class)); assertEquals(Iterables.size(members), 2); From 3d231e8dcc7fb6b471331171a3aa2cd45d7385de Mon Sep 17 00:00:00 2001 From: Aled Sage Date: Fri, 5 Sep 2014 20:02:12 +0100 Subject: [PATCH 4/7] Change EntityAdjuncts.findWithUniqueTag to tryFindWithUniqueTag - Make it consistent with guava naming --- .../main/java/brooklyn/entity/basic/EntityAdjuncts.java | 7 ++++--- .../main/java/brooklyn/entity/basic/ServiceStateLogic.java | 7 ++++--- .../java/brooklyn/entity/basic/ServiceStateLogicTest.java | 4 ++-- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/brooklyn/entity/basic/EntityAdjuncts.java b/core/src/main/java/brooklyn/entity/basic/EntityAdjuncts.java index ab1d003de8..242a61dbc5 100644 --- a/core/src/main/java/brooklyn/entity/basic/EntityAdjuncts.java +++ b/core/src/main/java/brooklyn/entity/basic/EntityAdjuncts.java @@ -28,6 +28,7 @@ import brooklyn.policy.Enricher; import brooklyn.policy.EntityAdjunct; import brooklyn.util.collections.MutableList; +import brooklyn.util.guava.Maybe; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; @@ -37,12 +38,12 @@ */ public class EntityAdjuncts { - public static T findWithUniqueTag(Iterable adjuncts, Object tag) { + public static Maybe tryFindWithUniqueTag(Iterable adjuncts, Object tag) { Preconditions.checkNotNull(tag, "tag"); for (T adjunct: adjuncts) if (tag.equals(adjunct.getUniqueTag())) - return adjunct; - return null; + return Maybe.of(adjunct); + return Maybe.absent("Not found with tag "+tag); } public static final List SYSTEM_ENRICHER_UNIQUE_TAGS = ImmutableList.of( diff --git a/core/src/main/java/brooklyn/entity/basic/ServiceStateLogic.java b/core/src/main/java/brooklyn/entity/basic/ServiceStateLogic.java index 459723067a..fd524ec85c 100644 --- a/core/src/main/java/brooklyn/entity/basic/ServiceStateLogic.java +++ b/core/src/main/java/brooklyn/entity/basic/ServiceStateLogic.java @@ -50,6 +50,7 @@ import brooklyn.util.collections.MutableMap; import brooklyn.util.collections.MutableSet; import brooklyn.util.guava.Functionals; +import brooklyn.util.guava.Maybe; import brooklyn.util.text.Strings; import com.google.common.base.Function; @@ -115,9 +116,9 @@ public static void updateMapSensorEntry(EntityLocal entity, Attribut public static void setExpectedState(Entity entity, Lifecycle state) { ((EntityInternal)entity).setAttribute(Attributes.SERVICE_STATE_EXPECTED, new Lifecycle.Transition(state, new Date())); - Enricher enricher = EntityAdjuncts.findWithUniqueTag(entity.getEnrichers(), ComputeServiceState.DEFAULT_ENRICHER_UNIQUE_TAG); - if (enricher instanceof ComputeServiceState) { - ((ComputeServiceState)enricher).onEvent(null); + Maybe enricher = EntityAdjuncts.tryFindWithUniqueTag(entity.getEnrichers(), ComputeServiceState.DEFAULT_ENRICHER_UNIQUE_TAG); + if (enricher.isPresent() && enricher.get() instanceof ComputeServiceState) { + ((ComputeServiceState)enricher.get()).onEvent(null); } } public static Lifecycle getExpectedState(Entity entity) { diff --git a/core/src/test/java/brooklyn/entity/basic/ServiceStateLogicTest.java b/core/src/test/java/brooklyn/entity/basic/ServiceStateLogicTest.java index ca5d8c1078..bd610318b8 100644 --- a/core/src/test/java/brooklyn/entity/basic/ServiceStateLogicTest.java +++ b/core/src/test/java/brooklyn/entity/basic/ServiceStateLogicTest.java @@ -36,6 +36,7 @@ import brooklyn.test.EntityTestUtils; import brooklyn.test.entity.TestEntity; import brooklyn.util.exceptions.Exceptions; +import brooklyn.util.guava.Maybe; import brooklyn.util.time.Duration; import com.google.common.collect.ImmutableList; @@ -180,8 +181,7 @@ public void testManuallySettingIndicatorsOnApplicationsIsMoreComplicated() { assertAttributeEquals(app, Attributes.SERVICE_STATE_ACTUAL, Lifecycle.RUNNING); // if we change the state quorum check for the app to be "all are healthy and at least one running" *then* it shows stopped // (normally this would be done in `initEnrichers` of course) - Enricher appChildrenBasedEnricher = EntityAdjuncts.findWithUniqueTag(app.getEnrichers(), ComputeServiceIndicatorsFromChildrenAndMembers.DEFAULT_UNIQUE_TAG); - Assert.assertNotNull(appChildrenBasedEnricher, "Expected enricher not found"); + Enricher appChildrenBasedEnricher = EntityAdjuncts.tryFindWithUniqueTag(app.getEnrichers(), ComputeServiceIndicatorsFromChildrenAndMembers.DEFAULT_UNIQUE_TAG).get(); appChildrenBasedEnricher.setConfig(ComputeServiceIndicatorsFromChildrenAndMembers.RUNNING_QUORUM_CHECK, QuorumChecks.allAndAtLeastOne()); assertAttributeEqualsEventually(app, Attributes.SERVICE_STATE_ACTUAL, Lifecycle.ON_FIRE); From 287947a05d68ef35a3472b59f2e7fd2a91be8ae5 Mon Sep 17 00:00:00 2001 From: Aled Sage Date: Fri, 5 Sep 2014 20:10:32 +0100 Subject: [PATCH 5/7] Move some tests form AbstractEntityLegacyTest to EntitySpecTest --- .../basic/AbstractEntityLegacyTest.java | 29 ------------------- .../brooklyn/entity/basic/EntitySpecTest.java | 24 +++++++++++++-- 2 files changed, 22 insertions(+), 31 deletions(-) diff --git a/core/src/test/java/brooklyn/entity/basic/AbstractEntityLegacyTest.java b/core/src/test/java/brooklyn/entity/basic/AbstractEntityLegacyTest.java index 344bd061b1..0555db0e62 100644 --- a/core/src/test/java/brooklyn/entity/basic/AbstractEntityLegacyTest.java +++ b/core/src/test/java/brooklyn/entity/basic/AbstractEntityLegacyTest.java @@ -107,18 +107,6 @@ public void testLegacyConstructionCallsConfigureMethod() throws Exception { assertEquals(entity.getConfigureDuringConstructionCount(), 1); } - // TODO move the testNewStyle methods (several of them) to EntitySpecTest ? - // make sure to keep them when legacy approach removed! - // see https://github.com/apache/incubator-brooklyn/pull/113#discussion-diff-16474353 - @Test - public void testNewStyleCallsConfigureAfterConstruction() throws Exception { - app = TestApplication.Factory.newManagedInstanceForTests(); - MyEntity entity = app.addChild(EntitySpec.create(MyEntity.class)); - - assertEquals(entity.getConfigureCount(), 1); - assertEquals(entity.getConfigureDuringConstructionCount(), 0); - } - @Test public void testLegacyConstructionSetsDefaultDisplayName() throws Exception { app = new TestApplicationImpl(); @@ -140,21 +128,4 @@ public void testLegacyConstructionUsesCustomDisplayName() throws Exception { assertEquals(entity.getDisplayName(), "entityname"); assertEquals(entity2.getDisplayName(), "entityname2"); } - - @Test - public void testNewStyleSetsDefaultDisplayName() throws Exception { - app = TestApplication.Factory.newManagedInstanceForTests(); - MyEntity entity = app.addChild(EntitySpec.create(MyEntity.class)); - - assertTrue(entity.getDisplayName().startsWith("MyEntity:"+entity.getId().substring(0,4)), "displayName="+entity.getDisplayName()); - } - - @Test - public void testNewStyleUsesCustomDisplayName() throws Exception { - app = ApplicationBuilder.newManagedApp(EntitySpec.create(TestApplication.class).displayName("appname"), LocalManagementContextForTests.newInstance()); - MyEntity entity = app.addChild(EntitySpec.create(MyEntity.class).displayName("entityname")); - - assertEquals(app.getDisplayName(), "appname"); - assertEquals(entity.getDisplayName(), "entityname"); - } } diff --git a/core/src/test/java/brooklyn/entity/basic/EntitySpecTest.java b/core/src/test/java/brooklyn/entity/basic/EntitySpecTest.java index 0a86e3121f..f80ab04bd4 100644 --- a/core/src/test/java/brooklyn/entity/basic/EntitySpecTest.java +++ b/core/src/test/java/brooklyn/entity/basic/EntitySpecTest.java @@ -45,8 +45,6 @@ public class EntitySpecTest extends BrooklynAppUnitTestSupport { - private static final int TIMEOUT_MS = 10*1000; - private SimulatedLocation loc; private TestEntity entity; @@ -146,6 +144,28 @@ public void testAddsGroups() throws Exception { Asserts.assertEqualsIgnoringOrder(entity.getGroups(), ImmutableSet.of(group)); } + @Test + public void testCallsConfigureAfterConstruction() throws Exception { + AbstractEntityLegacyTest.MyEntity entity = app.createAndManageChild(EntitySpec.create(AbstractEntityLegacyTest.MyEntity.class)); + + assertEquals(entity.getConfigureCount(), 1); + assertEquals(entity.getConfigureDuringConstructionCount(), 0); + } + + @Test + public void testSetsDefaultDisplayName() throws Exception { + TestEntity entity = app.addChild(EntitySpec.create(TestEntity.class)); + + assertTrue(entity.getDisplayName().startsWith("TestEntity:"+entity.getId().substring(0,4)), "displayName="+entity.getDisplayName()); + } + + @Test + public void testUsesCustomDisplayName() throws Exception { + TestEntity entity = app.createAndManageChild(EntitySpec.create(TestEntity.class).displayName("entityname")); + + assertEquals(entity.getDisplayName(), "entityname"); + } + public static class MyPolicy extends AbstractPolicy { public static final BasicConfigKey CONF1 = new BasicConfigKey(String.class, "testpolicy.conf1", "my descr, conf1", "defaultval1"); public static final BasicConfigKey CONF2 = new BasicConfigKey(Integer.class, "testpolicy.conf2", "my descr, conf2", 2); From 85f547e96d20d33aa36a3713c4d9b0ee3809a135 Mon Sep 17 00:00:00 2001 From: Aled Sage Date: Fri, 5 Sep 2014 20:14:02 +0100 Subject: [PATCH 6/7] Simplify HighAvailabilityManagerSplitBrainTest --- .../HighAvailabilityManagerSplitBrainTest.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/core/src/test/java/brooklyn/management/ha/HighAvailabilityManagerSplitBrainTest.java b/core/src/test/java/brooklyn/management/ha/HighAvailabilityManagerSplitBrainTest.java index a11e57fdb7..f83816e55a 100644 --- a/core/src/test/java/brooklyn/management/ha/HighAvailabilityManagerSplitBrainTest.java +++ b/core/src/test/java/brooklyn/management/ha/HighAvailabilityManagerSplitBrainTest.java @@ -23,12 +23,10 @@ import java.util.Date; import java.util.List; import java.util.Map; -import java.util.concurrent.Callable; import java.util.concurrent.atomic.AtomicLong; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import org.testng.Assert; import org.testng.annotations.AfterMethod; import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; @@ -44,12 +42,12 @@ import brooklyn.entity.rebind.persister.PersistenceObjectStore; import brooklyn.location.Location; import brooklyn.management.internal.ManagementContextInternal; +import brooklyn.test.Asserts; import brooklyn.test.entity.LocalManagementContextForTests; import brooklyn.test.entity.TestApplication; import brooklyn.util.collections.MutableList; import brooklyn.util.collections.MutableMap; import brooklyn.util.exceptions.Exceptions; -import brooklyn.util.repeat.Repeater; import brooklyn.util.time.Duration; import brooklyn.util.time.Time; @@ -313,8 +311,8 @@ protected void doTestConcurrentStartup(int size, final Duration staggerStart) th try { final Stopwatch timer = Stopwatch.createStarted(); - Assert.assertTrue(Repeater.create().backoff(Duration.millis(1), 1.2, Duration.millis(50)).limitTimeTo(Duration.THIRTY_SECONDS).until(new Callable() { - @Override public Boolean call() throws Exception { + Asserts.succeedsEventually(new Runnable() { + @Override public void run() { ManagementPlaneSyncRecord memento = nodes.get(0).ha.getManagementPlaneSyncState(); int masters=0, standbys=0, savedMasters=0, savedStandbys=0; for (HaMgmtNode n: nodes) { @@ -334,9 +332,11 @@ protected void doTestConcurrentStartup(int size, final Duration staggerStart) th log.warn("we seem to have a problem stabilizing"); //handy place to set a suspend-VM breakpoint! timer.stop(); } - return masters==1 && standbys==nodes.size()-1 && savedMasters==1 && savedStandbys==nodes.size()-1; - } - }).run()); + assertEquals(masters, 1); + assertEquals(standbys, nodes.size()-1); + assertEquals(savedMasters, 1); + assertEquals(savedStandbys, nodes.size()-1); + }}); } catch (Throwable t) { log.warn("Failed to stabilize (rethrowing): "+t, t); throw Exceptions.propagate(t); From b4930769930d8dbfef39e241f29e32f5efe7c3d3 Mon Sep 17 00:00:00 2001 From: Aled Sage Date: Fri, 5 Sep 2014 20:17:36 +0100 Subject: [PATCH 7/7] CatalogLibrariesDo: minor tidy --- .../java/brooklyn/catalog/internal/CatalogLibrariesDo.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/brooklyn/catalog/internal/CatalogLibrariesDo.java b/core/src/main/java/brooklyn/catalog/internal/CatalogLibrariesDo.java index 2a9fdbc3bd..d967e3b172 100644 --- a/core/src/main/java/brooklyn/catalog/internal/CatalogLibrariesDo.java +++ b/core/src/main/java/brooklyn/catalog/internal/CatalogLibrariesDo.java @@ -59,15 +59,14 @@ void load(ManagementContext managementContext) { Maybe osgi = mgmt.getOsgiManager(); if (osgi.isAbsent()) { throw new IllegalStateException("Unable to load bundles "+bundles+" because OSGi is not running."); - } else if (LOG.isDebugEnabled()) { - LOG.debug("{} loading bundles in {}: {}", - new Object[]{this, managementContext, Joiner.on(", ").join(bundles)}); } + if (LOG.isDebugEnabled()) LOG.debug("{} loading bundles in {}: {}", + new Object[] {this, managementContext, Joiner.on(", ").join(bundles)}); Stopwatch timer = Stopwatch.createStarted(); for (String bundleUrl : bundles) { osgi.get().registerBundle(bundleUrl); } - LOG.debug("{} registered {} bundles in {}", + if (LOG.isDebugEnabled()) LOG.debug("{} registered {} bundles in {}", new Object[]{this, bundles.size(), Time.makeTimeStringRounded(timer)}); } }