Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/480#discussion_r101849614
--- Diff:
core/src/test/java/org/apache/brooklyn/core/entity/EntityConfigTest.java ---
@@ -244,59 +252,152 @@ public void
testGetConfigMapWithSubValueAsStringNotCoerced() throws Exception {
// of the previous "test.confMapThing.obj".
//
// Presumably an earlier call to task.get() timed out, causing it to
cancel the task?
+ // Alex: yes, a task.cancel is performed for maps in
+ //
AbstractEntity$BasicConfigurationSupport(AbstractConfigurationSupportInternal).getNonBlockingResolvingStructuredKey(ConfigKey<T>)
+
+ //
// I (Aled) question whether we want to support passing a task (rather
than a
// DeferredSupplier or TaskFactory, for example). Our
EntitySpec.configure is overloaded
// to take a Task, but that feels wrong!?
- @Test(groups="Broken")
- public void testGetTaskNonBlocking() throws Exception {
- final CountDownLatch latch = new CountDownLatch(1);
- Task<String> task = Tasks.<String>builder().body(
+ //
+ // If starting clean I (Alex) would agree, we should use TaskFactory.
However the
+ // DependentConfiguration methods -- including the ubiquitous
AttributeWhenReady --
+ // return Task instances so they should survive a getNonBlocking or
get with a short timeout
+ // access, and if a value is subsequently available it should be
returned
+ // (which this test asserts, but is currently failing). If TaskFactory
is used the
+ // intended semantics are clear -- you create a new task on each
access, and can interrupt it
+ // and discard it if needed. For a Task it's less clear: probably the
semantics are that the
+ // first returned value is what the value is forevermore. Probably it
should not be interrupted
+ // on a non-blocking / short-wait access, or possibly it should simply
be re-run if a previous
+ // execution was interrupted (but take care if we have a simultaneous
non-blocking and blocking
+ // access, if the first one interrupts the second one should still get
a value).
+ // I tend to think ideally we should switch to using TaskFactory in
DependentConfiguration.
+ class ConfigNonBlockingFixture {
+ final Semaphore latch = new Semaphore(0);
+ final String expectedVal = "myval";
+ Object blockingVal;
+
+ protected ConfigNonBlockingFixture usingTask() {
+ blockingVal = taskFactory().newTask();
+ return this;
+ }
+
+ protected ConfigNonBlockingFixture usingTaskFactory() {
+ blockingVal = taskFactory();
+ return this;
+ }
+
+ protected ConfigNonBlockingFixture usingDeferredSupplier() {
+ blockingVal = deferredSupplier();
+ return this;
+ }
+
+ protected ConfigNonBlockingFixture usingImmediateSupplier() {
+ blockingVal = new
InterruptingImmediateSupplier<String>(deferredSupplier());
+ return this;
+ }
+
+ private TaskFactory<Task<String>> taskFactory() {
+ return Tasks.<String>builder().body(
new Callable<String>() {
@Override
public String call() throws Exception {
- latch.await();
+ if (!latch.tryAcquire()) latch.acquire();
+ latch.release();
return "myval";
}})
- .build();
- runGetConfigNonBlocking(latch, task, "myval");
- }
-
- @Test
- public void testGetDeferredSupplierNonBlocking() throws Exception {
- final CountDownLatch latch = new CountDownLatch(1);
- DeferredSupplier<String> task = new DeferredSupplier<String>() {
- @Override public String get() {
- try {
- latch.await();
- } catch (InterruptedException e) {
- throw Exceptions.propagate(e);
- }
- return "myval";
- }
- };
- runGetConfigNonBlocking(latch, task, "myval");
- }
-
- @SuppressWarnings({"unchecked", "rawtypes"})
- protected void runGetConfigNonBlocking(CountDownLatch latch, Object
blockingVal, String expectedVal) throws Exception {
- TestEntity entity = (TestEntity)
mgmt.getEntityManager().createEntity(EntitySpec.create(TestEntity.class)
- .configure(TestEntity.CONF_MAP_OBJ_THING,
ImmutableMap.<String, Object>of("mysub", blockingVal))
- .configure((ConfigKey)TestEntity.CONF_NAME, blockingVal));
-
- // Will initially return absent, because task is not done
-
assertTrue(entity.config().getNonBlocking(TestEntity.CONF_MAP_OBJ_THING).isAbsent());
-
assertTrue(entity.config().getNonBlocking(TestEntity.CONF_MAP_OBJ_THING.subKey("mysub")).isAbsent());
+ .buildFactory();
+ }
- latch.countDown();
+ private DeferredSupplier<String> deferredSupplier() {
+ return new DeferredSupplier<String>() {
+ @Override public String get() {
+ try {
+ log.trace("acquiring");
+ if (!latch.tryAcquire()) latch.acquire();
+ latch.release();
+ log.trace("acquired and released");
+ } catch (InterruptedException e) {
+ log.trace("interrupted");
+ throw Exceptions.propagate(e);
+ }
+ return "myval";
+ }
+ };
+ }
- // Can now finish task, so will return expectedVal
- assertEquals(entity.config().get(TestEntity.CONF_MAP_OBJ_THING),
ImmutableMap.of("mysub", expectedVal));
-
assertEquals(entity.config().get(TestEntity.CONF_MAP_OBJ_THING.subKey("mysub")),
expectedVal);
+ protected void runGetConfigNonBlockingInKey() throws Exception {
+ Preconditions.checkNotNull(blockingVal, "Fixture must set
blocking val before running this");
+
+ @SuppressWarnings("unchecked")
+ TestEntity entity = (TestEntity)
mgmt.getEntityManager().createEntity(EntitySpec.create(TestEntity.class)
+
.configure((ConfigKey<Object>)(ConfigKey<?>)TestEntity.CONF_NAME, blockingVal));
+
+ log.trace("get non-blocking");
+ // Will initially return absent, because task is not done
+
assertTrue(entity.config().getNonBlocking(TestEntity.CONF_NAME).isAbsent());
+ log.trace("got absent");
+
+ latch.release();
+
+ // Can now finish task, so will return expectedVal
+ log.trace("get blocking");
+ assertEquals(entity.config().get(TestEntity.CONF_NAME),
expectedVal);
+ log.trace("got blocking");
+
assertEquals(entity.config().getNonBlocking(TestEntity.CONF_NAME).get(),
expectedVal);
+
+ latch.acquire();
+ log.trace("finished");
+ }
-
assertEquals(entity.config().getNonBlocking(TestEntity.CONF_MAP_OBJ_THING).get(),
ImmutableMap.of("mysub", expectedVal));
-
assertEquals(entity.config().getNonBlocking(TestEntity.CONF_MAP_OBJ_THING.subKey("mysub")).get(),
expectedVal);
+ protected void runGetConfigNonBlockingInMap() throws Exception {
+ Preconditions.checkNotNull(blockingVal, "Fixture must set
blocking val before running this");
+ TestEntity entity = (TestEntity)
mgmt.getEntityManager().createEntity(EntitySpec.create(TestEntity.class)
+ .configure(TestEntity.CONF_MAP_OBJ_THING,
ImmutableMap.<String, Object>of("mysub", blockingVal)));
+
+ // Will initially return absent, because task is not done
+
assertTrue(entity.config().getNonBlocking(TestEntity.CONF_MAP_OBJ_THING).isAbsent());
+
assertTrue(entity.config().getNonBlocking(TestEntity.CONF_MAP_OBJ_THING.subKey("mysub")).isAbsent());
+
+ latch.release();
+
+ // Can now finish task, so will return expectedVal
+
assertEquals(entity.config().get(TestEntity.CONF_MAP_OBJ_THING),
ImmutableMap.of("mysub", expectedVal));
+
assertEquals(entity.config().get(TestEntity.CONF_MAP_OBJ_THING.subKey("mysub")),
expectedVal);
+
+
assertEquals(entity.config().getNonBlocking(TestEntity.CONF_MAP_OBJ_THING).get(),
ImmutableMap.of("mysub", expectedVal));
+
assertEquals(entity.config().getNonBlocking(TestEntity.CONF_MAP_OBJ_THING.subKey("mysub")).get(),
expectedVal);
+ }
}
+ @Test(groups="Integration") // because takes 1s+
+ public void testGetTaskNonBlockingKey() throws Exception {
+ new
ConfigNonBlockingFixture().usingTask().runGetConfigNonBlockingInKey(); }
--- End diff --
Interesting code layout - think it's the first time I've ever seen a layout
like this. Which would usually be a sign that we should accept a bit more white
space and lay out the code in the convention that is followed in the rest of
our code base!
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
