address svet code review comments
Project: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/commit/225f1820 Tree: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/tree/225f1820 Diff: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/diff/225f1820 Branch: refs/heads/master Commit: 225f182078b700ab6282a3aac000d3d304870bdd Parents: 6a8af41 Author: Alex Heneveld <[email protected]> Authored: Fri Aug 29 18:35:08 2014 -0400 Committer: Alex Heneveld <[email protected]> Committed: Fri Aug 29 18:35:08 2014 -0400 ---------------------------------------------------------------------- .../brooklyn/enricher/basic/AbstractAggregator.java | 2 +- .../basic/AbstractMultipleSensorAggregator.java | 11 ++++++++--- .../main/java/brooklyn/enricher/basic/Aggregator.java | 12 ++++++++---- .../main/java/brooklyn/entity/basic/AbstractEntity.java | 2 +- core/src/main/java/brooklyn/entity/basic/Lifecycle.java | 7 ------- .../java/brooklyn/entity/basic/ServiceStateLogic.java | 2 +- .../policy/loadbalancing/LoadBalancingPolicyTest.java | 7 +++---- .../java/brooklyn/util/collections/MutableList.java | 8 ++++++++ .../main/java/brooklyn/util/collections/MutableMap.java | 5 +++++ .../main/java/brooklyn/util/collections/MutableSet.java | 6 ++++++ .../src/main/java/brooklyn/util/guava/IfFunctions.java | 6 +++--- .../test/java/brooklyn/util/guava/IfFunctionsTest.java | 2 +- 12 files changed, 45 insertions(+), 25 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/225f1820/core/src/main/java/brooklyn/enricher/basic/AbstractAggregator.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/enricher/basic/AbstractAggregator.java b/core/src/main/java/brooklyn/enricher/basic/AbstractAggregator.java index ba7762c..9568332 100644 --- a/core/src/main/java/brooklyn/enricher/basic/AbstractAggregator.java +++ b/core/src/main/java/brooklyn/enricher/basic/AbstractAggregator.java @@ -85,7 +85,7 @@ public abstract class AbstractAggregator<T,U> extends AbstractEnricher implement if (fromHardcodedProducers == null && producer == null) producer = entity; checkState(fromHardcodedProducers != null ^ producer != null, "must specify one of %s (%s) or %s (%s)", PRODUCER.getName(), producer, FROM_HARDCODED_PRODUCERS.getName(), fromHardcodedProducers); - checkState(producer != null ? (Boolean.TRUE.equals(fromMembers) || Boolean.TRUE.equals(fromChildren)) : true, + checkState(producer == null || Boolean.TRUE.equals(fromMembers) || Boolean.TRUE.equals(fromChildren), "when specifying producer, must specify at least one of fromMembers (%s) or fromChildren (%s)", fromMembers, fromChildren); if (fromHardcodedProducers != null) { http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/225f1820/core/src/main/java/brooklyn/enricher/basic/AbstractMultipleSensorAggregator.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/enricher/basic/AbstractMultipleSensorAggregator.java b/core/src/main/java/brooklyn/enricher/basic/AbstractMultipleSensorAggregator.java index 85c36d8..07b0477 100644 --- a/core/src/main/java/brooklyn/enricher/basic/AbstractMultipleSensorAggregator.java +++ b/core/src/main/java/brooklyn/enricher/basic/AbstractMultipleSensorAggregator.java @@ -55,6 +55,7 @@ public abstract class AbstractMultipleSensorAggregator<U> extends AbstractAggreg Preconditions.checkNotNull(getSourceSensors(), "sourceSensors must be set"); } + @Override protected void setEntityBeforeSubscribingProducerChildrenEvents() { if (LOG.isDebugEnabled()) LOG.debug("{} subscribing to children of {}", new Object[] {this, producer }); for (Sensor<?> sourceSensor: getSourceSensors()) { @@ -62,6 +63,7 @@ public abstract class AbstractMultipleSensorAggregator<U> extends AbstractAggreg } } + @Override protected void addProducerHardcoded(Entity producer) { for (Sensor<?> sourceSensor: getSourceSensors()) { subscribe(producer, sourceSensor, this); @@ -69,17 +71,18 @@ public abstract class AbstractMultipleSensorAggregator<U> extends AbstractAggreg onProducerAdded(producer); } + @Override protected void addProducerChild(Entity producer) { - // not required due to subscribeToChildren call -// subscribe(producer, sourceSensor, this); + // no `subscribe` call needed here, due to previous subscribeToChildren call onProducerAdded(producer); } + @Override protected void addProducerMember(Entity producer) { addProducerHardcoded(producer); } - + @Override protected void onProducerAdded(Entity producer) { if (LOG.isDebugEnabled()) LOG.debug("{} listening to {}", new Object[] {this, producer}); synchronized (values) { @@ -106,6 +109,7 @@ public abstract class AbstractMultipleSensorAggregator<U> extends AbstractAggreg } } + @Override protected void onProducerRemoved(Entity producer) { synchronized (values) { for (Sensor<?> sensor: getSourceSensors()) { @@ -140,5 +144,6 @@ public abstract class AbstractMultipleSensorAggregator<U> extends AbstractAggreg } } + @Override protected abstract Object compute(); } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/225f1820/core/src/main/java/brooklyn/enricher/basic/Aggregator.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/enricher/basic/Aggregator.java b/core/src/main/java/brooklyn/enricher/basic/Aggregator.java index 034a604..e865799 100644 --- a/core/src/main/java/brooklyn/enricher/basic/Aggregator.java +++ b/core/src/main/java/brooklyn/enricher/basic/Aggregator.java @@ -70,29 +70,31 @@ public class Aggregator<T,U> extends AbstractAggregator<T,U> implements SensorEv this.transformation = (Function<? super Collection<T>, ? extends U>) getRequiredConfig(TRANSFORMATION); } - + @Override protected void setEntityBeforeSubscribingProducerChildrenEvents() { if (LOG.isDebugEnabled()) LOG.debug("{} subscribing to children of {}", new Object[] {this, producer }); subscribeToChildren(producer, sourceSensor, this); } + @Override protected void addProducerHardcoded(Entity producer) { subscribe(producer, sourceSensor, this); onProducerAdded(producer); } + @Override protected void addProducerChild(Entity producer) { - // not required due to subscribeToChildren call -// subscribe(producer, sourceSensor, this); + // no subscription needed here, due to the subscribeToChildren call onProducerAdded(producer); } + @Override protected void addProducerMember(Entity producer) { subscribe(producer, sourceSensor, this); onProducerAdded(producer); } - + @Override protected void onProducerAdded(Entity producer) { if (LOG.isDebugEnabled()) LOG.debug("{} listening to {}", new Object[] {this, producer}); synchronized (values) { @@ -115,6 +117,7 @@ public class Aggregator<T,U> extends AbstractAggregator<T,U> implements SensorEv } } + @Override protected void onProducerRemoved(Entity producer) { values.remove(producer); onUpdated(); @@ -142,6 +145,7 @@ public class Aggregator<T,U> extends AbstractAggregator<T,U> implements SensorEv } } + @Override protected Object compute() { synchronized (values) { // TODO Could avoid copying when filter not needed http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/225f1820/core/src/main/java/brooklyn/entity/basic/AbstractEntity.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/entity/basic/AbstractEntity.java b/core/src/main/java/brooklyn/entity/basic/AbstractEntity.java index 5e21fd7..ec2d5e9 100644 --- a/core/src/main/java/brooklyn/entity/basic/AbstractEntity.java +++ b/core/src/main/java/brooklyn/entity/basic/AbstractEntity.java @@ -1058,7 +1058,7 @@ public abstract class AbstractEntity extends AbstractBrooklynObject implements E // these enrichers do nothing unless Attributes.SERVICE_NOT_UP_INDICATORS are used // and/or SERVICE_STATE_EXPECTED protected void initEnrichers() { - addEnricher(ServiceNotUpLogic.newEnricherForServiceUpIfNoNotUpIndicators()); + addEnricher(ServiceNotUpLogic.newEnricherForServiceUpIfNotUpIndicatorsEmpty()); addEnricher(ServiceStateLogic.newEnricherForServiceStateFromProblemsAndUp()); } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/225f1820/core/src/main/java/brooklyn/entity/basic/Lifecycle.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/entity/basic/Lifecycle.java b/core/src/main/java/brooklyn/entity/basic/Lifecycle.java index 5e7d0fe..26d3916 100644 --- a/core/src/main/java/brooklyn/entity/basic/Lifecycle.java +++ b/core/src/main/java/brooklyn/entity/basic/Lifecycle.java @@ -46,15 +46,8 @@ public enum Lifecycle { * When this completes the entity will normally transition to * {@link Lifecycle#RUNNING}. */ -// * {@link Lifecycle#STARTED} or STARTING, -// /** -// * The entity has been started and no further start-up steps are needed from the management plane, -// * but the entity has not yet been confirmed as running. -// */ -// STARTED, -// /** * The entity service is expected to be running. In healthy operation, {@link Attributes#SERVICE_UP} will be true, * or will shortly be true if all service start actions have been completed and we are merely waiting for it to be running. http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/225f1820/core/src/main/java/brooklyn/entity/basic/ServiceStateLogic.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/entity/basic/ServiceStateLogic.java b/core/src/main/java/brooklyn/entity/basic/ServiceStateLogic.java index 5154bff..95d5e57 100644 --- a/core/src/main/java/brooklyn/entity/basic/ServiceStateLogic.java +++ b/core/src/main/java/brooklyn/entity/basic/ServiceStateLogic.java @@ -136,7 +136,7 @@ public class ServiceStateLogic { private ServiceNotUpLogic() {} @SuppressWarnings({ "unchecked", "rawtypes" }) - public static final EnricherSpec<?> newEnricherForServiceUpIfNoNotUpIndicators() { + public static final EnricherSpec<?> newEnricherForServiceUpIfNotUpIndicatorsEmpty() { return Enrichers.builder() .transforming(SERVICE_NOT_UP_INDICATORS).publishing(Attributes.SERVICE_UP) .computing( /* cast hacks to support removing */ (Function) http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/225f1820/policy/src/test/java/brooklyn/policy/loadbalancing/LoadBalancingPolicyTest.java ---------------------------------------------------------------------- diff --git a/policy/src/test/java/brooklyn/policy/loadbalancing/LoadBalancingPolicyTest.java b/policy/src/test/java/brooklyn/policy/loadbalancing/LoadBalancingPolicyTest.java index c0ff7b7..e0a720b 100644 --- a/policy/src/test/java/brooklyn/policy/loadbalancing/LoadBalancingPolicyTest.java +++ b/policy/src/test/java/brooklyn/policy/loadbalancing/LoadBalancingPolicyTest.java @@ -27,8 +27,6 @@ import brooklyn.entity.basic.Entities; import brooklyn.entity.basic.EntityLocal; import brooklyn.test.Asserts; import brooklyn.util.collections.MutableMap; -import brooklyn.util.time.Duration; -import brooklyn.util.time.Time; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -235,7 +233,7 @@ public class LoadBalancingPolicyTest extends AbstractLoadBalancingPolicyTest { ImmutableList.of(item1, item2, item3, item4, item5, item6), ImmutableList.of(30d, 30d)); - MockItemEntity item7 = newItem(app, containerA, "7", 40); + newItem(app, containerA, "7", 40); assertWorkratesEventually( ImmutableList.of(containerA, containerB), @@ -305,6 +303,7 @@ public class LoadBalancingPolicyTest extends AbstractLoadBalancingPolicyTest { ImmutableList.of(40d, 40d)); } + @SuppressWarnings({ "unchecked", "rawtypes" }) @Test public void testModelIncludesItemsAndContainersStartedBeforePolicyCreated() { pool.removePolicy(policy); @@ -312,7 +311,7 @@ public class LoadBalancingPolicyTest extends AbstractLoadBalancingPolicyTest { // Set-up containers and items. final MockContainerEntity containerA = newContainer(app, "A", 10, 100); - MockItemEntity item1 = newItem(app, containerA, "1", 10); + newItem(app, containerA, "1", 10); policy = new LoadBalancingPolicy(MutableMap.of(), TEST_METRIC, model); pool.addPolicy(policy); http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/225f1820/utils/common/src/main/java/brooklyn/util/collections/MutableList.java ---------------------------------------------------------------------- diff --git a/utils/common/src/main/java/brooklyn/util/collections/MutableList.java b/utils/common/src/main/java/brooklyn/util/collections/MutableList.java index a6638da..1c1b2d2 100644 --- a/utils/common/src/main/java/brooklyn/util/collections/MutableList.java +++ b/utils/common/src/main/java/brooklyn/util/collections/MutableList.java @@ -29,6 +29,8 @@ import javax.annotation.Nullable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import brooklyn.util.exceptions.Exceptions; + import com.google.common.collect.ImmutableList; public class MutableList<V> extends ArrayList<V> { @@ -84,17 +86,23 @@ public class MutableList<V> extends ArrayList<V> { public ImmutableList<V> toImmutable() { return ImmutableList.copyOf(this); } + /** creates an {@link ImmutableList} which is a copy of this list. note that the list should not contain nulls. */ public List<V> asImmutableCopy() { try { return ImmutableList.copyOf(this); } catch (Exception e) { + Exceptions.propagateIfFatal(e); log.warn("Error converting list to Immutable, using unmodifiable instead: "+e, e); return asUnmodifiableCopy(); } } + /** creates a {@link Collections#unmodifiableList(List)} wrapper around this list. the method is efficient, + * as there is no copying, but the returned view might change if the list here is changed. */ public List<V> asUnmodifiable() { return Collections.unmodifiableList(this); } + /** creates a {@link Collections#unmodifiableList(List)} of a copy of this list. + * the returned item is immutable, but unlike {@link #asImmutableCopy()} nulls are permitted. */ public List<V> asUnmodifiableCopy() { return Collections.unmodifiableList(MutableList.copyOf(this)); } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/225f1820/utils/common/src/main/java/brooklyn/util/collections/MutableMap.java ---------------------------------------------------------------------- diff --git a/utils/common/src/main/java/brooklyn/util/collections/MutableMap.java b/utils/common/src/main/java/brooklyn/util/collections/MutableMap.java index 2630dbf..f8b2f6c 100644 --- a/utils/common/src/main/java/brooklyn/util/collections/MutableMap.java +++ b/utils/common/src/main/java/brooklyn/util/collections/MutableMap.java @@ -29,6 +29,7 @@ import javax.annotation.Nullable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import brooklyn.util.exceptions.Exceptions; import brooklyn.util.guava.Maybe; import com.google.common.base.Predicate; @@ -146,17 +147,21 @@ public class MutableMap<K,V> extends LinkedHashMap<K,V> { public ImmutableMap<K,V> toImmutable() { return ImmutableMap.copyOf(this); } + /** as {@link MutableList#asImmutableCopy()} */ public Map<K,V> asImmutableCopy() { try { return ImmutableMap.copyOf(this); } catch (Exception e) { + Exceptions.propagateIfFatal(e); log.warn("Error converting list to Immutable, using unmodifiable instead: "+e, e); return asUnmodifiableCopy(); } } + /** as {@link MutableList#asUnmodifiable()} */ public Map<K,V> asUnmodifiable() { return Collections.unmodifiableMap(this); } + /** as {@link MutableList#asUnmodifiableCopy()} */ public Map<K,V> asUnmodifiableCopy() { return Collections.unmodifiableMap(MutableMap.copyOf(this)); } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/225f1820/utils/common/src/main/java/brooklyn/util/collections/MutableSet.java ---------------------------------------------------------------------- diff --git a/utils/common/src/main/java/brooklyn/util/collections/MutableSet.java b/utils/common/src/main/java/brooklyn/util/collections/MutableSet.java index 0653cfd..e795158 100644 --- a/utils/common/src/main/java/brooklyn/util/collections/MutableSet.java +++ b/utils/common/src/main/java/brooklyn/util/collections/MutableSet.java @@ -29,6 +29,8 @@ import javax.annotation.Nullable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import brooklyn.util.exceptions.Exceptions; + import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; @@ -79,17 +81,21 @@ public class MutableSet<V> extends LinkedHashSet<V> { // Don't use ImmutableSet as that does not accept nulls return Collections.unmodifiableSet(Sets.newLinkedHashSet(this)); } + /** as {@link MutableList#asImmutableCopy()()} */ public Set<V> asImmutableCopy() { try { return ImmutableSet.copyOf(this); } catch (Exception e) { + Exceptions.propagateIfFatal(e); log.warn("Error converting list to Immutable, using unmodifiable instead: "+e, e); return asUnmodifiableCopy(); } } + /** as {@link MutableList#asUnmodifiable()} */ public Set<V> asUnmodifiable() { return Collections.unmodifiableSet(this); } + /** as {@link MutableList#asUnmodifiableCopy()} */ public Set<V> asUnmodifiableCopy() { return Collections.unmodifiableSet(MutableSet.copyOf(this)); } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/225f1820/utils/common/src/main/java/brooklyn/util/guava/IfFunctions.java ---------------------------------------------------------------------- diff --git a/utils/common/src/main/java/brooklyn/util/guava/IfFunctions.java b/utils/common/src/main/java/brooklyn/util/guava/IfFunctions.java index 5384436..ceea8c3 100644 --- a/utils/common/src/main/java/brooklyn/util/guava/IfFunctions.java +++ b/utils/common/src/main/java/brooklyn/util/guava/IfFunctions.java @@ -38,13 +38,13 @@ public class IfFunctions { return new IfFunctionBuilder<I,O>(); } - public static <I,O> IfFunctionBuilderApplyingFirst<I> ifPredicate(Predicate<? super I> test) { + public static <I> IfFunctionBuilderApplyingFirst<I> ifPredicate(Predicate<? super I> test) { return new IfFunctionBuilderApplyingFirst<I>(test); } - public static <I,O> IfFunctionBuilderApplyingFirst<I> ifEquals(I test) { + public static <I> IfFunctionBuilderApplyingFirst<I> ifEquals(I test) { return ifPredicate(Predicates.equalTo(test)); } - public static <I,O> IfFunctionBuilderApplyingFirst<I> ifNotEquals(I test) { + public static <I> IfFunctionBuilderApplyingFirst<I> ifNotEquals(I test) { return ifPredicate(Predicates.not(Predicates.equalTo(test))); } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/225f1820/utils/common/src/test/java/brooklyn/util/guava/IfFunctionsTest.java ---------------------------------------------------------------------- diff --git a/utils/common/src/test/java/brooklyn/util/guava/IfFunctionsTest.java b/utils/common/src/test/java/brooklyn/util/guava/IfFunctionsTest.java index 74a5d5b..e52dc34 100644 --- a/utils/common/src/test/java/brooklyn/util/guava/IfFunctionsTest.java +++ b/utils/common/src/test/java/brooklyn/util/guava/IfFunctionsTest.java @@ -67,7 +67,7 @@ public class IfFunctionsTest { @Test public void testWithCast() { - Function<Boolean, String> f = IfFunctions.<Boolean,String>ifEquals(false).value("F").ifEquals(true).value("T").defaultValue("?").build(); + Function<Boolean, String> f = IfFunctions.ifEquals(false).value("F").ifEquals(true).value("T").defaultValue("?").build(); checkTF(f, "?"); }
