You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by aledsage <gi...@git.apache.org> on 2016/06/01 19:35:29 UTC
[GitHub] brooklyn-server pull request #174: pr173/rename confMapThing.obj
GitHub user aledsage opened a pull request:
https://github.com/apache/brooklyn-server/pull/174
pr173/rename confMapThing.obj
Part of #173 - please review + merge here, to decrease the size of PR #173 !
The previous name caused problems because there is also a
\u201cconfMapThing\u201d of type MapConfigKey. That has special behaviour,
where it looks up any config with that prefix - so it picked up
any config defined against confMapThing.obj as well.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aledsage/brooklyn-server pr173/rename-confMapObjThing
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/174.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 #174
----
commit fcfcacfe91c41462b7f97a75721669acf596732e
Author: Aled Sage <al...@gmail.com>
Date: 2016-05-26T09:39:52Z
Rename TestEntity confMapThing.obj to confMapObjThing
The previous name caused problems because there is also a
\u201cconfMapThing\u201d of type MapConfigKey. That has special behaviour,
where it looks up any config with that prefix - so it picked up
any config defined against confMapThing.obj as well.
commit 7e66c2099519d59336ff7ae014a892b37e369cc2
Author: Aled Sage <al...@gmail.com>
Date: 2016-06-01T16:57:50Z
Disable EntityConfigTest.testGetConfigNonBlocking
----
---
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] brooklyn-server pull request #174: pr173/rename confMapThing.obj
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/174#discussion_r65519069
--- Diff: core/src/test/java/org/apache/brooklyn/core/entity/EntityConfigTest.java ---
@@ -208,8 +210,18 @@ public void testGetConfigMapWithExplicitMap() throws Exception {
assertEquals(entity.config().getNonBlocking(TestEntity.CONF_MAP_THING.subKey("mysub")).get(), "myval");
}
- @Test
- public void testGetConfigNonBlocking() throws Exception {
+ // TODO This now fails because the task has been cancelled, in entity.config().get().
+ // But it used to pass (e.g. with commit 56fcc1632ea4f5ac7f4136a7e04fabf501337540).
+ // It failed after the rename of CONF_MAP_THING_OBJ to CONF_MAP_OBJ_THING, which
+ // suggests there was an underlying problem that was masked by the unfortunate naming
+ // of the previous "test.confMapThing.obj".
+ //
+ // Presumably an earlier call to task.get() timed out, causing it to cancel the task?
+ // 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!?
--- End diff --
@neykov you're right - we support passing any of Task, Future and DeferredSupplier. I agree we should continue to support those, and should fix whatever is cancelling this task. I think it's probably to do with the implementation of `MapConfigKey`, given the test only broke after the rename (which meant that "obj" couldn't be mistaken for a sub-key of the other MapConfigKey called "test.confMapThing".
I also see that `MapConfigKey.applyEntryValueToMap` does an instanceof check for `Supplier`, but that `ValueResolver` does not - that seems wrong.
---
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] brooklyn-server pull request #174: pr173/rename confMapThing.obj
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/174
---
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] brooklyn-server pull request #174: pr173/rename confMapThing.obj
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/174#discussion_r65498168
--- Diff: core/src/test/java/org/apache/brooklyn/core/entity/EntityConfigTest.java ---
@@ -208,8 +210,18 @@ public void testGetConfigMapWithExplicitMap() throws Exception {
assertEquals(entity.config().getNonBlocking(TestEntity.CONF_MAP_THING.subKey("mysub")).get(), "myval");
}
- @Test
- public void testGetConfigNonBlocking() throws Exception {
+ // TODO This now fails because the task has been cancelled, in entity.config().get().
+ // But it used to pass (e.g. with commit 56fcc1632ea4f5ac7f4136a7e04fabf501337540).
+ // It failed after the rename of CONF_MAP_THING_OBJ to CONF_MAP_OBJ_THING, which
+ // suggests there was an underlying problem that was masked by the unfortunate naming
+ // of the previous "test.confMapThing.obj".
+ //
+ // Presumably an earlier call to task.get() timed out, causing it to cancel the task?
+ // 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!?
--- End diff --
Passing Tasks as config keys makes sense for me (same as passing a future). Creating a task to compute some value and passing it to an entity is a common thing in Brooklyn.
---
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] brooklyn-server issue #174: pr173/rename confMapThing.obj
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/174
LGTM, but tasks shouldn't be cancelled while trying to get their result. I think this needs further investigation.
---
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.
---