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/11/04 12:52:36 UTC
[GitHub] brooklyn-server pull request #403: Adds DynamicFabric.includeInitialChildren...
GitHub user aledsage opened a pull request:
https://github.com/apache/brooklyn-server/pull/403
Adds DynamicFabric.includeInitialChildren config
And adds tests for existing behaviour.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aledsage/brooklyn-server dynamicFabric-add-includeInitialChildren
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/403.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 #403
----
commit 00182b50742069dae1c70121991be4d06d7aad2e
Author: Aled Sage <al...@gmail.com>
Date: 2016-11-04T12:50:53Z
Adds DynamicFabric.includeInitialChildren config
And adds tests for existing behaviour.
----
---
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 #403: Adds DynamicFabric.includeInitialChildren...
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/403#discussion_r86957911
--- Diff: core/src/test/java/org/apache/brooklyn/entity/group/DynamicRegionsFabricTest.java ---
@@ -60,16 +64,98 @@ public void setUp() throws Exception {
}
@Test
- public void testUsesInitialLocations() throws Exception {
+ public void testUsesInitialLocationsFromStartEffectorArgs() throws Exception {
app.start(ImmutableList.of(loc1, loc2));
- assertEquals(fabric.getChildren().size(), 2, "children="+fabric.getChildren());
- assertEquals(fabric.getMembers().size(), 2, "members="+fabric.getMembers());
- assertEqualsIgnoringOrder(fabric.getChildren(), fabric.getMembers());
- assertEqualsIgnoringOrder(getLocationsOfChildren(fabric), ImmutableList.of(loc1, loc2));
+ assertFabricChildren(fabric, 2, ImmutableList.of(loc1, loc2));
+ }
+
+ @Test
+ @SuppressWarnings("deprecation")
+ public void testUsesInitialLocationsFromAppSpec() throws Exception {
+ TestApplication app2 = mgmt.getEntityManager().createEntity(EntitySpec.create(TestApplication.class)
+ .configure(BrooklynConfigKeys.SKIP_ON_BOX_BASE_DIR_RESOLUTION, true)
+ .child(EntitySpec.create(DynamicRegionsFabric.class)
+ .configure("memberSpec", EntitySpec.create(TestEntity.class)))
+ .locations(ImmutableList.of(loc1, loc2)));
+ DynamicRegionsFabric fabric2 = (DynamicRegionsFabric) Iterables.getOnlyElement(app2.getChildren());
+ app2.start(ImmutableList.<Location>of());
+
+ assertFabricChildren(fabric2, 2, ImmutableList.of(loc1, loc2));
}
@Test
+ @SuppressWarnings("deprecation")
+ public void testUsesInitialLocationsFromEntitySpec() throws Exception {
--- End diff --
Not quite the same. In `testUsesInitialLocationsFromAppSpec` we set the locations on the app's spec. In `testUsesInitialLocationsFromEntitySpec` we set the locations on the fabric's spec.
Could refactor to remove duplication, but I think that it's probably simpler to have the 7 lines duplicated rather than a delegate method with an if-else block (given setting the location is buried in the 5 lines required to build the spec).
---
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 #403: BROOKLYN-380: Adds DynamicFabric.includeInitialC...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/403
@mikezaccardo thanks, I've incorporated the review comments (see second commit). I've also created a jira ticket to track this, and referred to it in the PR title / commit message.
If jenkins confirms this builds ok, I'll merge.
---
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 #403: Adds DynamicFabric.includeInitialChildren...
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/403#discussion_r86957078
--- Diff: core/src/main/java/org/apache/brooklyn/entity/group/DynamicFabricImpl.java ---
@@ -122,27 +124,50 @@ public void start(Collection<? extends Location> locsO) {
try {
Map<Entity, Task<?>> tasks = Maps.newLinkedHashMap();
- // first look at existing Startable children - start them with the locations passed in here,
- // if they have no locations yet
- for (Entity child: getChildren()) {
- if (child instanceof Startable) {
- addMember(child);
- Location it = null;
- if (child.getLocations().isEmpty())
- // give him any of these locations if he has none, allowing round robin here
- if (!locationsToStart.isEmpty()) {
- it = locationsToStart.get(locIndex++ % locationsToStart.size());
- ((EntityInternal)child).addLocations(Arrays.asList(it));
+ if (includeInitialChildren) {
+ // first look at existing Startable children - start them with the locations passed in here,
+ // if they have no locations yet
+ for (Entity child: getChildren()) {
+ if (child instanceof Startable) {
+ addMember(child);
--- End diff --
Yes - if `includeInitialChildren=false` then these initial children are not considered "members" of the fabric, but are extra things.
---
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 #403: Adds DynamicFabric.includeInitialChildren...
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/403#discussion_r86839669
--- Diff: core/src/main/java/org/apache/brooklyn/entity/group/DynamicFabricImpl.java ---
@@ -122,27 +124,50 @@ public void start(Collection<? extends Location> locsO) {
try {
Map<Entity, Task<?>> tasks = Maps.newLinkedHashMap();
- // first look at existing Startable children - start them with the locations passed in here,
- // if they have no locations yet
- for (Entity child: getChildren()) {
- if (child instanceof Startable) {
- addMember(child);
- Location it = null;
- if (child.getLocations().isEmpty())
- // give him any of these locations if he has none, allowing round robin here
- if (!locationsToStart.isEmpty()) {
- it = locationsToStart.get(locIndex++ % locationsToStart.size());
- ((EntityInternal)child).addLocations(Arrays.asList(it));
+ if (includeInitialChildren) {
--- End diff --
This block could potentially be extracted into a method `startChildren()` which accepts `includeInitialChildren` as a flag. Helps with DRY since the differences between the code blocks are relatively minimal.
---
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 #403: Adds DynamicFabric.includeInitialChildren...
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/403#discussion_r86838197
--- Diff: core/src/main/java/org/apache/brooklyn/entity/group/DynamicFabricImpl.java ---
@@ -122,27 +124,50 @@ public void start(Collection<? extends Location> locsO) {
try {
Map<Entity, Task<?>> tasks = Maps.newLinkedHashMap();
- // first look at existing Startable children - start them with the locations passed in here,
- // if they have no locations yet
- for (Entity child: getChildren()) {
- if (child instanceof Startable) {
- addMember(child);
- Location it = null;
- if (child.getLocations().isEmpty())
- // give him any of these locations if he has none, allowing round robin here
- if (!locationsToStart.isEmpty()) {
- it = locationsToStart.get(locIndex++ % locationsToStart.size());
- ((EntityInternal)child).addLocations(Arrays.asList(it));
+ if (includeInitialChildren) {
+ // first look at existing Startable children - start them with the locations passed in here,
+ // if they have no locations yet
+ for (Entity child: getChildren()) {
+ if (child instanceof Startable) {
+ addMember(child);
--- End diff --
I just want to confirm that this line was intentionally removed from the `else` case below?
---
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 #403: Adds DynamicFabric.includeInitialChildren...
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/403#discussion_r86839055
--- Diff: core/src/main/java/org/apache/brooklyn/entity/group/DynamicFabricImpl.java ---
@@ -122,27 +124,50 @@ public void start(Collection<? extends Location> locsO) {
try {
Map<Entity, Task<?>> tasks = Maps.newLinkedHashMap();
- // first look at existing Startable children - start them with the locations passed in here,
- // if they have no locations yet
- for (Entity child: getChildren()) {
- if (child instanceof Startable) {
- addMember(child);
- Location it = null;
- if (child.getLocations().isEmpty())
- // give him any of these locations if he has none, allowing round robin here
- if (!locationsToStart.isEmpty()) {
- it = locationsToStart.get(locIndex++ % locationsToStart.size());
- ((EntityInternal)child).addLocations(Arrays.asList(it));
+ if (includeInitialChildren) {
+ // first look at existing Startable children - start them with the locations passed in here,
+ // if they have no locations yet
+ for (Entity child: getChildren()) {
+ if (child instanceof Startable) {
+ addMember(child);
--- End diff --
It would appear intentional, as it is the other fundamental difference between the cases (the first being location consumption).
---
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 #403: Adds DynamicFabric.includeInitialChildren config
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on the issue:
https://github.com/apache/brooklyn-server/pull/403
Reviewed, all tests pass
---
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 #403: Adds DynamicFabric.includeInitialChildren...
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/403#discussion_r86846586
--- Diff: core/src/test/java/org/apache/brooklyn/entity/group/DynamicRegionsFabricTest.java ---
@@ -60,16 +64,98 @@ public void setUp() throws Exception {
}
@Test
- public void testUsesInitialLocations() throws Exception {
+ public void testUsesInitialLocationsFromStartEffectorArgs() throws Exception {
app.start(ImmutableList.of(loc1, loc2));
- assertEquals(fabric.getChildren().size(), 2, "children="+fabric.getChildren());
- assertEquals(fabric.getMembers().size(), 2, "members="+fabric.getMembers());
- assertEqualsIgnoringOrder(fabric.getChildren(), fabric.getMembers());
- assertEqualsIgnoringOrder(getLocationsOfChildren(fabric), ImmutableList.of(loc1, loc2));
+ assertFabricChildren(fabric, 2, ImmutableList.of(loc1, loc2));
+ }
+
+ @Test
+ @SuppressWarnings("deprecation")
+ public void testUsesInitialLocationsFromAppSpec() throws Exception {
+ TestApplication app2 = mgmt.getEntityManager().createEntity(EntitySpec.create(TestApplication.class)
+ .configure(BrooklynConfigKeys.SKIP_ON_BOX_BASE_DIR_RESOLUTION, true)
+ .child(EntitySpec.create(DynamicRegionsFabric.class)
+ .configure("memberSpec", EntitySpec.create(TestEntity.class)))
+ .locations(ImmutableList.of(loc1, loc2)));
+ DynamicRegionsFabric fabric2 = (DynamicRegionsFabric) Iterables.getOnlyElement(app2.getChildren());
+ app2.start(ImmutableList.<Location>of());
+
+ assertFabricChildren(fabric2, 2, ImmutableList.of(loc1, loc2));
}
@Test
+ @SuppressWarnings("deprecation")
+ public void testUsesInitialLocationsFromEntitySpec() throws Exception {
--- End diff --
This test is identical to `testUsesInitialLocationsFromAppSpec()`
---
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 #403: BROOKLYN-380: Adds DynamicFabric.includeI...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/403
---
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.
---