You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by ahgittin <gi...@git.apache.org> on 2015/04/13 03:02:28 UTC

[GitHub] incubator-brooklyn pull request: Make YAML more powerful

GitHub user ahgittin opened a pull request:

    https://github.com/apache/incubator-brooklyn/pull/595

    Make YAML more powerful

    allowing a `firstMemberSpec` for cluster, make enrichers easier to set up in yaml (with good example), and better support for referring to sensors within maps (esp when configuring enrichers)

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/ahgittin/incubator-brooklyn yaml-more-powerful

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/incubator-brooklyn/pull/595.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 #595
    
----
commit 64b19928d80bef311f3f1dfebfa075466aad82c7
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date:   2015-04-02T11:23:47Z

    add an optional config `firstMemberSpec` to DynamicCluster, with test

commit 8591999ff24555cf798c590b27b31f4da5368c87
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date:   2015-04-10T11:24:54Z

    enhance yaml DSL for specifying a sensor
    
    allows `sensor("sensor.name")`, looking up on entity, falling back to untyped Object sensor

commit bf66d3ec9e8c9423b7552650f5168fb5af9245a1
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date:   2015-04-10T11:31:16Z

    tidy yaml ref docs

commit 18b6529f557794c8ff180b19bdd4937f01c8efcb
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date:   2015-04-12T16:51:44Z

    map config key improvements
    
    * resolve deep on extraction
    * do not coerce/resolve on setting
    * subkey extraction looks in parent map
    * keys in maps put in the config map will be resolved when the map is gotten (but subkeys will not match suppliers as keys)

commit f7142a3333fdabdbec0e6eb606e7b595fd8491ef
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date:   2015-04-13T00:56:16Z

    make enrichers easier to configure from yaml
    
    * entity spec keeps the list of specs, for things like enrichers, because equality (set duplication) is not very good for specs
    * makes many of the basic enrichers easier to configure from yaml, with more flexible config
    * in particular `Transformer` can be given a value supplier, e.g. `$brooklyn:formatString`
    * adds a `Joiner` enricher which does `Strings.join`, handy for converting a list to something which can be used in bash
    * good example of all of these in test-app-with-enrichers-slightly-simpler.yaml, referenced in the docs reference page

----


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:

    https://github.com/apache/incubator-brooklyn/pull/595#issuecomment-93612756
  
    great comments @aledsage - think i've addressed them all


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/595#discussion_r28530640
  
    --- Diff: core/src/main/java/brooklyn/enricher/basic/Transformer.java ---
    @@ -52,7 +57,7 @@
     
         public static ConfigKey<Sensor<?>> TARGET_SENSOR = ConfigKeys.newConfigKey(new TypeToken<Sensor<?>>() {}, "enricher.targetSensor");
         
    -    protected Function<? super SensorEvent<T>, ? extends U> transformation;
    +//    protected Function<? super SensorEvent<T>, ? extends U> transformation;
    --- End diff --
    
    Delete commented out code, or include a TODO comment to say why it's useful.


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/595#discussion_r28598363
  
    --- Diff: docs/guide/yaml/example_yaml/test-app-with-enrichers-slightly-simpler.yaml ---
    @@ -0,0 +1,57 @@
    +#
    +# example showing how enrichers can be set 
    +# 
    +name: test-app-with-enrichers
    +description: Testing many enrichers
    +services:
    +- type: brooklyn.entity.group.DynamicCluster
    +  id: cluster
    +  initialSize: 3
    +  location: localhost
    +  memberSpec:
    +    $brooklyn:entitySpec:
    +      type: brooklyn.test.entity.TestEntity
    +      brooklyn.enrichers:
    +      - type: brooklyn.enricher.basic.Transformer
    +        # transform "ip" (which we expect a feed, not shown here, to set) to a URL;
    +        # you can curl an address string to the sensors/ip endpoint an entity to trigger these enrichers 
    +        brooklyn.config:
    +          enricher.sourceSensor: $brooklyn:sensor("ip")
    +          enricher.targetSensor: $brooklyn:sensor("url")
    +          enricher.targetValue: $brooklyn:formatString("http://%s/", $brooklyn:attributeWhenReady("ip"))
    +      - type: brooklyn.enricher.basic.Propagator
    +        # use propagator to duplicate one sensor as another, giving the supplied sensor mapping;
    +        # the other use of Propagator is where you specify a producer (using $brooklyn:entity(...) as below)
    +        # from which to take sensors; in that mode you can specify `propagate` as a list of sensors whose names are unchanged,
    +        # instead of (or in addition to) this map 
    +        brooklyn.config:
    +          sensorMapping:
    +            $brooklyn:sensor("url"): $brooklyn:sensor("brooklyn.entity.basic.Attributes", "main.uri")
    +  brooklyn.enrichers:
    +  - type: brooklyn.enricher.basic.Aggregator
    +    # aggregate `url` sensors from children into a list
    +    brooklyn.config:
    +      enricher.sourceSensor: $brooklyn:sensor("url")
    +      enricher.targetSensor: $brooklyn:sensor("urls.list")
    +      enricher.aggregating.fromMembers: true
    +  - type: brooklyn.enricher.basic.Joiner
    +    # create a string from that list, for use e.g. in bash scripts
    +    brooklyn.config:
    +      enricher.sourceSensor: $brooklyn:sensor("urls.list")
    +      enricher.targetSensor: $brooklyn:sensor("urls.list.comma_separated.max_2")
    +      maximum: 2
    +      # TODO infer uniqueTag, name etc
    +      uniqueTag: urls.list.comma_separated.max_2
    +  - type: brooklyn.enricher.basic.Joiner
    +    # pick one uri as the main one to use
    +    brooklyn.config:
    +      enricher.sourceSensor: $brooklyn:sensor("urls.list")
    +      enricher.targetSensor: $brooklyn:sensor("brooklyn.entity.basic.Attributes", "main.uri")
    +      quote: false
    +      maximum: 1
    +brooklyn.enrichers:
    +- type: brooklyn.enricher.basic.Propagator
    +  # if nothing specified for `propagating` or `sensorMapping` then 
    +  # Propagator will do all but the usual lifecycle defaults, handy at the root!
    --- End diff --
    
    it's what i would expect as the default (the only reasonable option).  however if we think of a better name i'm okay with marking this beta.


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/595#discussion_r28547386
  
    --- Diff: docs/guide/yaml/yaml-reference.md ---
    @@ -37,7 +37,10 @@ the entity being defined, with these being the most common:
     
     * `brooklyn.policies`: a list of policies, each as a map described with their `type` and their `brooklyn.config` as keys
     
    -* `brooklyn.enrichers`: a list of enrichers, each as a map described with their `type` and their `brooklyn.config` as keys
    +* `brooklyn.enrichers`: a list of enrichers, each as a map described with their `type` and their `brooklyn.config` as keys;
    +  see the keys declared on individual enrichers; 
    +  also see [this enricher example](example_yaml/test-app-with-enrichers-slightly-simpler.yaml) for a detailed and commented illustration
    --- End diff --
    
    Would definitely like a different name - the "slightly simpler" should just be "normal" in the eyes of the user, so is redundant and confusing in the name.


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/595#discussion_r28544130
  
    --- Diff: core/src/test/java/brooklyn/enricher/EnrichersTest.java ---
    @@ -431,4 +435,45 @@ protected void doUpdatingMapChecks(AttributeSensor mapSensor) {
             EntityTestUtils.assertAttributeEqualsEventually(entity, mapSensor, MutableMap.<String,String>of());
         }
     
    +    private static AttributeSensor<Object> LIST_SENSOR = Sensors.newSensor(Object.class, "sensor.list");
    +    
    +    @Test
    +    public void testJoinerDefault() {
    +        entity.addEnricher(Enrichers.builder()
    +                .joining(LIST_SENSOR)
    +                .publishing(TestEntity.NAME)
    +                .build());
    +        // null values ignored, and it quotes
    +        entity.setAttribute(LIST_SENSOR, MutableList.<String>of("a", "\"b").append(null));
    +        EntityTestUtils.assertAttributeEqualsEventually(entity, TestEntity.NAME, "\"a\",\"\\\"b\"");
    +        
    +        // empty list causes ""
    +        entity.setAttribute(LIST_SENSOR, MutableList.<String>of().append(null));
    +        EntityTestUtils.assertAttributeEqualsEventually(entity, TestEntity.NAME, "");
    +        
    +        // null causes null
    +        entity.setAttribute(LIST_SENSOR, null);
    +        EntityTestUtils.assertAttributeEqualsEventually(entity, TestEntity.NAME, null);
    +    }
    +
    +    
    +    @Test
    +    public void testJoinerUnquoted() {
    --- End diff --
    
    Need more tests (e.g. that minimum/maximum are respected - when list length is increasing and when it's decreasing, etc).


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/595#discussion_r28598525
  
    --- Diff: docs/guide/yaml/example_yaml/test-app-with-enrichers-slightly-simpler.yaml ---
    @@ -0,0 +1,57 @@
    +#
    +# example showing how enrichers can be set 
    +# 
    +name: test-app-with-enrichers
    +description: Testing many enrichers
    +services:
    +- type: brooklyn.entity.group.DynamicCluster
    +  id: cluster
    +  initialSize: 3
    +  location: localhost
    +  memberSpec:
    +    $brooklyn:entitySpec:
    +      type: brooklyn.test.entity.TestEntity
    +      brooklyn.enrichers:
    +      - type: brooklyn.enricher.basic.Transformer
    +        # transform "ip" (which we expect a feed, not shown here, to set) to a URL;
    +        # you can curl an address string to the sensors/ip endpoint an entity to trigger these enrichers 
    +        brooklyn.config:
    +          enricher.sourceSensor: $brooklyn:sensor("ip")
    +          enricher.targetSensor: $brooklyn:sensor("url")
    +          enricher.targetValue: $brooklyn:formatString("http://%s/", $brooklyn:attributeWhenReady("ip"))
    +      - type: brooklyn.enricher.basic.Propagator
    +        # use propagator to duplicate one sensor as another, giving the supplied sensor mapping;
    +        # the other use of Propagator is where you specify a producer (using $brooklyn:entity(...) as below)
    +        # from which to take sensors; in that mode you can specify `propagate` as a list of sensors whose names are unchanged,
    +        # instead of (or in addition to) this map 
    +        brooklyn.config:
    +          sensorMapping:
    +            $brooklyn:sensor("url"): $brooklyn:sensor("brooklyn.entity.basic.Attributes", "main.uri")
    +  brooklyn.enrichers:
    +  - type: brooklyn.enricher.basic.Aggregator
    +    # aggregate `url` sensors from children into a list
    +    brooklyn.config:
    +      enricher.sourceSensor: $brooklyn:sensor("url")
    +      enricher.targetSensor: $brooklyn:sensor("urls.list")
    +      enricher.aggregating.fromMembers: true
    +  - type: brooklyn.enricher.basic.Joiner
    +    # create a string from that list, for use e.g. in bash scripts
    +    brooklyn.config:
    +      enricher.sourceSensor: $brooklyn:sensor("urls.list")
    +      enricher.targetSensor: $brooklyn:sensor("urls.list.comma_separated.max_2")
    +      maximum: 2
    +      # TODO infer uniqueTag, name etc
    +      uniqueTag: urls.list.comma_separated.max_2
    +  - type: brooklyn.enricher.basic.Joiner
    +    # pick one uri as the main one to use
    +    brooklyn.config:
    +      enricher.sourceSensor: $brooklyn:sensor("urls.list")
    +      enricher.targetSensor: $brooklyn:sensor("brooklyn.entity.basic.Attributes", "main.uri")
    +      quote: false
    +      maximum: 1
    +brooklyn.enrichers:
    +- type: brooklyn.enricher.basic.Propagator
    --- End diff --
    
    agree.  not in this PR though, part of massively simpler enricher yaml.  :)
    
    could we use a default catalog for this (and need to support enrichers there) ?


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/595#discussion_r28545867
  
    --- Diff: docs/guide/yaml/yaml-reference.md ---
    @@ -142,11 +145,17 @@ However you can combine locations using `multi`:
     Dependency injection other powerful references and types can be built up within the YAML using the
     concise DSL defined here:
      
    -* `$brooklyn:component("ID")` refers to a Brooklyn component with the given ID; you can then access the following subfields:
    -  * `.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
    -  * `.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: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
    +* `$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,
    --- End diff --
    
    Going through the rest of the PR, I see several examples of `brooklyn:sensor(...)` from enrichers/policies. I hadn't used those much before, so now see that accessing "this" entity is more common than I thought.
    
    However, still happy for it to show the `component("ID")` first in the docs.


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/595#discussion_r28327099
  
    --- Diff: core/src/main/java/brooklyn/config/internal/AbstractConfigMapImpl.java ---
    @@ -0,0 +1,110 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.config.internal;
    +
    +import java.util.Collections;
    +import java.util.LinkedHashMap;
    +import java.util.Map;
    +import java.util.concurrent.Future;
    +
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import brooklyn.config.ConfigKey;
    +import brooklyn.config.ConfigKey.HasConfigKey;
    +import brooklyn.config.ConfigMap;
    +import brooklyn.entity.basic.ConfigMapViewWithStringKeys;
    +import brooklyn.event.basic.StructuredConfigKey;
    +import brooklyn.util.flags.TypeCoercions;
    +import brooklyn.util.task.DeferredSupplier;
    +
    +public abstract class AbstractConfigMapImpl implements ConfigMap {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(AbstractConfigMapImpl.class);
    +    
    +    protected final ConfigMapViewWithStringKeys mapViewWithStringKeys = new ConfigMapViewWithStringKeys(this);
    +    
    +    /**
    +     * Map of configuration information that is defined at start-up time for the entity. These
    +     * configuration parameters are shared and made accessible to the "children" of this
    +     * entity.
    +     */
    +    protected Map<ConfigKey<?>,Object> ownConfig = Collections.synchronizedMap(new LinkedHashMap<ConfigKey<?>, Object>());
    +
    +    public <T> T getConfig(ConfigKey<T> key) {
    +        return getConfig(key, null);
    +    }
    +    
    +    public <T> T getConfig(HasConfigKey<T> key) {
    +        return getConfig(key.getConfigKey(), null);
    +    }
    +    
    +    public <T> T getConfig(HasConfigKey<T> key, T defaultValue) {
    +        return getConfig(key.getConfigKey(), defaultValue);
    +    }
    +    
    +    @Override @Deprecated
    +    public Object getRawConfig(ConfigKey<?> key) {
    +        return getConfigRaw(key, true).orNull();
    +    }
    +    
    +    protected Object setConfigPrep1(ConfigKey<?> key, Object v) {
    +        Object val;
    +        if ((v instanceof Future) || (v instanceof DeferredSupplier)) {
    +            // no coercion for these (coerce on exit)
    +            val = v;
    +        } else if (key instanceof StructuredConfigKey) {
    +            // no coercion for these structures (they decide what to do)
    +            val = v;
    +        } else if ((v instanceof Map || v instanceof Iterable) && key.getType().isInstance(v)) {
    +            // don't do coercion on put for these, if the key type is compatible, 
    +            // because that will force resolution deeply
    +            val = v;
    +        } else {
    +            try {
    +                // try to coerce on input, to detect errors sooner
    +                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
    --- End diff --
    
    Include a `TODO` in the comment for things that are to be changed in the future. Can you include in the comment why for now we are failing fast, so when would someone want to change this.


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin closed the pull request at:

    https://github.com/apache/incubator-brooklyn/pull/595


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/595#discussion_r28478242
  
    --- Diff: docs/guide/yaml/yaml-reference.md ---
    @@ -142,11 +145,17 @@ However you can combine locations using `multi`:
     Dependency injection other powerful references and types can be built up within the YAML using the
     concise DSL defined here:
      
    -* `$brooklyn:component("ID")` refers to a Brooklyn component with the given ID; you can then access the following subfields:
    -  * `.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
    -  * `.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: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
    +* `$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,
    --- End diff --
    
    agree with these, esp for `attributeWhenReady` -- have fixed text and added explicit example for `component...attributeWhenReady`.  (`config` in contrast in my experience is mostly used for local refs.)


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/595#discussion_r28597808
  
    --- Diff: core/src/main/java/brooklyn/enricher/basic/Transformer.java ---
    @@ -102,13 +95,65 @@ public void setEntity(EntityLocal entity) {
             }
         }
     
    +    /** returns a function for transformation, for immediate use only (not for caching, as it may change) */
    +    @SuppressWarnings("unchecked")
    +    protected Function<SensorEvent<T>, U> getTransformation() {
    --- End diff --
    
    it wouldn't change, but this design makes it easier to support changing config in future. if it's an efficiency hole we can switch to populate this at start.  will add a comment to this effect.


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/595#discussion_r28598229
  
    --- Diff: docs/guide/yaml/example_yaml/test-app-with-enrichers-slightly-simpler.yaml ---
    @@ -0,0 +1,57 @@
    +#
    +# example showing how enrichers can be set 
    --- End diff --
    
    a refactoring of these files would be nice.  let's wait until we have a massively simpler syntax however.


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/595#discussion_r28478367
  
    --- Diff: core/src/main/java/brooklyn/config/internal/AbstractConfigMapImpl.java ---
    @@ -0,0 +1,110 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.config.internal;
    +
    +import java.util.Collections;
    +import java.util.LinkedHashMap;
    +import java.util.Map;
    +import java.util.concurrent.Future;
    +
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import brooklyn.config.ConfigKey;
    +import brooklyn.config.ConfigKey.HasConfigKey;
    +import brooklyn.config.ConfigMap;
    +import brooklyn.entity.basic.ConfigMapViewWithStringKeys;
    +import brooklyn.event.basic.StructuredConfigKey;
    +import brooklyn.util.flags.TypeCoercions;
    +import brooklyn.util.task.DeferredSupplier;
    +
    +public abstract class AbstractConfigMapImpl implements ConfigMap {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(AbstractConfigMapImpl.class);
    +    
    +    protected final ConfigMapViewWithStringKeys mapViewWithStringKeys = new ConfigMapViewWithStringKeys(this);
    +    
    +    /**
    +     * Map of configuration information that is defined at start-up time for the entity. These
    +     * configuration parameters are shared and made accessible to the "children" of this
    +     * entity.
    +     */
    +    protected Map<ConfigKey<?>,Object> ownConfig = Collections.synchronizedMap(new LinkedHashMap<ConfigKey<?>, Object>());
    +
    +    public <T> T getConfig(ConfigKey<T> key) {
    +        return getConfig(key, null);
    +    }
    +    
    +    public <T> T getConfig(HasConfigKey<T> key) {
    +        return getConfig(key.getConfigKey(), null);
    +    }
    +    
    +    public <T> T getConfig(HasConfigKey<T> key, T defaultValue) {
    +        return getConfig(key.getConfigKey(), defaultValue);
    +    }
    +    
    +    @Override @Deprecated
    +    public Object getRawConfig(ConfigKey<?> key) {
    +        return getConfigRaw(key, true).orNull();
    +    }
    +    
    +    protected Object setConfigPrep1(ConfigKey<?> key, Object v) {
    +        Object val;
    +        if ((v instanceof Future) || (v instanceof DeferredSupplier)) {
    +            // no coercion for these (coerce on exit)
    +            val = v;
    +        } else if (key instanceof StructuredConfigKey) {
    +            // no coercion for these structures (they decide what to do)
    +            val = v;
    +        } else if ((v instanceof Map || v instanceof Iterable) && key.getType().isInstance(v)) {
    +            // don't do coercion on put for these, if the key type is compatible, 
    +            // because that will force resolution deeply
    +            val = v;
    +        } else {
    +            try {
    +                // try to coerce on input, to detect errors sooner
    +                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
    --- End diff --
    
    have elaborated on comment.  i don't think it's a TODO, but rather a reminder if someone encounters this exception and thinks we've made the wrong call here.


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:

    https://github.com/apache/incubator-brooklyn/pull/595#issuecomment-93841155
  
    Finished reviewing; looks really good. Happy for you to merge once you've looked through the rest of my comments.


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/595#discussion_r28326207
  
    --- Diff: docs/guide/yaml/yaml-reference.md ---
    @@ -142,11 +145,17 @@ However you can combine locations using `multi`:
     Dependency injection other powerful references and types can be built up within the YAML using the
     concise DSL defined here:
      
    -* `$brooklyn:component("ID")` refers to a Brooklyn component with the given ID; you can then access the following subfields:
    -  * `.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
    -  * `.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: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
    +* `$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,
    --- End diff --
    
    Ah, I see from this (and the code) that if you omit `component("ID")` then it refers to this entity.
    
    I'd prefer our examples here to include component, such as `$brooklyn:component("ID").config("key")`. I believe that is the most common usage, so that is what we should show people (e.g. they'll want to copy-paste). We can then say that if you omit `component(...)` then it defaults to referring to this entity. We can also move the definition of `$brooklyn:component(...)` to be first.


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/595#discussion_r28478267
  
    --- Diff: docs/guide/yaml/yaml-reference.md ---
    @@ -157,12 +166,14 @@ concise DSL defined here:
     * `$brooklyn:formatString("pattern e.g. %s %s", "field 1", "field 2")` returns a future which creates the formatted string
       with the given parameters, where parameters may be strings *or* other tasks such as `attributeWhenReady`
     * `$brooklyn:literal("string")` returns the given string as a literal (suppressing any `$brooklyn:` expansion)
    -* `$brooklyn:sensor("io.brooklyn.ContainingEntityClass", "sensor.name")` returns the strongly typed sensor defined in the given class
    +* `$brooklyn:object(Map)` creates an object, using keys `type` to define the java type,
    --- End diff --
    
    agree -- have inserted TODO for both of these


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/595#discussion_r28547133
  
    --- Diff: docs/guide/yaml/example_yaml/test-app-with-enrichers-slightly-simpler.yaml ---
    @@ -0,0 +1,57 @@
    +#
    +# example showing how enrichers can be set 
    +# 
    +name: test-app-with-enrichers
    +description: Testing many enrichers
    +services:
    +- type: brooklyn.entity.group.DynamicCluster
    +  id: cluster
    +  initialSize: 3
    +  location: localhost
    +  memberSpec:
    +    $brooklyn:entitySpec:
    +      type: brooklyn.test.entity.TestEntity
    +      brooklyn.enrichers:
    +      - type: brooklyn.enricher.basic.Transformer
    +        # transform "ip" (which we expect a feed, not shown here, to set) to a URL;
    +        # you can curl an address string to the sensors/ip endpoint an entity to trigger these enrichers 
    +        brooklyn.config:
    +          enricher.sourceSensor: $brooklyn:sensor("ip")
    +          enricher.targetSensor: $brooklyn:sensor("url")
    +          enricher.targetValue: $brooklyn:formatString("http://%s/", $brooklyn:attributeWhenReady("ip"))
    +      - type: brooklyn.enricher.basic.Propagator
    +        # use propagator to duplicate one sensor as another, giving the supplied sensor mapping;
    +        # the other use of Propagator is where you specify a producer (using $brooklyn:entity(...) as below)
    +        # from which to take sensors; in that mode you can specify `propagate` as a list of sensors whose names are unchanged,
    +        # instead of (or in addition to) this map 
    +        brooklyn.config:
    +          sensorMapping:
    +            $brooklyn:sensor("url"): $brooklyn:sensor("brooklyn.entity.basic.Attributes", "main.uri")
    +  brooklyn.enrichers:
    +  - type: brooklyn.enricher.basic.Aggregator
    +    # aggregate `url` sensors from children into a list
    +    brooklyn.config:
    +      enricher.sourceSensor: $brooklyn:sensor("url")
    +      enricher.targetSensor: $brooklyn:sensor("urls.list")
    +      enricher.aggregating.fromMembers: true
    +  - type: brooklyn.enricher.basic.Joiner
    +    # create a string from that list, for use e.g. in bash scripts
    +    brooklyn.config:
    +      enricher.sourceSensor: $brooklyn:sensor("urls.list")
    +      enricher.targetSensor: $brooklyn:sensor("urls.list.comma_separated.max_2")
    +      maximum: 2
    +      # TODO infer uniqueTag, name etc
    +      uniqueTag: urls.list.comma_separated.max_2
    +  - type: brooklyn.enricher.basic.Joiner
    +    # pick one uri as the main one to use
    +    brooklyn.config:
    +      enricher.sourceSensor: $brooklyn:sensor("urls.list")
    +      enricher.targetSensor: $brooklyn:sensor("brooklyn.entity.basic.Attributes", "main.uri")
    +      quote: false
    +      maximum: 1
    +brooklyn.enrichers:
    +- type: brooklyn.enricher.basic.Propagator
    +  # if nothing specified for `propagating` or `sensorMapping` then 
    +  # Propagator will do all but the usual lifecycle defaults, handy at the root!
    --- End diff --
    
    Slightly hesitant about this being the default? I can see why people would want that, but it's probably a bit surprising. 
    
    It's also a bit surprising that there's not an explicit config key corresponding to this default (so that one can be explicit about behaviour, rather than relying on implicit defaults).
    
    My gut feel was that you'd need (or at least be able to specify) a config key to indicate intent - e.g. "propagateAllButGenericHealth". But obviously I dislike that name; not sure what a better name would be.
    



---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/595#discussion_r28549896
  
    --- Diff: docs/guide/yaml/example_yaml/test-app-with-enrichers-slightly-simpler.yaml ---
    @@ -0,0 +1,57 @@
    +#
    +# example showing how enrichers can be set 
    +# 
    +name: test-app-with-enrichers
    +description: Testing many enrichers
    +services:
    +- type: brooklyn.entity.group.DynamicCluster
    +  id: cluster
    +  initialSize: 3
    +  location: localhost
    +  memberSpec:
    +    $brooklyn:entitySpec:
    +      type: brooklyn.test.entity.TestEntity
    +      brooklyn.enrichers:
    +      - type: brooklyn.enricher.basic.Transformer
    +        # transform "ip" (which we expect a feed, not shown here, to set) to a URL;
    +        # you can curl an address string to the sensors/ip endpoint an entity to trigger these enrichers 
    +        brooklyn.config:
    +          enricher.sourceSensor: $brooklyn:sensor("ip")
    +          enricher.targetSensor: $brooklyn:sensor("url")
    +          enricher.targetValue: $brooklyn:formatString("http://%s/", $brooklyn:attributeWhenReady("ip"))
    --- End diff --
    
    Great examples, and great that we can now support all of these. I like the yaml here.
    
    Rambling thoughts follow, not pertinent to this PR!
    
    I wonder about things like `$brooklyn:sensor("brooklyn.entity.basic.Attributes", "main.uri")`. As long as people hardly ever write it, then I'm ok. Otherwise, need to think of the trade-off for letting them just create a new untyped sensor with the same name, versus it finding the statically defined sensor. The untyped obviously wouldn't have the description and type metadata, but would be simpler in the yaml.


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/595#discussion_r28545581
  
    --- Diff: docs/guide/yaml/example_yaml/test-app-with-enrichers-slightly-simpler.yaml ---
    @@ -0,0 +1,57 @@
    +#
    +# example showing how enrichers can be set 
    --- End diff --
    
    I'd prefer a file name that is more descriptive (particularly for when someone is deciding what yaml to look at). It's unclear from the name whether this is simpler use-cases of enrichers or simple yaml format for achieving the same thing with enrichers.
    
    Assuming the latter, if there are old files with the old way, I'd prefer us to rename those yaml files to "legacy" or some such. If there aren't old files, then why call this "slightly-simpler"?


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/595#discussion_r28478044
  
    --- Diff: core/src/main/java/brooklyn/event/basic/MapConfigKey.java ---
    @@ -125,6 +130,20 @@ protected Object applyEntryValueToMap(Entry value, Map target) {
             } else if (k instanceof String) {
                 k = subKey((String)k);
             } 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);
    +                }
    +                if (mapAtRoot instanceof Map) {
    +                    synchronized (mapAtRoot) {
    --- End diff --
    
    often the map will be what we `put` just above -- ie an LHMap, and sometimes it will come from another source, e.g. yaml parse (also often an LHMap).
    
    in most cases synching on the map is the standard way to synchronize.  it's not foolproof of course.  but feels likely to cause a problem than a CME on an LHMap.
    
    will add a comment to that effect.


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by ahgittin <gi...@git.apache.org>.
GitHub user ahgittin reopened a pull request:

    https://github.com/apache/incubator-brooklyn/pull/595

    Make YAML more powerful

    allowing a `firstMemberSpec` for cluster, make enrichers easier to set up in yaml (with good example), and better support for referring to sensors within maps (esp when configuring enrichers)

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/ahgittin/incubator-brooklyn yaml-more-powerful

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/incubator-brooklyn/pull/595.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 #595
    
----
commit 64b19928d80bef311f3f1dfebfa075466aad82c7
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date:   2015-04-02T11:23:47Z

    add an optional config `firstMemberSpec` to DynamicCluster, with test

commit 8591999ff24555cf798c590b27b31f4da5368c87
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date:   2015-04-10T11:24:54Z

    enhance yaml DSL for specifying a sensor
    
    allows `sensor("sensor.name")`, looking up on entity, falling back to untyped Object sensor

commit bf66d3ec9e8c9423b7552650f5168fb5af9245a1
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date:   2015-04-10T11:31:16Z

    tidy yaml ref docs

commit 18b6529f557794c8ff180b19bdd4937f01c8efcb
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date:   2015-04-12T16:51:44Z

    map config key improvements
    
    * resolve deep on extraction
    * do not coerce/resolve on setting
    * subkey extraction looks in parent map
    * keys in maps put in the config map will be resolved when the map is gotten (but subkeys will not match suppliers as keys)

commit f7142a3333fdabdbec0e6eb606e7b595fd8491ef
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date:   2015-04-13T00:56:16Z

    make enrichers easier to configure from yaml
    
    * entity spec keeps the list of specs, for things like enrichers, because equality (set duplication) is not very good for specs
    * makes many of the basic enrichers easier to configure from yaml, with more flexible config
    * in particular `Transformer` can be given a value supplier, e.g. `$brooklyn:formatString`
    * adds a `Joiner` enricher which does `Strings.join`, handy for converting a list to something which can be used in bash
    * good example of all of these in test-app-with-enrichers-slightly-simpler.yaml, referenced in the docs reference page

commit 7ba6df707b7485469c6c0d75cddab46bd849ccb9
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date:   2015-04-16T01:31:19Z

    addressing comments from code review on enhancing yaml

----


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/595#discussion_r28531127
  
    --- Diff: core/src/main/java/brooklyn/enricher/basic/Transformer.java ---
    @@ -102,13 +95,65 @@ public void setEntity(EntityLocal entity) {
             }
         }
     
    +    /** returns a function for transformation, for immediate use only (not for caching, as it may change) */
    +    @SuppressWarnings("unchecked")
    +    protected Function<SensorEvent<T>, U> getTransformation() {
    --- End diff --
    
    Why might the result of `getTransformation()` change from one call to the other? Is it not based on the config values? If so and if we're catering for config values changing, then we could override `doReconfigureConfig(...)`.


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/595#discussion_r28326756
  
    --- Diff: docs/guide/yaml/yaml-reference.md ---
    @@ -157,12 +166,14 @@ concise DSL defined here:
     * `$brooklyn:formatString("pattern e.g. %s %s", "field 1", "field 2")` returns a future which creates the formatted string
       with the given parameters, where parameters may be strings *or* other tasks such as `attributeWhenReady`
     * `$brooklyn:literal("string")` returns the given string as a literal (suppressing any `$brooklyn:` expansion)
    -* `$brooklyn:sensor("io.brooklyn.ContainingEntityClass", "sensor.name")` returns the strongly typed sensor defined in the given class
    +* `$brooklyn:object(Map)` creates an object, using keys `type` to define the java type,
    +  and either `object.fields` or `brooklyn.config` to supply bean/constructor/flags to create an instance
     * `$brooklyn:entitySpec(Map)` returns a new `ServiceSpecification` as defined by the given `Map`,
    --- End diff --
    
    Longer term, this could do with a cross-reference to a section that describes an `EntitySpec` (for those who don't look at the Java api), and we should avoid java generics syntax like `ConfigKey<EntitySpec>`.


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/595#discussion_r28328337
  
    --- Diff: core/src/main/java/brooklyn/event/basic/MapConfigKey.java ---
    @@ -125,6 +130,20 @@ protected Object applyEntryValueToMap(Entry value, Map target) {
             } else if (k instanceof String) {
                 k = subKey((String)k);
             } 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);
    +                }
    +                if (mapAtRoot instanceof Map) {
    +                    synchronized (mapAtRoot) {
    --- End diff --
    
    Not convinced we should just synchronize on this map, without knowing anything about it. That feels dangerous and ad hoc. We can presumably rely on the map's put method to be synchronized if the map really needs to be synchronized. What do we expect this map (returned by `target.get(this)`) to be?


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/595#discussion_r28327337
  
    --- Diff: core/src/main/java/brooklyn/config/internal/AbstractConfigMapImpl.java ---
    @@ -0,0 +1,110 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.config.internal;
    +
    +import java.util.Collections;
    +import java.util.LinkedHashMap;
    +import java.util.Map;
    +import java.util.concurrent.Future;
    +
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import brooklyn.config.ConfigKey;
    +import brooklyn.config.ConfigKey.HasConfigKey;
    +import brooklyn.config.ConfigMap;
    +import brooklyn.entity.basic.ConfigMapViewWithStringKeys;
    +import brooklyn.event.basic.StructuredConfigKey;
    +import brooklyn.util.flags.TypeCoercions;
    +import brooklyn.util.task.DeferredSupplier;
    +
    +public abstract class AbstractConfigMapImpl implements ConfigMap {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(AbstractConfigMapImpl.class);
    +    
    +    protected final ConfigMapViewWithStringKeys mapViewWithStringKeys = new ConfigMapViewWithStringKeys(this);
    +    
    +    /**
    +     * Map of configuration information that is defined at start-up time for the entity. These
    +     * configuration parameters are shared and made accessible to the "children" of this
    +     * entity.
    +     */
    +    protected Map<ConfigKey<?>,Object> ownConfig = Collections.synchronizedMap(new LinkedHashMap<ConfigKey<?>, Object>());
    +
    +    public <T> T getConfig(ConfigKey<T> key) {
    +        return getConfig(key, null);
    +    }
    +    
    +    public <T> T getConfig(HasConfigKey<T> key) {
    +        return getConfig(key.getConfigKey(), null);
    +    }
    +    
    +    public <T> T getConfig(HasConfigKey<T> key, T defaultValue) {
    +        return getConfig(key.getConfigKey(), defaultValue);
    +    }
    +    
    +    @Override @Deprecated
    +    public Object getRawConfig(ConfigKey<?> key) {
    +        return getConfigRaw(key, true).orNull();
    +    }
    +    
    +    protected Object setConfigPrep1(ConfigKey<?> key, Object v) {
    --- End diff --
    
    Is there a better name for this, such as `coerceConfigVal(...)` perhaps?


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/595#discussion_r28550544
  
    --- Diff: core/src/main/java/brooklyn/event/basic/MapConfigKey.java ---
    @@ -125,6 +131,25 @@ protected Object applyEntryValueToMap(Entry value, Map target) {
             } else if (k instanceof String) {
                 k = subKey((String)k);
             } else {
    +            // supplier or other unexpected value
    +            if (k instanceof Supplier) {
    +                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) {
    +                    if (mapAtRoot instanceof ConcurrentMap) {
    +                        return ((Map)mapAtRoot).put(k, value.getValue());
    +                    } else {
    +                        synchronized (mapAtRoot) {
    --- End diff --
    
    Or the supplier of the map might have done `Collections.synchronizedMap(...)`. I'm ok with leaving this synchornized in here, but think that we should never be relying on it - if synchronization is important, then the map should have been created with that in mind (e.g. `Collections.synchronizedMap(...)` means that you never need to synchronize explicitly except for multi-call actions such as iterating).


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/595#discussion_r28529574
  
    --- Diff: core/src/main/java/brooklyn/enricher/basic/Joiner.java ---
    @@ -0,0 +1,128 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.enricher.basic;
    +
    +import java.util.Map;
    +
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import brooklyn.config.ConfigKey;
    +import brooklyn.entity.Entity;
    +import brooklyn.entity.basic.ConfigKeys;
    +import brooklyn.entity.basic.EntityLocal;
    +import brooklyn.event.AttributeSensor;
    +import brooklyn.event.Sensor;
    +import brooklyn.event.SensorEvent;
    +import brooklyn.event.SensorEventListener;
    +import brooklyn.event.basic.BasicSensorEvent;
    +import brooklyn.util.collections.MutableList;
    +import brooklyn.util.flags.SetFromFlag;
    +import brooklyn.util.text.StringEscapes;
    +import brooklyn.util.text.Strings;
    +
    +import com.google.common.reflect.TypeToken;
    +
    +//@Catalog(name="Transformer", description="Transforms attributes of an entity; see Enrichers.builder().transforming(...)")
    +@SuppressWarnings("serial")
    +public class Joiner<T> extends AbstractEnricher implements SensorEventListener<T> {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(Joiner.class);
    +
    +    public static ConfigKey<Entity> PRODUCER = ConfigKeys.newConfigKey(Entity.class, "enricher.producer");
    +    public static ConfigKey<Sensor<?>> SOURCE_SENSOR = ConfigKeys.newConfigKey(new TypeToken<Sensor<?>>() {}, "enricher.sourceSensor");
    +    public static ConfigKey<Sensor<?>> TARGET_SENSOR = ConfigKeys.newConfigKey(new TypeToken<Sensor<?>>() {}, "enricher.targetSensor");
    +    @SetFromFlag("separator")
    +    public static ConfigKey<String> SEPARATOR = ConfigKeys.newStringConfigKey("enricher.joiner.separator", "Separator string to insert between each argument", ",");
    +    @SetFromFlag("quote")
    +    public static ConfigKey<Boolean> QUOTE = ConfigKeys.newBooleanConfigKey("enricher.joiner.quote", "Whether to bash-escape each parameter and wrap in double-quotes, defaulting to true", true);
    +    @SetFromFlag("minimum")
    +    public static ConfigKey<Integer> MINIMUM = ConfigKeys.newIntegerConfigKey("enricher.joiner.minimum", "Minimum number of elements to join; if fewer than this, sets null; default 0 (no minimum)");
    +    @SetFromFlag("maximum")
    +    public static ConfigKey<Integer> MAXIMUM = ConfigKeys.newIntegerConfigKey("enricher.joiner.maximum", "Maximum number of elements to join; default null means all elements always taken");
    +    
    +//    protected Function<? super SensorEvent<T>, ? extends U> transformation;
    --- End diff --
    
    Delete commented out code, or include a TODO comment to say why it's useful.


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/595#discussion_r28547255
  
    --- Diff: docs/guide/yaml/example_yaml/test-app-with-enrichers-slightly-simpler.yaml ---
    @@ -0,0 +1,57 @@
    +#
    +# example showing how enrichers can be set 
    +# 
    +name: test-app-with-enrichers
    +description: Testing many enrichers
    +services:
    +- type: brooklyn.entity.group.DynamicCluster
    +  id: cluster
    +  initialSize: 3
    +  location: localhost
    +  memberSpec:
    +    $brooklyn:entitySpec:
    +      type: brooklyn.test.entity.TestEntity
    +      brooklyn.enrichers:
    +      - type: brooklyn.enricher.basic.Transformer
    +        # transform "ip" (which we expect a feed, not shown here, to set) to a URL;
    +        # you can curl an address string to the sensors/ip endpoint an entity to trigger these enrichers 
    +        brooklyn.config:
    +          enricher.sourceSensor: $brooklyn:sensor("ip")
    +          enricher.targetSensor: $brooklyn:sensor("url")
    +          enricher.targetValue: $brooklyn:formatString("http://%s/", $brooklyn:attributeWhenReady("ip"))
    +      - type: brooklyn.enricher.basic.Propagator
    +        # use propagator to duplicate one sensor as another, giving the supplied sensor mapping;
    +        # the other use of Propagator is where you specify a producer (using $brooklyn:entity(...) as below)
    +        # from which to take sensors; in that mode you can specify `propagate` as a list of sensors whose names are unchanged,
    +        # instead of (or in addition to) this map 
    +        brooklyn.config:
    +          sensorMapping:
    +            $brooklyn:sensor("url"): $brooklyn:sensor("brooklyn.entity.basic.Attributes", "main.uri")
    +  brooklyn.enrichers:
    +  - type: brooklyn.enricher.basic.Aggregator
    +    # aggregate `url` sensors from children into a list
    +    brooklyn.config:
    +      enricher.sourceSensor: $brooklyn:sensor("url")
    +      enricher.targetSensor: $brooklyn:sensor("urls.list")
    +      enricher.aggregating.fromMembers: true
    +  - type: brooklyn.enricher.basic.Joiner
    +    # create a string from that list, for use e.g. in bash scripts
    +    brooklyn.config:
    +      enricher.sourceSensor: $brooklyn:sensor("urls.list")
    +      enricher.targetSensor: $brooklyn:sensor("urls.list.comma_separated.max_2")
    +      maximum: 2
    +      # TODO infer uniqueTag, name etc
    +      uniqueTag: urls.list.comma_separated.max_2
    +  - type: brooklyn.enricher.basic.Joiner
    +    # pick one uri as the main one to use
    +    brooklyn.config:
    +      enricher.sourceSensor: $brooklyn:sensor("urls.list")
    +      enricher.targetSensor: $brooklyn:sensor("brooklyn.entity.basic.Attributes", "main.uri")
    +      quote: false
    +      maximum: 1
    +brooklyn.enrichers:
    +- type: brooklyn.enricher.basic.Propagator
    --- End diff --
    
    In the same way as we have short-hand for "Autoscaler" not needing to be fully qualified, we should probably have short-hand for these ones. The argument being that these are the things explicitly on `Enrichers.builder()`.


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/595#discussion_r28478334
  
    --- Diff: core/src/main/java/brooklyn/config/internal/AbstractConfigMapImpl.java ---
    @@ -0,0 +1,110 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.config.internal;
    +
    +import java.util.Collections;
    +import java.util.LinkedHashMap;
    +import java.util.Map;
    +import java.util.concurrent.Future;
    +
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import brooklyn.config.ConfigKey;
    +import brooklyn.config.ConfigKey.HasConfigKey;
    +import brooklyn.config.ConfigMap;
    +import brooklyn.entity.basic.ConfigMapViewWithStringKeys;
    +import brooklyn.event.basic.StructuredConfigKey;
    +import brooklyn.util.flags.TypeCoercions;
    +import brooklyn.util.task.DeferredSupplier;
    +
    +public abstract class AbstractConfigMapImpl implements ConfigMap {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(AbstractConfigMapImpl.class);
    +    
    +    protected final ConfigMapViewWithStringKeys mapViewWithStringKeys = new ConfigMapViewWithStringKeys(this);
    +    
    +    /**
    +     * Map of configuration information that is defined at start-up time for the entity. These
    +     * configuration parameters are shared and made accessible to the "children" of this
    +     * entity.
    +     */
    +    protected Map<ConfigKey<?>,Object> ownConfig = Collections.synchronizedMap(new LinkedHashMap<ConfigKey<?>, Object>());
    +
    +    public <T> T getConfig(ConfigKey<T> key) {
    +        return getConfig(key, null);
    +    }
    +    
    +    public <T> T getConfig(HasConfigKey<T> key) {
    +        return getConfig(key.getConfigKey(), null);
    +    }
    +    
    +    public <T> T getConfig(HasConfigKey<T> key, T defaultValue) {
    +        return getConfig(key.getConfigKey(), defaultValue);
    +    }
    +    
    +    @Override @Deprecated
    +    public Object getRawConfig(ConfigKey<?> key) {
    +        return getConfigRaw(key, true).orNull();
    +    }
    +    
    +    protected Object setConfigPrep1(ConfigKey<?> key, Object v) {
    --- End diff --
    
    +1


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/595#discussion_r28326802
  
    --- Diff: docs/guide/yaml/yaml-reference.md ---
    @@ -157,12 +166,14 @@ concise DSL defined here:
     * `$brooklyn:formatString("pattern e.g. %s %s", "field 1", "field 2")` returns a future which creates the formatted string
       with the given parameters, where parameters may be strings *or* other tasks such as `attributeWhenReady`
     * `$brooklyn:literal("string")` returns the given string as a literal (suppressing any `$brooklyn:` expansion)
    -* `$brooklyn:sensor("io.brooklyn.ContainingEntityClass", "sensor.name")` returns the strongly typed sensor defined in the given class
    +* `$brooklyn:object(Map)` creates an object, using keys `type` to define the java type,
    --- End diff --
    
    Longer term, we should include an example here. Folk not familiar with yaml will likely struggle with what `object(Map)` means.


---
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] incubator-brooklyn pull request: Make YAML more powerful

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/incubator-brooklyn/pull/595


---
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.
---