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/05/23 20:08:47 UTC
[GitHub] brooklyn-server pull request: BROOKLYN-278: fix Propagator validat...
GitHub user aledsage opened a pull request:
https://github.com/apache/brooklyn-server/pull/154
BROOKLYN-278: fix Propagator validation/defaults
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aledsage/brooklyn-server BROOKLYN-278
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/154.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 #154
----
commit c0fdca6e69b7c49ae6e5948639d651cc6b42c39c
Author: Aled Sage <al...@gmail.com>
Date: 2016-05-23T20:08:08Z
BROOKLYN-278: fix Propagator validation/defaults
----
---
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: BROOKLYN-278: fix Propagator validat...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/brooklyn-server/pull/154#issuecomment-221321690
Thanks @geomacy - comments incorporated, and jenkins confirms build works. Merging.
---
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: BROOKLYN-278: fix Propagator validat...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/brooklyn-server/pull/154#issuecomment-222119849
@aledsage This seems to break `EnrichersSlightlySimplerYamlTest.testWithAppEnricher`:
```
testWithAppEnricher(org.apache.brooklyn.qa.camp.EnrichersSlightlySimplerYamlTest) Time elapsed: 1.235 sec <<< FAILURE!
org.apache.brooklyn.util.exceptions.PropagatedRuntimeException:
at java.util.concurrent.FutureTask.report(FutureTask.java:122)
at java.util.concurrent.FutureTask.get(FutureTask.java:192)
at com.google.common.util.concurrent.ForwardingFuture.get(ForwardingFuture.java:63)
at org.apache.brooklyn.util.core.task.BasicTask.get(BasicTask.java:361)
at org.apache.brooklyn.util.core.task.BasicTask.getUnchecked(BasicTask.java:370)
at org.apache.brooklyn.core.objs.proxy.InternalEntityFactory.initEntityAndDescendants(InternalEntityFactory.java:361)
at org.apache.brooklyn.core.objs.proxy.InternalEntityFactory.createEntity(InternalEntityFactory.java:188)
at org.apache.brooklyn.core.mgmt.internal.LocalEntityManager.createEntity(LocalEntityManager.java:145)
at org.apache.brooklyn.camp.brooklyn.AbstractYamlTest.createAndStartApplication(AbstractYamlTest.java:128)
at org.apache.brooklyn.camp.brooklyn.AbstractYamlTest.createAndStartApplication(AbstractYamlTest.java:123)
at org.apache.brooklyn.camp.brooklyn.AbstractYamlTest.createAndStartApplication(AbstractYamlTest.java:119)
at org.apache.brooklyn.qa.camp.EnrichersSlightlySimplerYamlTest.testWithAppEnricher(EnrichersSlightlySimplerYamlTest.java:57)
Caused by: java.lang.IllegalStateException: Propagator enricher Propagator{running=true, entity=BasicApplicationImpl{id=B4jktQsQ}, id=FH9VxCyW} must have 'propagating' and/or 'sensorMapping', or 'propagatingAll' or 'propagatingAllBut' set
at org.apache.brooklyn.enricher.stock.Propagator.setEntity(Propagator.java:106)
at org.apache.brooklyn.core.entity.AbstractEntity$BasicEnricherSupport.add(AbstractEntity.java:1746)
at org.apache.brooklyn.core.objs.proxy.InternalEntityFactory$1.run(InternalEntityFactory.java:344)
at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
at org.apache.brooklyn.util.core.task.BasicExecutionManager$SubmissionCallable.call(BasicExecutionManager.java:519)
at java.util.concurrent.FutureTask.run(FutureTask.java:266)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
at java.lang.Thread.run(Thread.java:745)
```
Will fix.
---
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: BROOKLYN-278: fix Propagator validat...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/brooklyn-server/pull/154#issuecomment-222120490
Oh, I guess it wasn't caught because the test is in `brooklyn-library`. The split repos make this difficult to track.
---
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: BROOKLYN-278: fix Propagator validat...
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/154#discussion_r64399555
--- Diff: core/src/main/java/org/apache/brooklyn/enricher/stock/Propagator.java ---
@@ -87,24 +93,41 @@ public Propagator() {
public void setEntity(EntityLocal entity) {
super.setEntity(entity);
- this.producer = getConfig(PRODUCER) == null ? entity : getConfig(PRODUCER);
- boolean sensorMappingSet = getConfig(SENSOR_MAPPING)!=null;
- MutableMap<Sensor<?>,Sensor<?>> sensorMappingTemp = MutableMap.copyOf(getConfig(SENSOR_MAPPING));
- this.propagatingAll = Boolean.TRUE.equals(getConfig(PROPAGATING_ALL)) || getConfig(PROPAGATING_ALL_BUT)!=null;
+ this.producer = getConfig(PRODUCER);
+ this.sensorMapping = resolveSensorMappings(getConfig(SENSOR_MAPPING));
+ this.propagatingAllBut = resolveSensorCollection(getConfig(PROPAGATING_ALL_BUT));
+ this.propagatingAll = Boolean.TRUE.equals(getConfig(PROPAGATING_ALL)) || propagatingAllBut.size() > 0;
+ Collection<Sensor<?>> propagating = resolveSensorCollection(getConfig(PROPAGATING));
- if (getConfig(PROPAGATING) != null) {
+ if (producer == null) {
+ throw new IllegalStateException("Propagator enricher "+this+" missing config '"+PRODUCER.getName());
+ }
+ if (propagating.isEmpty() && sensorMapping.isEmpty() && !propagatingAll) {
+ throw new IllegalStateException("Propagator enricher "+this+" must have 'propagating' and/or 'sensorMapping', or 'propagatingAll' or 'propagatingAllBut' set");
+ }
+ if (entity.equals(producer)) {
if (propagatingAll) {
- throw new IllegalStateException("Propagator enricher "+this+" must not have 'propagating' set at same time as either 'propagatingAll' or 'propagatingAllBut'");
+ throw new IllegalStateException("Propagator enricher "+this+" must not have "+PROPAGATING_ALL.getName()+" or "+PROPAGATING_ALL_BUT.getName()+", when publishing to own entity (to avoid infinite loop)");
+ } else if (propagating.size() > 0) {
+ throw new IllegalStateException("Propagator enricher "+this+" must not have "+PROPAGATING.getName()+", when publishing to own entity (to avoid infinite loop)");
+ } else if (filterForKeyEqualsValue(sensorMapping).size() > 0) {
+ Map<? extends Sensor<?>, ? extends Sensor<?>> selfPublishingSensors = filterForKeyEqualsValue(sensorMapping);
+ throw new IllegalStateException("Propagator enricher "+this+" must not publish to same sensor in config "+SENSOR_MAPPING.getName()+" ("+selfPublishingSensors.keySet()+"), when publishing to own entity (to avoid infinite loop)");
}
-
- for (Object sensorO : getConfig(PROPAGATING)) {
- Sensor<?> sensor = Tasks.resolving(sensorO).as(Sensor.class).timeout(ValueResolver.REAL_QUICK_WAIT).context(producer).get();
- if (!sensorMappingTemp.containsKey(sensor)) {
- sensorMappingTemp.put(sensor, sensor);
+ }
+ if ((propagating.size() > 0 || sensorMapping.size() > 0) && propagatingAll) {
+ throw new IllegalStateException("Propagator enricher "+this+" must not have 'propagating' or 'sensorMapping' set at same time as either 'propagatingAll' or 'propagatingAllBut'");
+ }
+
+ if (propagating.size() > 0) {
+ for (Sensor<?> sensor : propagating) {
+ if (!sensorMapping.containsKey(sensor)) {
+ sensorMapping.put(sensor, sensor);
}
}
- this.sensorMapping = ImmutableMap.copyOf(sensorMappingTemp);
- this.sensorFilter = new Predicate<Sensor<?>>() {
+ this.sensorMapping = ImmutableMap.copyOf(sensorMapping);
--- End diff --
@geomacy agree, normally only use `this` when there's scope ambiguity as with most ctors
---
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: BROOKLYN-278: fix Propagator validat...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on the pull request:
https://github.com/apache/brooklyn-server/pull/154#issuecomment-221265849
\U0001f44d Looks good to me, a couple of minor comments but happy for this to be committed as-is. I also pulled and tested it, and can confirm that the example given in [BROOKLYN-278|https://issues.apache.org/jira/browse/BROOKLYN-278] is rejected by this fix.
---
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: BROOKLYN-278: fix Propagator validat...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/154#discussion_r64388028
--- Diff: core/src/main/java/org/apache/brooklyn/enricher/stock/Propagator.java ---
@@ -87,24 +93,41 @@ public Propagator() {
public void setEntity(EntityLocal entity) {
super.setEntity(entity);
- this.producer = getConfig(PRODUCER) == null ? entity : getConfig(PRODUCER);
- boolean sensorMappingSet = getConfig(SENSOR_MAPPING)!=null;
- MutableMap<Sensor<?>,Sensor<?>> sensorMappingTemp = MutableMap.copyOf(getConfig(SENSOR_MAPPING));
- this.propagatingAll = Boolean.TRUE.equals(getConfig(PROPAGATING_ALL)) || getConfig(PROPAGATING_ALL_BUT)!=null;
+ this.producer = getConfig(PRODUCER);
+ this.sensorMapping = resolveSensorMappings(getConfig(SENSOR_MAPPING));
+ this.propagatingAllBut = resolveSensorCollection(getConfig(PROPAGATING_ALL_BUT));
+ this.propagatingAll = Boolean.TRUE.equals(getConfig(PROPAGATING_ALL)) || propagatingAllBut.size() > 0;
+ Collection<Sensor<?>> propagating = resolveSensorCollection(getConfig(PROPAGATING));
- if (getConfig(PROPAGATING) != null) {
+ if (producer == null) {
+ throw new IllegalStateException("Propagator enricher "+this+" missing config '"+PRODUCER.getName());
+ }
+ if (propagating.isEmpty() && sensorMapping.isEmpty() && !propagatingAll) {
+ throw new IllegalStateException("Propagator enricher "+this+" must have 'propagating' and/or 'sensorMapping', or 'propagatingAll' or 'propagatingAllBut' set");
+ }
+ if (entity.equals(producer)) {
if (propagatingAll) {
- throw new IllegalStateException("Propagator enricher "+this+" must not have 'propagating' set at same time as either 'propagatingAll' or 'propagatingAllBut'");
+ throw new IllegalStateException("Propagator enricher "+this+" must not have "+PROPAGATING_ALL.getName()+" or "+PROPAGATING_ALL_BUT.getName()+", when publishing to own entity (to avoid infinite loop)");
+ } else if (propagating.size() > 0) {
+ throw new IllegalStateException("Propagator enricher "+this+" must not have "+PROPAGATING.getName()+", when publishing to own entity (to avoid infinite loop)");
+ } else if (filterForKeyEqualsValue(sensorMapping).size() > 0) {
+ Map<? extends Sensor<?>, ? extends Sensor<?>> selfPublishingSensors = filterForKeyEqualsValue(sensorMapping);
+ throw new IllegalStateException("Propagator enricher "+this+" must not publish to same sensor in config "+SENSOR_MAPPING.getName()+" ("+selfPublishingSensors.keySet()+"), when publishing to own entity (to avoid infinite loop)");
}
-
- for (Object sensorO : getConfig(PROPAGATING)) {
- Sensor<?> sensor = Tasks.resolving(sensorO).as(Sensor.class).timeout(ValueResolver.REAL_QUICK_WAIT).context(producer).get();
- if (!sensorMappingTemp.containsKey(sensor)) {
- sensorMappingTemp.put(sensor, sensor);
+ }
+ if ((propagating.size() > 0 || sensorMapping.size() > 0) && propagatingAll) {
+ throw new IllegalStateException("Propagator enricher "+this+" must not have 'propagating' or 'sensorMapping' set at same time as either 'propagatingAll' or 'propagatingAllBut'");
+ }
+
+ if (propagating.size() > 0) {
+ for (Sensor<?> sensor : propagating) {
+ if (!sensorMapping.containsKey(sensor)) {
+ sensorMapping.put(sensor, sensor);
}
}
- this.sensorMapping = ImmutableMap.copyOf(sensorMappingTemp);
- this.sensorFilter = new Predicate<Sensor<?>>() {
+ this.sensorMapping = ImmutableMap.copyOf(sensorMapping);
--- End diff --
I would get rid of the "this." myself - it's superfluous here and below. In particular on this line and L140 it gives the visual impression that "this.sensorMapping" is different from just "sensorMapping" which is not the case - it's the same variable in both cases.
---
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: BROOKLYN-278: fix Propagator validat...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/154#discussion_r64388088
--- Diff: core/src/main/java/org/apache/brooklyn/enricher/stock/Propagator.java ---
@@ -113,33 +136,15 @@ public void setEntity(EntityLocal entity) {
return true;
}
};
- } else if (sensorMappingSet) {
- if (propagatingAll) {
- throw new IllegalStateException("Propagator enricher "+this+" must not have 'sensorMapping' set at same time as either 'propagatingAll' or 'propagatingAllBut'");
- }
- this.sensorMapping = ImmutableMap.copyOf(sensorMappingTemp);
+ } else if (sensorMapping.size() > 0) {
+ this.sensorMapping = ImmutableMap.copyOf(sensorMapping);
this.sensorFilter = Predicates.alwaysTrue();
} else {
- this.sensorMapping = ImmutableMap.<Sensor<?>, Sensor<?>>of();
- if (!propagatingAll) {
- // default if nothing specified is to do all but the ones not usually propagated
- propagatingAll = true;
- // user specified nothing, so *set* the all_but to the default set
- // if desired, we could allow this to be dynamically reconfigurable, remove this field and always look up;
- // slight performance hit (always looking up), and might need to recompute subscriptions, so not supported currently
- // TODO this default is @Beta behaviour! -- maybe better to throw?
- propagatingAllBut = SENSORS_NOT_USUALLY_PROPAGATED;
- } else {
- propagatingAllBut = getConfig(PROPAGATING_ALL_BUT);
- }
+ assert propagatingAll;
--- End diff --
Could this not use org.apache.brooklyn.test.Asserts or com.google.common.base.Preconditions and add a message?
---
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: BROOKLYN-278: fix Propagator validat...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/154
---
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.
---