addressing comments from code review on enhancing yaml
Project: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/commit/7ba6df70 Tree: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/tree/7ba6df70 Diff: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/diff/7ba6df70 Branch: refs/heads/master Commit: 7ba6df707b7485469c6c0d75cddab46bd849ccb9 Parents: f7142a3 Author: Alex Heneveld <[email protected]> Authored: Wed Apr 15 20:31:19 2015 -0500 Committer: Alex Heneveld <[email protected]> Committed: Wed Apr 15 20:33:14 2015 -0500 ---------------------------------------------------------------------- .../brooklyn/config/internal/AbstractConfigMapImpl.java | 7 ++++--- .../main/java/brooklyn/entity/basic/EntityConfigMap.java | 2 +- core/src/main/java/brooklyn/event/basic/MapConfigKey.java | 10 ++++++++-- .../main/java/brooklyn/policy/basic/ConfigMapImpl.java | 2 +- docs/guide/yaml/yaml-reference.md | 8 ++++++-- 5 files changed, 20 insertions(+), 9 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/7ba6df70/core/src/main/java/brooklyn/config/internal/AbstractConfigMapImpl.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/config/internal/AbstractConfigMapImpl.java b/core/src/main/java/brooklyn/config/internal/AbstractConfigMapImpl.java index 20f5812..defb5bc 100644 --- a/core/src/main/java/brooklyn/config/internal/AbstractConfigMapImpl.java +++ b/core/src/main/java/brooklyn/config/internal/AbstractConfigMapImpl.java @@ -64,7 +64,7 @@ public abstract class AbstractConfigMapImpl implements ConfigMap { return getConfigRaw(key, true).orNull(); } - protected Object setConfigPrep1(ConfigKey<?> key, Object v) { + protected Object coerceConfigVal(ConfigKey<?> key, Object v) { Object val; if ((v instanceof Future) || (v instanceof DeferredSupplier)) { // no coercion for these (coerce on exit) @@ -82,8 +82,9 @@ public abstract class AbstractConfigMapImpl implements ConfigMap { val = TypeCoercions.coerce(v, key.getTypeToken()); } catch (Exception e) { throw new IllegalArgumentException("Cannot coerce or set "+v+" to "+key, e); - // if can't coerce, we could just log, and *throw* the error when we retrieve the config - // but for now, fail fast (above) + // if can't coerce, we could just log as below and *throw* the error when we retrieve the config + // but for now, fail fast (above), because we haven't encountered strong use cases + // where we want to do coercion on retrieval, except for the exceptions above // Exceptions.propagateIfFatal(e); // LOG.warn("Cannot coerce or set "+v+" to "+key+" (ignoring): "+e, e); // val = v; http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/7ba6df70/core/src/main/java/brooklyn/entity/basic/EntityConfigMap.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/entity/basic/EntityConfigMap.java b/core/src/main/java/brooklyn/entity/basic/EntityConfigMap.java index 3efe8fa..f2821d8 100644 --- a/core/src/main/java/brooklyn/entity/basic/EntityConfigMap.java +++ b/core/src/main/java/brooklyn/entity/basic/EntityConfigMap.java @@ -187,7 +187,7 @@ public class EntityConfigMap extends AbstractConfigMapImpl { @SuppressWarnings("unchecked") public Object setConfig(ConfigKey<?> key, Object v) { - Object val = setConfigPrep1(key, v); + Object val = coerceConfigVal(key, v); Object oldVal; if (key instanceof StructuredConfigKey) { oldVal = ((StructuredConfigKey)key).applyValueToMap(val, ownConfig); http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/7ba6df70/core/src/main/java/brooklyn/event/basic/MapConfigKey.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/event/basic/MapConfigKey.java b/core/src/main/java/brooklyn/event/basic/MapConfigKey.java index c682b3b..811aabb 100644 --- a/core/src/main/java/brooklyn/event/basic/MapConfigKey.java +++ b/core/src/main/java/brooklyn/event/basic/MapConfigKey.java @@ -22,6 +22,7 @@ import java.util.Collections; import java.util.LinkedHashMap; import java.util.Map; import java.util.Map.Entry; +import java.util.concurrent.ConcurrentMap; import java.util.concurrent.ExecutionException; import org.slf4j.Logger; @@ -132,15 +133,20 @@ public class MapConfigKey<V> extends AbstractStructuredConfigKey<Map<String,V>,M } else { // supplier or other unexpected value if (k instanceof Supplier) { - // TODO not thread-safe Object mapAtRoot = target.get(this); if (mapAtRoot==null) { mapAtRoot = new LinkedHashMap(); target.put(this, mapAtRoot); } + // TODO above is not thread-safe, and below is assuming synching on map + // is the best way to prevent CME's, which is often but not always true if (mapAtRoot instanceof Map) { - synchronized (mapAtRoot) { + if (mapAtRoot instanceof ConcurrentMap) { return ((Map)mapAtRoot).put(k, value.getValue()); + } else { + synchronized (mapAtRoot) { + return ((Map)mapAtRoot).put(k, value.getValue()); + } } } } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/7ba6df70/core/src/main/java/brooklyn/policy/basic/ConfigMapImpl.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/policy/basic/ConfigMapImpl.java b/core/src/main/java/brooklyn/policy/basic/ConfigMapImpl.java index c438fef..dc34b17 100644 --- a/core/src/main/java/brooklyn/policy/basic/ConfigMapImpl.java +++ b/core/src/main/java/brooklyn/policy/basic/ConfigMapImpl.java @@ -105,7 +105,7 @@ public class ConfigMapImpl extends AbstractConfigMapImpl { } public Object setConfig(ConfigKey<?> key, Object v) { - Object val = setConfigPrep1(key, v); + Object val = coerceConfigVal(key, v); if (key instanceof StructuredConfigKey) { return ((StructuredConfigKey)key).applyValueToMap(val, ownConfig); } else { http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/7ba6df70/docs/guide/yaml/yaml-reference.md ---------------------------------------------------------------------- diff --git a/docs/guide/yaml/yaml-reference.md b/docs/guide/yaml/yaml-reference.md index 71993ae..64c81a5 100644 --- a/docs/guide/yaml/yaml-reference.md +++ b/docs/guide/yaml/yaml-reference.md @@ -146,13 +146,15 @@ Dependency injection other powerful references and types can be built up within concise DSL defined here: * `$brooklyn:attributeWhenReady("sensor")` will store a future which will be blocked when it is accessed, - until the given `sensor` from the component `ID` has a "truthy" (i.e. non-trivial, non-empty, non-zero) value + until the given `sensor` from this entity "truthy" (i.e. non-trivial, non-empty, non-zero) value + (see below on `component` for looking up values on other sensors) * `$brooklyn:config("key")` will insert the value set against the given key at this entity (or nearest ancestor); can be used to supply config at the root which is used in multiple places in the plan * `$brooklyn:sensor("sensor.name")` returns the given sensor on the current entity if found, or an untyped (Object) sensor; `$brooklyn:sensor("io.brooklyn.ContainingEntityClass", "sensor.name")` returns the strongly typed sensor defined in the given class * `$brooklyn:component("ID")` refers to a Brooklyn component with the given ID; you can then access the following subfields, - using the same syntax as defined above but with a different reference entity: + using the same syntax as defined above but with a different reference entity, + e.g. `$brooklyn:component("ID").attributeWhenReady("sensor")`: * `.attributeWhenReady("sensor")` * `.config("key")` * `.sensor("sensor.name")` @@ -172,6 +174,8 @@ concise DSL defined here: but as an `EntitySpec` suitable for setting as the value of `ConfigKey<EntitySpec>` config items (such as `memberSpec` in `DynamicCluster`) +<!-- TODO examples for object and entitySpec --> + Parameters above can be supplied either as strings or as lists and maps in YAML, and the `$brooklyn:` syntax can be used within those parameters.
