Mailing-List: contact dev-help@brooklyn.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@brooklyn.apache.org
From: aledsage <git@git.apache.org>
To: dev@brooklyn.apache.org
Reply-To: dev@brooklyn.apache.org
References: <git-pr-480-brooklyn-server@git.apache.org>
In-Reply-To: <git-pr-480-brooklyn-server@git.apache.org>
Subject: [GitHub] brooklyn-server pull request #480: Config self reference fix
Content-Type: text/plain
Message-Id: <20170217234058.2C31DE0B24@git1-us-west.apache.org>
Date: Fri, 17 Feb 2017 23:40:58 +0000 (UTC)
archived-at: Fri, 17 Feb 2017 23:41:00 -0000

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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---