You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by aledsage <gi...@git.apache.org> on 2015/08/05 11:14:06 UTC

[GitHub] incubator-brooklyn pull request: Performance improvements and othe...

GitHub user aledsage opened a pull request:

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

    Performance improvements and other things!

    An eclectic mix of things (which could have been split into multiple PRs, but if you're happy to review it in one PR then that's easier for me). See individual commits for details (copy-pasted below for your convenience, because I find it annoying to click through to each commit to see the details of all commits in the PR in github!).
    
    commit e1a06b18c893ad04019d406e8411d7aa1d029b6d
    Author: Aled Sage <al...@gmail.com>
    Date:   Tue Aug 4 23:29:43 2015 +0100
    
        Adds CatalogYamlAppTest.testAddCatalogItemWithCircularReference
        
        A work-in-progress test, demonstrating a StackOverflowError.
    
    commit 651afa11a5f488ae8d8467bc051925b8370e5a59
    Author: Aled Sage <al...@gmail.com>
    Date:   Tue Aug 4 23:23:58 2015 +0100
    
        Creates ApplicationsYamlTest
        
        Moves tests about app-wrapping from EntitiesYamlTest to
        ApplicationsYamlTest.
    
    commit 78d0aa2fa17a7957fc7e930f3a2998bd1072ebfb
    Author: Aled Sage <al...@gmail.com>
    Date:   Tue Aug 4 23:23:02 2015 +0100
    
        Adds GeoscalingDnsService.SSL_TRUST_ALL
    
    commit fed5b8004f63fb8139a464c12fb07c6ebab1cabe
    Author: Aled Sage <al...@gmail.com>
    Date:   Tue Aug 4 23:21:05 2015 +0100
    
        Fix SameServerEntity: up when all members up
    
    commit 33f39cdd2723e38b0d8274ca5697b72e7c0b6dbc
    Author: Aled Sage <al...@gmail.com>
    Date:   Tue Aug 4 23:18:43 2015 +0100
    
        Tests for defaultDisplayName
    
    commit fecadef01f74f600dd3d4a8301bbf8637f8a82bc
    Author: Aled Sage <al...@gmail.com>
    Date:   Tue Aug 4 23:17:43 2015 +0100
    
        Fix EntityManagementUtils.hasNoNameOrCustomKeysOrRoot
    
    commit 071a9232e864087c0be394cb813636b8e89b7d94
    Author: Aled Sage <al...@gmail.com>
    Date:   Wed May 27 11:47:19 2015 +0100
    
        Adds sensor-notifications for GROUP_ADDED/REMOVED
    
    commit 22175cb4c30e7b92c2da16b304f700d215684e2c
    Author: Aled Sage <al...@gmail.com>
    Date:   Tue Aug 4 21:47:12 2015 +0100
    
        Adds RecordingSensorEventListener (in src/test/java)
        
        Deletes the other multiple versions.
    
    commit 02e71ca4f48bdf4a80dda1aa870a3704cadddb66
    Author: Aled Sage <al...@gmail.com>
    Date:   Mon May 25 22:25:30 2015 +0100
    
        Adds ControlledDynamicWebAppCluster.LOAD_BALANCED_GROUP
    
    commit 97fb0b02c75e7ad4079f65c093a7fc13e5bbe2ec
    Author: Aled Sage <al...@gmail.com>
    Date:   Fri May 1 20:18:06 2015 +0100
    
        Fix subnetHostname (in JcloudsLocation’s private hostname)
    
    commit 8b473e576c10c419f6e65854b7287b4a061b47c9
    Author: Aled Sage <al...@gmail.com>
    Date:   Fri May 1 20:17:04 2015 +0100
    
        Tidy Machines.java logging
    
    commit 37459b5f1e4558e8688415168668e812cd7dac21
    Author: Aled Sage <al...@gmail.com>
    Date:   Thu Apr 23 21:21:18 2015 +0100
    
        Adds SoftwareProcess.SERVICE_NOT_UP_DIAGNOSTICS
        
        In SoftwareProcess, only do expensive checks (e.g. process-running)
        if serviceUp is false.
    
    commit acf3dd72cec01e8d74f8d83ea3ee7c3059d4eef9
    Author: Aled Sage <al...@gmail.com>
    Date:   Sat Apr 18 14:40:33 2015 +0100
    
        Adds SoftwareProcess.RETRIEVE_USAGE_METRICS
        
        - RETRIEVE_USAGE_METRICS defaults to true
        - Setting RETRIEVE_USAGE_METRICS to false will disable the automatic
          retrieval of performance/usage metrics in some entity types.
        - Adds FeedConfig.enabled as an easy way to programmatically
          prevent a feed from being enabled (which can be easier to program
          against than guarding its creation with an if statement).
        - Adds TomcatServerDisableRetrieveUsageMetricsIntegrationTest
    
    commit c0c31e8c006c01b26f138124b56d5d4e7cefaf07
    Author: Aled Sage <al...@gmail.com>
    Date:   Sat Apr 18 14:38:50 2015 +0100
    
        Adds TypeCoercions.function(Class)
    
    commit 3120987974bde20a85d780dfbe773d9bed6aa748
    Author: Aled Sage <al...@gmail.com>
    Date:   Sat Apr 18 13:01:36 2015 +0100
    
        Entities suppress duplicates in feeds
    
    commit f20244bd51ffdba9f246a4afe317f1556322374e
    Author: Aled Sage <al...@gmail.com>
    Date:   Sat Apr 18 12:50:48 2015 +0100
    
        Support suppressDuplicates for attributes in feeds
    
    commit 7d20c6a00836e2ec6a69367e3f680d6be0cfccc9
    Author: Aled Sage <al...@gmail.com>
    Date:   Sat Apr 18 12:44:35 2015 +0100
    
        HttpValueFunctions: not anonymous inner classes
        
        - HttpValueFunctions uses static classes rather than anonymous inner
          classes
          (because they are extremely brittle in persisted state)
        - Adds containsHeader(), used by nginx
        - Nginx uses static classes, rather than anonymous inner classes
    


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

    $ git pull https://github.com/aledsage/incubator-brooklyn fix/performance-improvements-rebased-20150804

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

    https://github.com/apache/incubator-brooklyn/pull/790.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 #790
    
----
commit 7d20c6a00836e2ec6a69367e3f680d6be0cfccc9
Author: Aled Sage <al...@gmail.com>
Date:   2015-04-18T11:44:35Z

    HttpValueFunctions: not anonymous inner classes
    
    - HttpValueFunctions uses static classes rather than anonymous inner
      classes
      (because they are extremely brittle in persisted state)
    - Adds containsHeader(), used by nginx
    - Nginx uses static classes, rather than anonymous inner classes

commit f20244bd51ffdba9f246a4afe317f1556322374e
Author: Aled Sage <al...@gmail.com>
Date:   2015-04-18T11:50:48Z

    Support suppressDuplicates for attributes in feeds

commit 3120987974bde20a85d780dfbe773d9bed6aa748
Author: Aled Sage <al...@gmail.com>
Date:   2015-04-18T12:01:36Z

    Entities suppress duplicates in feeds

commit c0c31e8c006c01b26f138124b56d5d4e7cefaf07
Author: Aled Sage <al...@gmail.com>
Date:   2015-04-18T13:38:50Z

    Adds TypeCoercions.function(Class)

commit acf3dd72cec01e8d74f8d83ea3ee7c3059d4eef9
Author: Aled Sage <al...@gmail.com>
Date:   2015-04-18T13:40:33Z

    Adds SoftwareProcess.RETRIEVE_USAGE_METRICS
    
    - RETRIEVE_USAGE_METRICS defaults to true
    - Setting RETRIEVE_USAGE_METRICS to false will disable the automatic
      retrieval of performance/usage metrics in some entity types.
    - Adds FeedConfig.enabled as an easy way to programmatically
      prevent a feed from being enabled (which can be easier to program
      against than guarding its creation with an if statement).
    - Adds TomcatServerDisableRetrieveUsageMetricsIntegrationTest

commit 37459b5f1e4558e8688415168668e812cd7dac21
Author: Aled Sage <al...@gmail.com>
Date:   2015-04-23T20:21:18Z

    Adds SoftwareProcess.SERVICE_NOT_UP_DIAGNOSTICS
    
    In SoftwareProcess, only do expensive checks (e.g. process-running)
    if serviceUp is false.

commit 8b473e576c10c419f6e65854b7287b4a061b47c9
Author: Aled Sage <al...@gmail.com>
Date:   2015-05-01T19:17:04Z

    Tidy Machines.java logging

commit 97fb0b02c75e7ad4079f65c093a7fc13e5bbe2ec
Author: Aled Sage <al...@gmail.com>
Date:   2015-05-01T19:18:06Z

    Fix subnetHostname (in JcloudsLocation’s private hostname)

commit 02e71ca4f48bdf4a80dda1aa870a3704cadddb66
Author: Aled Sage <al...@gmail.com>
Date:   2015-05-25T21:25:30Z

    Adds ControlledDynamicWebAppCluster.LOAD_BALANCED_GROUP

commit 071a9232e864087c0be394cb813636b8e89b7d94
Author: Aled Sage <al...@gmail.com>
Date:   2015-05-27T10:47:19Z

    Adds sensor-notifications for GROUP_ADDED/REMOVED

commit 22175cb4c30e7b92c2da16b304f700d215684e2c
Author: Aled Sage <al...@gmail.com>
Date:   2015-08-04T20:47:12Z

    Adds RecordingSensorEventListener (in src/test/java)
    
    Deletes the other multiple versions.

commit fecadef01f74f600dd3d4a8301bbf8637f8a82bc
Author: Aled Sage <al...@gmail.com>
Date:   2015-08-04T22:17:43Z

    Fix EntityManagementUtils.hasNoNameOrCustomKeysOrRoot

commit 33f39cdd2723e38b0d8274ca5697b72e7c0b6dbc
Author: Aled Sage <al...@gmail.com>
Date:   2015-08-04T22:18:43Z

    Tests for defaultDisplayName

commit fed5b8004f63fb8139a464c12fb07c6ebab1cabe
Author: Aled Sage <al...@gmail.com>
Date:   2015-08-04T22:21:05Z

    Fix SameServerEntity: up when all members up

commit 78d0aa2fa17a7957fc7e930f3a2998bd1072ebfb
Author: Aled Sage <al...@gmail.com>
Date:   2015-08-04T22:23:02Z

    Adds GeoscalingDnsService.SSL_TRUST_ALL

commit 651afa11a5f488ae8d8467bc051925b8370e5a59
Author: Aled Sage <al...@gmail.com>
Date:   2015-08-04T22:23:58Z

    Creates ApplicationsYamlTest
    
    Moves tests about app-wrapping from EntitiesYamlTest to
    ApplicationsYamlTest.

commit e1a06b18c893ad04019d406e8411d7aa1d029b6d
Author: Aled Sage <al...@gmail.com>
Date:   2015-08-04T22:29:43Z

    Adds CatalogYamlAppTest.testAddCatalogItemWithCircularReference
    
    A work-in-progress test, demonstrating a StackOverflowError.

----


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36289674
  
    --- Diff: core/src/test/java/brooklyn/test/entity/RecordingSensorEventListener.java ---
    @@ -0,0 +1,137 @@
    +/*
    + * 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.test.entity;
    +
    +import static com.google.common.base.Preconditions.checkNotNull;
    +
    +import java.util.Collection;
    +import java.util.Collections;
    +import java.util.List;
    +
    +import com.google.common.base.Objects;
    +import com.google.common.base.Supplier;
    +import com.google.common.collect.ArrayListMultimap;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.Iterables;
    +import com.google.common.collect.Lists;
    +import com.google.common.collect.Multimap;
    +import com.google.common.collect.Multimaps;
    +
    +import brooklyn.entity.Entity;
    +import brooklyn.event.Sensor;
    +import brooklyn.event.SensorEvent;
    +import brooklyn.event.SensorEventListener;
    +
    +public class RecordingSensorEventListener<T> implements SensorEventListener<T> {
    +    
    +    private final boolean skipDuplicateValues;
    +    private final List<SensorEvent<T>> events = Collections.synchronizedList(Lists.<SensorEvent<T>>newArrayList());
    +    private final List<T> values = Collections.synchronizedList(Lists.<T>newArrayList());
    +    private final Multimap<EntityAndSensor<?>, SensorEvent<T>> eventsBySource = 
    +            Multimaps.synchronizedListMultimap(ArrayListMultimap.<EntityAndSensor<?>, SensorEvent<T>>create());
    +    private final Multimap<EntityAndSensor<?>, T> valuesBySource = 
    +            Multimaps.synchronizedListMultimap(ArrayListMultimap.<EntityAndSensor<?>, T>create());
    +
    +    public RecordingSensorEventListener() {
    +        this(false);
    +    }
    +    
    +    public RecordingSensorEventListener(boolean skipDuplicateValues) {
    +        this.skipDuplicateValues = skipDuplicateValues;
    +    }
    +    
    +    @Override
    +    public void onEvent(SensorEvent<T> event) {
    +        EntityAndSensor<?> source = new EntityAndSensor<T>(event.getSource(),  event.getSensor());
    +        Collection<T> previousValues = valuesBySource.get(source);
    +        if (skipDuplicateValues && !previousValues.isEmpty() && Objects.equal(Iterables.get(previousValues, previousValues.size()-1), event.getValue())) {
    --- End diff --
    
    Also: `Iterables.getLast()`


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36395503
  
    --- Diff: core/src/main/java/brooklyn/event/feed/shell/ShellFeed.java ---
    @@ -196,6 +196,7 @@ protected ShellFeed(Builder builder) {
     
             SetMultimap<ShellPollIdentifier, ShellPollConfig<?>> polls = HashMultimap.<ShellPollIdentifier,ShellPollConfig<?>>create();
             for (ShellPollConfig<?> config : builder.polls) {
    +            if (!config.isEnabled()) continue;
    --- End diff --
    
    As above (easy to forget).


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36393147
  
    --- Diff: usage/camp/src/test/java/io/brooklyn/camp/brooklyn/catalog/CatalogYamlAppTest.java ---
    @@ -0,0 +1,59 @@
    +/*
    + * 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 io.brooklyn.camp.brooklyn.catalog;
    +
    +import org.testng.annotations.Test;
    +
    +import io.brooklyn.camp.brooklyn.AbstractYamlTest;
    +
    +
    +public class CatalogYamlAppTest extends AbstractYamlTest {
    +    
    +    // "Contrived" example was encountered by a customer in a real use-case!
    +    // I couldn't yet simplify it further while still reproducing the failure.
    +    // Throws StackOverlfowError, without giving a nice error message about 
    +    // "BasicEntity" cyclic reference
    +    @Test(groups="WIP") // TODO Fix this!
    +    public void testAddCatalogItemWithCircularReference() throws Exception {
    +        addCatalogItems(
    +                "brooklyn.catalog:",
    +                "  id: brooklyn.entity.basic.BasicEntity",
    +                "  version: "+TEST_VERSION,
    +                "services:",
    +                "- type: brooklyn.entity.basic.BasicApplication",
    +                "  brooklyn.config:",
    +                "    memberSpec:",
    +                "      $brooklyn:entitySpec:",
    +                "      - type: brooklyn.entity.basic.BasicApplication",
    +                "        brooklyn.children:",
    +                "        - type: brooklyn.entity.basic.BasicEntity");
    +
    +        try {
    +            addCatalogItems(
    +                    "brooklyn.catalog:",
    +                    "  id: another.app.in.the.catalog",
    +                    "  version: "+TEST_VERSION,
    +                    "services:",
    +                    "- type: brooklyn.entity.basic.BasicEntity");
    +            deleteCatalogEntity("another.app.in.the.catalog");
    +        } finally {
    +            deleteCatalogEntity("brooklyn.entity.basic.BasicEntity");
    +        }
    +    }
    +}
    --- End diff --
    
    This could be clearer to human readers:
    * It's not readily apparent where the circular reference stems from; I'm guessing it's the member spec referring to the ID of the enclosing app?
    * Is it necessary to have a catalog ID that resembles a Java classname? If it's not pertinent, use something distinct.
    * `memberSpec` looks odd, as it's not actually one of `BasicApplication`'s config keys; I guess it doesn't matter what the key is, just that it's value is an `EntitySpec`? Maybe use "foo" instead, to clarify that it's not important.
    * Where does the actual stack overflow occur? In the *second* `addCatalogItems` call? That's just weird.
    
    A sprinkling of comments will guide the tester to what's important.


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36290218
  
    --- Diff: utils/common/src/main/java/brooklyn/util/collections/CollectionFunctionals.java ---
    @@ -80,6 +80,21 @@ public Integer get() {
             @Override public String toString() { return "sizeSupplier("+collection+")"; }
         }
     
    +    private static final class SizeSupplierFromSupplier implements Supplier<Integer> {
    --- End diff --
    
    Too many `Supplier`s make my head hurt! Minor naming nitpick to clarify: `SizeSupplierFromIterableSupplier` (adds `Iterable`).


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#issuecomment-128310365
  
    Great set of improvements, @aledsage. A handful of comments from me, but nothing major. +1 to Sam's suggestion of dynamically enabling feeds, but would happily see that in a separate PR.


---
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: Performance improvements and othe...

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

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


---
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: Performance improvements and othe...

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/790#discussion_r36770933
  
    --- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java ---
    @@ -2664,6 +2664,59 @@ private String getPublicHostnameAws(HostAndPort sshHostAndPort, ConfigBag setup)
             }
         }
     
    +    /**
    +     * Attempts to obtain the private hostname or IP of the node, as advertised by the cloud provider.
    +     * 
    +     * For some clouds (e.g. aws-ec2), it will attempt to find the hostname (as that works in public+private).
    +     */
    +    protected String getPrivateHostname(NodeMetadata node, Optional<HostAndPort> sshHostAndPort, ConfigBag setup) {
    +        String provider = (setup != null) ? setup.get(CLOUD_PROVIDER) : null;
    +        if (provider == null) provider= getProvider();
    +
    +        if ("aws-ec2".equals(provider)) {
    +            // TODO Remove duplication from getPublicHostname
    +            HostAndPort inferredHostAndPort = null;
    +            if (!sshHostAndPort.isPresent()) {
    +                try {
    +                    String vmIp = JcloudsUtil.getFirstReachableAddress(this.getComputeService().getContext(), node);
    +                    int port = node.getLoginPort();
    +                    inferredHostAndPort = HostAndPort.fromParts(vmIp, port);
    +                } catch (Exception e) {
    +                    LOG.warn("Error reaching aws-ec2 instance "+node.getId()+"@"+node.getLocation()+" on port "+node.getLoginPort()+"; falling back to jclouds metadata for address", e);
    +                }
    +            }
    +            if (sshHostAndPort.isPresent() || inferredHostAndPort != null) {
    +                HostAndPort hostAndPortToUse = sshHostAndPort.isPresent() ? sshHostAndPort.get() : inferredHostAndPort;
    +                try {
    +                    return getPublicHostnameAws(hostAndPortToUse, setup);
    +                } catch (Exception e) {
    +                    LOG.warn("Error querying aws-ec2 instance instance "+node.getId()+"@"+node.getLocation()+" over ssh for its hostname; falling back to jclouds metadata for address", e);
    +                }
    +            }
    +        }
    +
    +        return getPrivateHostnameGeneric(node, setup);
    +    }
    +
    +    private String getPrivateHostnameGeneric(NodeMetadata node, @Nullable ConfigBag setup) {
    +        //prefer the private address to the hostname because hostname is sometimes wrong/abbreviated
    +        //(see that javadoc; also e.g. on rackspace/cloudstack, the hostname lacks the domain).
    --- End diff --
    
    Changing comment to:
    
            //(see that javadoc; also e.g. on rackspace/cloudstack, the hostname is not registered with any DNS).



---
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: Performance improvements and othe...

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/790#discussion_r36766616
  
    --- Diff: software/webapp/src/test/java/brooklyn/test/entity/TestJavaWebAppEntity.java ---
    @@ -38,7 +39,7 @@
      * Mock web application server entity for testing.
      */
     @ImplementedBy(TestJavaWebAppEntityImpl.class)
    -public interface TestJavaWebAppEntity extends VanillaJavaApp, WebAppService {
    +public interface TestJavaWebAppEntity extends VanillaJavaApp, WebAppService, EntityLocal {
    --- End diff --
    
    Makes tests a little simpler, because don't need to cast to `EntityLocal` for methods like `setAttribute(...)`. It would be nice to get rid of `EntityLocal` entirely!


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36397160
  
    --- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java ---
    @@ -2664,6 +2664,59 @@ private String getPublicHostnameAws(HostAndPort sshHostAndPort, ConfigBag setup)
             }
         }
     
    +    /**
    +     * Attempts to obtain the private hostname or IP of the node, as advertised by the cloud provider.
    +     * 
    +     * For some clouds (e.g. aws-ec2), it will attempt to find the hostname (as that works in public+private).
    +     */
    +    protected String getPrivateHostname(NodeMetadata node, Optional<HostAndPort> sshHostAndPort, ConfigBag setup) {
    +        String provider = (setup != null) ? setup.get(CLOUD_PROVIDER) : null;
    +        if (provider == null) provider= getProvider();
    +
    +        if ("aws-ec2".equals(provider)) {
    --- End diff --
    
    Might be tidier to encapsulate the AWS-specific steps in a separate private method alongside `getPrivateHostnameGeneric`.


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36395446
  
    --- Diff: software/base/src/main/java/brooklyn/event/feed/jmx/JmxFeed.java ---
    @@ -207,6 +208,7 @@ protected JmxFeed(Builder builder) {
             
             SetMultimap<List<?>, JmxOperationPollConfig<?>> operationPolls = HashMultimap.<List<?>,JmxOperationPollConfig<?>>create();
             for (JmxOperationPollConfig<?> config : builder.operationPolls) {
    +            if (!config.isEnabled()) continue;
    --- End diff --
    
    As above (easy to forget).


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36289936
  
    --- Diff: core/src/test/java/brooklyn/test/entity/RecordingSensorEventListener.java ---
    @@ -0,0 +1,137 @@
    +/*
    + * 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.test.entity;
    +
    +import static com.google.common.base.Preconditions.checkNotNull;
    +
    +import java.util.Collection;
    +import java.util.Collections;
    +import java.util.List;
    +
    +import com.google.common.base.Objects;
    +import com.google.common.base.Supplier;
    +import com.google.common.collect.ArrayListMultimap;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.Iterables;
    +import com.google.common.collect.Lists;
    +import com.google.common.collect.Multimap;
    +import com.google.common.collect.Multimaps;
    +
    +import brooklyn.entity.Entity;
    +import brooklyn.event.Sensor;
    +import brooklyn.event.SensorEvent;
    +import brooklyn.event.SensorEventListener;
    +
    +public class RecordingSensorEventListener<T> implements SensorEventListener<T> {
    +    
    +    private final boolean skipDuplicateValues;
    +    private final List<SensorEvent<T>> events = Collections.synchronizedList(Lists.<SensorEvent<T>>newArrayList());
    +    private final List<T> values = Collections.synchronizedList(Lists.<T>newArrayList());
    +    private final Multimap<EntityAndSensor<?>, SensorEvent<T>> eventsBySource = 
    +            Multimaps.synchronizedListMultimap(ArrayListMultimap.<EntityAndSensor<?>, SensorEvent<T>>create());
    +    private final Multimap<EntityAndSensor<?>, T> valuesBySource = 
    +            Multimaps.synchronizedListMultimap(ArrayListMultimap.<EntityAndSensor<?>, T>create());
    +
    +    public RecordingSensorEventListener() {
    +        this(false);
    +    }
    +    
    +    public RecordingSensorEventListener(boolean skipDuplicateValues) {
    +        this.skipDuplicateValues = skipDuplicateValues;
    +    }
    +    
    +    @Override
    +    public void onEvent(SensorEvent<T> event) {
    +        EntityAndSensor<?> source = new EntityAndSensor<T>(event.getSource(),  event.getSensor());
    +        Collection<T> previousValues = valuesBySource.get(source);
    +        if (skipDuplicateValues && !previousValues.isEmpty() && Objects.equal(Iterables.get(previousValues, previousValues.size()-1), event.getValue())) {
    +            // skip
    +        } else {
    +            events.add(event);
    +            values.add(event.getValue());
    +            eventsBySource.put(source, event);
    +            valuesBySource.put(source, event.getValue());
    +        }
    +    }
    +    
    +    public void clear() {
    +        events.clear();
    +        eventsBySource.clear();
    +        valuesBySource.clear();
    +    }
    +
    +    public List<SensorEvent<T>> getEvents() {
    +        return ImmutableList.copyOf(events);
    +    }
    +    
    +    public List<T> getValues(Entity entity, Sensor<?> sensor) {
    +        return ImmutableList.copyOf(valuesBySource.get(new EntityAndSensor(entity, sensor)));
    +    }
    +    
    +    public List<T> getAllValues() {
    +        return ImmutableList.copyOf(values);
    +    }
    +    
    +    public Supplier<List<SensorEvent<T>>> getEventsSupplier() {
    --- End diff --
    
    Seemed an odd thing to include until I spotted `Asserts.eventually`, which takes a `Supplier`. Definitely worth a `@see` link.


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36292073
  
    --- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsSshMachineLocation.java ---
    @@ -166,7 +166,7 @@ public String getHostname() {
         /** In most clouds, the public hostname is the only way to ensure VMs in different zones can access each other. */
         @Override
         public String getSubnetHostname() {
    -        String publicHostname = jcloudsParent.getPublicHostname(node, Optional.<HostAndPort>absent(), config().getBag());
    +        String publicHostname = jcloudsParent.getPrivateHostname(node, Optional.<HostAndPort>absent(), config().getBag());
    --- End diff --
    
    Rename variable!


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36293224
  
    --- Diff: core/src/test/java/brooklyn/event/feed/http/HttpValueFunctionsTest.java ---
    @@ -0,0 +1,82 @@
    +/*
    + * 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.event.feed.http;
    +
    +import static org.testng.Assert.assertEquals;
    +import static org.testng.Assert.assertFalse;
    +import static org.testng.Assert.assertTrue;
    +
    +import org.testng.annotations.BeforeMethod;
    +import org.testng.annotations.Test;
    +
    +import brooklyn.util.http.HttpToolResponse;
    +
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.ImmutableMap;
    +import com.google.gson.JsonElement;
    +import com.google.gson.JsonPrimitive;
    +
    +public class HttpValueFunctionsTest {
    +
    +    private int responseCode = 200;
    +    private long fullLatency = 1000;
    +    private String headerName = "my_header";
    +    private String body = "{mykey:myvalue}";
    +    private long now;
    +    private HttpToolResponse response;
    +    
    +    @BeforeMethod
    +    public void setUp() throws Exception {
    +        now = System.currentTimeMillis();
    +        response = new HttpToolResponse(responseCode, ImmutableMap.of(headerName, ImmutableList.of("myvalue")), body.getBytes(), now-fullLatency, fullLatency/2, fullLatency);
    --- End diff --
    
    Whitespace around the binary operators, please! (pet peeve)


---
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: Performance improvements and othe...

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/790#discussion_r36417372
  
    --- Diff: core/src/test/java/brooklyn/test/entity/RecordingSensorEventListener.java ---
    @@ -0,0 +1,137 @@
    +/*
    + * 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.test.entity;
    --- End diff --
    
    @sjcorbett has done this in https://github.com/apache/incubator-brooklyn/pull/798 - let's merge this version (as it's simpler than mine!), and I'll delete my commit.


---
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: Performance improvements and othe...

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/790#discussion_r36765868
  
    --- Diff: core/src/main/java/brooklyn/event/feed/AttributePollHandler.java ---
    @@ -52,6 +54,7 @@
         @SuppressWarnings("rawtypes")
         private final AttributeSensor sensor;
         private final AbstractFeed feed;
    +    private final boolean suppressDuplicates;
    --- End diff --
    
    A couple of reasons: first (lame) it's preserving backwards compatibility; second for performance-related metrics then values such as "total requests processed" matter - there are enrichers that calculate the diff between recent values to determine the average requests per second.
    
    I suggest you take it to the mailing list for changing the default behaviour to always-suppress before we change that.


---
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: Performance improvements and othe...

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/790#discussion_r36770851
  
    --- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java ---
    @@ -2664,6 +2664,59 @@ private String getPublicHostnameAws(HostAndPort sshHostAndPort, ConfigBag setup)
             }
         }
     
    +    /**
    +     * Attempts to obtain the private hostname or IP of the node, as advertised by the cloud provider.
    +     * 
    +     * For some clouds (e.g. aws-ec2), it will attempt to find the hostname (as that works in public+private).
    +     */
    +    protected String getPrivateHostname(NodeMetadata node, Optional<HostAndPort> sshHostAndPort, ConfigBag setup) {
    +        String provider = (setup != null) ? setup.get(CLOUD_PROVIDER) : null;
    +        if (provider == null) provider= getProvider();
    +
    +        if ("aws-ec2".equals(provider)) {
    +            // TODO Remove duplication from getPublicHostname
    +            HostAndPort inferredHostAndPort = null;
    +            if (!sshHostAndPort.isPresent()) {
    +                try {
    +                    String vmIp = JcloudsUtil.getFirstReachableAddress(this.getComputeService().getContext(), node);
    +                    int port = node.getLoginPort();
    +                    inferredHostAndPort = HostAndPort.fromParts(vmIp, port);
    +                } catch (Exception e) {
    +                    LOG.warn("Error reaching aws-ec2 instance "+node.getId()+"@"+node.getLocation()+" on port "+node.getLoginPort()+"; falling back to jclouds metadata for address", e);
    +                }
    +            }
    +            if (sshHostAndPort.isPresent() || inferredHostAndPort != null) {
    +                HostAndPort hostAndPortToUse = sshHostAndPort.isPresent() ? sshHostAndPort.get() : inferredHostAndPort;
    +                try {
    +                    return getPublicHostnameAws(hostAndPortToUse, setup);
    +                } catch (Exception e) {
    +                    LOG.warn("Error querying aws-ec2 instance instance "+node.getId()+"@"+node.getLocation()+" over ssh for its hostname; falling back to jclouds metadata for address", e);
    +                }
    +            }
    +        }
    +
    +        return getPrivateHostnameGeneric(node, setup);
    +    }
    +
    +    private String getPrivateHostnameGeneric(NodeMetadata node, @Nullable ConfigBag setup) {
    +        //prefer the private address to the hostname because hostname is sometimes wrong/abbreviated
    +        //(see that javadoc; also e.g. on rackspace/cloudstack, the hostname lacks the domain).
    --- End diff --
    
    Good point! Our "hostname" attribute is a bit messed up. I think some things might treat it as a fully qualified name.
    
    Not good enough to just rename this method, as then it contradicts the other public method(s) like `JcloudsSshMachineLocation.getSubnetHostname()` (which is named similar to `JcloudsSshMachineLocation.getHostname()`, and might end up populating `Attributes.HOSTNAME`.
    
    It is that sensor that hostname sensor that I think might get used as a FQDN, but not sure.


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36293339
  
    --- Diff: core/src/test/java/brooklyn/event/feed/http/HttpValueFunctionsTest.java ---
    @@ -0,0 +1,82 @@
    +/*
    + * 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.event.feed.http;
    +
    +import static org.testng.Assert.assertEquals;
    +import static org.testng.Assert.assertFalse;
    +import static org.testng.Assert.assertTrue;
    +
    +import org.testng.annotations.BeforeMethod;
    +import org.testng.annotations.Test;
    +
    +import brooklyn.util.http.HttpToolResponse;
    +
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.ImmutableMap;
    +import com.google.gson.JsonElement;
    +import com.google.gson.JsonPrimitive;
    +
    +public class HttpValueFunctionsTest {
    +
    +    private int responseCode = 200;
    +    private long fullLatency = 1000;
    +    private String headerName = "my_header";
    +    private String body = "{mykey:myvalue}";
    +    private long now;
    +    private HttpToolResponse response;
    +    
    +    @BeforeMethod
    +    public void setUp() throws Exception {
    +        now = System.currentTimeMillis();
    +        response = new HttpToolResponse(responseCode, ImmutableMap.of(headerName, ImmutableList.of("myvalue")), body.getBytes(), now-fullLatency, fullLatency/2, fullLatency);
    +    }
    +    
    +    @Test
    +    public void testResponseCode() throws Exception {
    +        assertEquals(HttpValueFunctions.responseCode().apply(response), Integer.valueOf(responseCode));
    +    }
    +
    +    @Test
    +    public void testContainsHeader() throws Exception {
    +        assertTrue(HttpValueFunctions.containsHeader(headerName).apply(response));
    +        assertFalse(HttpValueFunctions.containsHeader("wrong_header").apply(response));
    +    }
    +    
    +    @Test
    +    public void testStringContents() throws Exception {
    +        assertEquals(HttpValueFunctions.stringContentsFunction().apply(response), body);
    +    }
    +
    +    @Test
    +    public void testJsonContents() throws Exception {
    +        JsonElement json = HttpValueFunctions.jsonContents().apply(response);
    +        assertTrue(json.isJsonObject());
    +        assertEquals(json.getAsJsonObject().entrySet(), ImmutableMap.of("mykey", new JsonPrimitive("myvalue")).entrySet());
    --- End diff --
    
    Nitpick: promote magic literals to fields, construct `body` field with reference to them.


---
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: Performance improvements and othe...

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/790#discussion_r36766750
  
    --- Diff: core/src/main/java/brooklyn/entity/basic/Attributes.java ---
    @@ -114,6 +115,15 @@
             "service.problems", 
             "A map of namespaced indicators of problems with a service");
     
    +    /**
    +     * @since 0.8.0
    +     */
    +    @SuppressWarnings("serial")
    --- End diff --
    
    Personally: for TypeToken, I think yes. We're not relying on java serialization anyway.


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#issuecomment-130061843
  
    Thanks @alasdairhodge @sjcorbett for the review - incorporated almost all your comments. The things that remain are:
    
    * The `if (!config.isEnabled()) continue;` check in the feeds.
    * Should we have a global enabled option on the feed itself to cater for these cases where all pollAttributes are gated on the same condition?
    * dynamically update SoftwareProcess.RETRIEVE_USAGE_METRICS on an entity and have its feeds enabled or disabled automatically.
    
    I think our feeds need to be revisited. They were written to be (relatively) simple - they assume that the configuration is static. To cope with dynamic changes, we'll need to ensure that poll period etc will automatically reconfigure on-the-fly as things are enabled/disabled or added/removed.
    
    Let's not address these in this PR. It warrants discussion on the mailing list.
    
    Merging now.


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36395438
  
    --- Diff: software/base/src/main/java/brooklyn/event/feed/jmx/JmxFeed.java ---
    @@ -198,6 +198,7 @@ protected JmxFeed(Builder builder) {
             
             SetMultimap<String, JmxAttributePollConfig<?>> attributePolls = HashMultimap.<String,JmxAttributePollConfig<?>>create();
             for (JmxAttributePollConfig<?> config : builder.attributePolls) {
    +            if (!config.isEnabled()) continue;
    --- End diff --
    
    As above (easy to forget).


---
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: Performance improvements and othe...

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/790#discussion_r36795378
  
    --- Diff: software/base/src/main/java/brooklyn/event/feed/jmx/JmxFeed.java ---
    @@ -198,6 +198,7 @@ protected JmxFeed(Builder builder) {
             
             SetMultimap<String, JmxAttributePollConfig<?>> attributePolls = HashMultimap.<String,JmxAttributePollConfig<?>>create();
             for (JmxAttributePollConfig<?> config : builder.attributePolls) {
    +            if (!config.isEnabled()) continue;
    --- End diff --
    
    These are tricky. Not sure I follow your suggestion of "adding `builder.enabledPolls`"? If we put the config into `attributePolls`, then `preStart()` would need to do the filtering. For example, in `registerAttributePoller` it would need to guard the call to `getPoller().scheduleAtFixedRate` if they were all disabled and ensure the handler(s) didn't get called. It would also need to set minPeriod appropriately (i.e. miss out the value of disabled poll configs).


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36393997
  
    --- Diff: software/webapp/src/test/java/brooklyn/test/entity/TestJavaWebAppEntity.java ---
    @@ -38,7 +39,7 @@
      * Mock web application server entity for testing.
      */
     @ImplementedBy(TestJavaWebAppEntityImpl.class)
    -public interface TestJavaWebAppEntity extends VanillaJavaApp, WebAppService {
    +public interface TestJavaWebAppEntity extends VanillaJavaApp, WebAppService, EntityLocal {
    --- End diff --
    
    Just curious: why was this necessary?


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36396819
  
    --- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java ---
    @@ -2664,6 +2664,59 @@ private String getPublicHostnameAws(HostAndPort sshHostAndPort, ConfigBag setup)
             }
         }
     
    +    /**
    +     * Attempts to obtain the private hostname or IP of the node, as advertised by the cloud provider.
    +     * 
    +     * For some clouds (e.g. aws-ec2), it will attempt to find the hostname (as that works in public+private).
    +     */
    +    protected String getPrivateHostname(NodeMetadata node, Optional<HostAndPort> sshHostAndPort, ConfigBag setup) {
    +        String provider = (setup != null) ? setup.get(CLOUD_PROVIDER) : null;
    +        if (provider == null) provider= getProvider();
    +
    +        if ("aws-ec2".equals(provider)) {
    --- End diff --
    
    This smells. I can live with it as a workaround in this one exceptional case, because the cost of refactoring to introduce a provider-agnostic abstraction is high. Let's not allow the special cases to proliferate though. Please add a comment to discourage others from following this example.


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36288309
  
    --- Diff: software/webapp/src/main/java/brooklyn/entity/proxy/nginx/NginxControllerImpl.java ---
    @@ -117,17 +118,24 @@ public void connectSensors() {
                     .poll(new HttpPollConfig<Boolean>(NGINX_URL_ANSWERS_NICELY)
                             // Any response from Nginx is good.
                             .checkSuccess(Predicates.alwaysTrue())
    -                        .onResult(new Function<HttpToolResponse, Boolean>() {
    -                                @Override
    -                                public Boolean apply(HttpToolResponse input) {
    -                                    // Accept any nginx response (don't assert specific version), so that sub-classing
    -                                    // for a custom nginx build is not strict about custom version numbers in headers
    -                                    List<String> actual = input.getHeaderLists().get("Server");
    -                                    return actual != null && actual.size() == 1;
    -                                }})
    -                        .setOnException(false))
    +                        // Accept any nginx response (don't assert specific version), so that sub-classing
    +                        // for a custom nginx build is not strict about custom version numbers in headers
    +                        .onResult(HttpValueFunctions.containsHeader("Server"))
    +                        .setOnException(false)
    +                        .suppressDuplicates(true))
                     .build());
             
    +        // TODO kept anonymous function in case referenced in persisted state
    --- End diff --
    
    Argh, I hate this, but realise it's necessary for the time being; looking forward to a long-term solution! Perhaps we should add easily-searchable markers (as is done for `BROOKLYN_VERSION`), something like: `/* PERSISTENCE WORKAROUND */`?


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36395223
  
    --- Diff: sandbox/extra/src/main/java/brooklyn/entity/database/postgresql/PostgreSqlNodeSaltImpl.java ---
    @@ -174,4 +175,8 @@ public String executeScript(String commands) {
                     ConfigBag.newInstance().configure(ExecuteScriptEffectorBody.SCRIPT, commands).getAllConfig()).getUnchecked();
         }
     
    +    @Override
    +    public void populateServiceNotUpDiagnostics() {
    +        // FIXME no-op currently; should check ssh'able etc
    +    }
    --- End diff --
    
    `FIXME` is a bit strong. This surely applies to many other entities, so highlighting this case seems a bit random ;o)


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36395477
  
    --- Diff: core/src/main/java/brooklyn/event/feed/function/FunctionFeed.java ---
    @@ -175,6 +175,7 @@ protected FunctionFeed(Builder builder) {
             
             SetMultimap<FunctionPollIdentifier, FunctionPollConfig<?,?>> polls = HashMultimap.<FunctionPollIdentifier,FunctionPollConfig<?,?>>create();
             for (FunctionPollConfig<?,?> config : builder.polls) {
    +            if (!config.isEnabled()) continue;
    --- End diff --
    
    As above (easy to forget).


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36395513
  
    --- Diff: core/src/main/java/brooklyn/event/feed/windows/WindowsPerformanceCounterFeed.java ---
    @@ -169,6 +169,7 @@ public WindowsPerformanceCounterFeed() {
         protected WindowsPerformanceCounterFeed(Builder builder) {
             List<WindowsPerformanceCounterPollConfig<?>> polls = Lists.newArrayList();
             for (WindowsPerformanceCounterPollConfig<?> config : builder.polls) {
    +            if (!config.isEnabled()) continue;
    --- End diff --
    
    As above (easy to forget).


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36395370
  
    --- Diff: sandbox/monitoring/src/main/java/brooklyn/entity/monitoring/zabbix/ZabbixFeed.java ---
    @@ -305,6 +305,7 @@ protected ZabbixFeed(final Builder<? extends ZabbixFeed, ?> builder) {
     
             Set<ZabbixPollConfig<?>> polls = Sets.newLinkedHashSet();
             for (ZabbixPollConfig<?> config : builder.polls) {
    +            if (!config.isEnabled()) continue;
    --- End diff --
    
    This seems like the sort of thing that's easy to forget. Worth adding `builder.enabledPolls`?


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36395488
  
    --- Diff: core/src/main/java/brooklyn/event/feed/http/HttpFeed.java ---
    @@ -279,6 +279,7 @@ protected HttpFeed(Builder builder) {
             
             SetMultimap<HttpPollIdentifier, HttpPollConfig<?>> polls = HashMultimap.<HttpPollIdentifier,HttpPollConfig<?>>create();
             for (HttpPollConfig<?> config : builder.polls) {
    +            if (!config.isEnabled()) continue;
    --- End diff --
    
    As above (easy to forget).


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36289561
  
    --- Diff: core/src/test/java/brooklyn/test/entity/RecordingSensorEventListener.java ---
    @@ -0,0 +1,137 @@
    +/*
    + * 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.test.entity;
    +
    +import static com.google.common.base.Preconditions.checkNotNull;
    +
    +import java.util.Collection;
    +import java.util.Collections;
    +import java.util.List;
    +
    +import com.google.common.base.Objects;
    +import com.google.common.base.Supplier;
    +import com.google.common.collect.ArrayListMultimap;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.Iterables;
    +import com.google.common.collect.Lists;
    +import com.google.common.collect.Multimap;
    +import com.google.common.collect.Multimaps;
    +
    +import brooklyn.entity.Entity;
    +import brooklyn.event.Sensor;
    +import brooklyn.event.SensorEvent;
    +import brooklyn.event.SensorEventListener;
    +
    +public class RecordingSensorEventListener<T> implements SensorEventListener<T> {
    +    
    +    private final boolean skipDuplicateValues;
    +    private final List<SensorEvent<T>> events = Collections.synchronizedList(Lists.<SensorEvent<T>>newArrayList());
    +    private final List<T> values = Collections.synchronizedList(Lists.<T>newArrayList());
    +    private final Multimap<EntityAndSensor<?>, SensorEvent<T>> eventsBySource = 
    +            Multimaps.synchronizedListMultimap(ArrayListMultimap.<EntityAndSensor<?>, SensorEvent<T>>create());
    +    private final Multimap<EntityAndSensor<?>, T> valuesBySource = 
    +            Multimaps.synchronizedListMultimap(ArrayListMultimap.<EntityAndSensor<?>, T>create());
    +
    +    public RecordingSensorEventListener() {
    +        this(false);
    +    }
    +    
    +    public RecordingSensorEventListener(boolean skipDuplicateValues) {
    +        this.skipDuplicateValues = skipDuplicateValues;
    +    }
    +    
    +    @Override
    +    public void onEvent(SensorEvent<T> event) {
    +        EntityAndSensor<?> source = new EntityAndSensor<T>(event.getSource(),  event.getSensor());
    +        Collection<T> previousValues = valuesBySource.get(source);
    +        if (skipDuplicateValues && !previousValues.isEmpty() && Objects.equal(Iterables.get(previousValues, previousValues.size()-1), event.getValue())) {
    --- End diff --
    
    This check isn't thread safe, but perhaps that doesn't matter since tests will be run single-threaded.


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36395901
  
    --- Diff: core/src/test/java/brooklyn/test/entity/RecordingSensorEventListener.java ---
    @@ -0,0 +1,137 @@
    +/*
    + * 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.test.entity;
    +
    +import static com.google.common.base.Preconditions.checkNotNull;
    +
    +import java.util.Collection;
    +import java.util.Collections;
    +import java.util.List;
    +
    +import com.google.common.base.Objects;
    +import com.google.common.base.Supplier;
    +import com.google.common.collect.ArrayListMultimap;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.Iterables;
    +import com.google.common.collect.Lists;
    +import com.google.common.collect.Multimap;
    +import com.google.common.collect.Multimaps;
    +
    +import brooklyn.entity.Entity;
    +import brooklyn.event.Sensor;
    +import brooklyn.event.SensorEvent;
    +import brooklyn.event.SensorEventListener;
    +
    +public class RecordingSensorEventListener<T> implements SensorEventListener<T> {
    --- End diff --
    
    What a coincidence: https://github.com/apache/incubator-brooklyn/pull/798/files#diff-00ca0a99710c783a4fc656c789e26070


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36288011
  
    --- Diff: software/webapp/src/main/java/brooklyn/entity/dns/geoscaling/GeoscalingWebClient.java ---
    @@ -176,7 +176,8 @@ public int hashCode() {
         
         
         public GeoscalingWebClient() {
    -        this(new DefaultHttpClient());
    +        // TODO Don't want to trustAll!
    +        this(HttpTool.httpClientBuilder().trustAll().build());
    --- End diff --
    
    Revert this, given the change to `GeoscalingDnsServiceImpl`.


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36394879
  
    --- Diff: core/src/main/java/brooklyn/entity/basic/Attributes.java ---
    @@ -114,6 +115,15 @@
             "service.problems", 
             "A map of namespaced indicators of problems with a service");
     
    +    /**
    +     * @since 0.8.0
    +     */
    +    @SuppressWarnings("serial")
    --- End diff --
    
    Oh, this is acceptable? I got into the habit of just defining my `TypeToken`s with the version UUID inline.


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36395462
  
    --- Diff: software/base/src/main/java/brooklyn/event/feed/jmx/JmxFeed.java ---
    @@ -216,6 +218,7 @@ protected JmxFeed(Builder builder) {
             
             SetMultimap<NotificationFilter, JmxNotificationSubscriptionConfig<?>> notificationSubscriptions = HashMultimap.create();
             for (JmxNotificationSubscriptionConfig<?> config : builder.notificationSubscriptions) {
    +            if (!config.isEnabled()) continue;
    --- End diff --
    
    As above (easy to forget).


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36396507
  
    --- Diff: locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java ---
    @@ -2664,6 +2664,59 @@ private String getPublicHostnameAws(HostAndPort sshHostAndPort, ConfigBag setup)
             }
         }
     
    +    /**
    +     * Attempts to obtain the private hostname or IP of the node, as advertised by the cloud provider.
    +     * 
    +     * For some clouds (e.g. aws-ec2), it will attempt to find the hostname (as that works in public+private).
    +     */
    +    protected String getPrivateHostname(NodeMetadata node, Optional<HostAndPort> sshHostAndPort, ConfigBag setup) {
    +        String provider = (setup != null) ? setup.get(CLOUD_PROVIDER) : null;
    +        if (provider == null) provider= getProvider();
    +
    +        if ("aws-ec2".equals(provider)) {
    +            // TODO Remove duplication from getPublicHostname
    +            HostAndPort inferredHostAndPort = null;
    +            if (!sshHostAndPort.isPresent()) {
    +                try {
    +                    String vmIp = JcloudsUtil.getFirstReachableAddress(this.getComputeService().getContext(), node);
    +                    int port = node.getLoginPort();
    +                    inferredHostAndPort = HostAndPort.fromParts(vmIp, port);
    +                } catch (Exception e) {
    +                    LOG.warn("Error reaching aws-ec2 instance "+node.getId()+"@"+node.getLocation()+" on port "+node.getLoginPort()+"; falling back to jclouds metadata for address", e);
    +                }
    +            }
    +            if (sshHostAndPort.isPresent() || inferredHostAndPort != null) {
    +                HostAndPort hostAndPortToUse = sshHostAndPort.isPresent() ? sshHostAndPort.get() : inferredHostAndPort;
    +                try {
    +                    return getPublicHostnameAws(hostAndPortToUse, setup);
    +                } catch (Exception e) {
    +                    LOG.warn("Error querying aws-ec2 instance instance "+node.getId()+"@"+node.getLocation()+" over ssh for its hostname; falling back to jclouds metadata for address", e);
    +                }
    +            }
    +        }
    +
    +        return getPrivateHostnameGeneric(node, setup);
    +    }
    +
    +    private String getPrivateHostnameGeneric(NodeMetadata node, @Nullable ConfigBag setup) {
    +        //prefer the private address to the hostname because hostname is sometimes wrong/abbreviated
    +        //(see that javadoc; also e.g. on rackspace/cloudstack, the hostname lacks the domain).
    --- End diff --
    
    One shouldn't expect a hostname to contain a domain part!


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36293770
  
    --- Diff: core/src/main/java/brooklyn/event/feed/AttributePollHandler.java ---
    @@ -52,6 +54,7 @@
         @SuppressWarnings("rawtypes")
         private final AttributeSensor sensor;
         private final AbstractFeed feed;
    +    private final boolean suppressDuplicates;
    --- End diff --
    
    What are the compelling reasons to ever *include* duplicates? Should they be suppressed by default, with a config option to explicitly enable them?


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36289537
  
    --- Diff: core/src/test/java/brooklyn/test/entity/RecordingSensorEventListener.java ---
    @@ -0,0 +1,137 @@
    +/*
    + * 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.test.entity;
    --- End diff --
    
    Should this class go in `brooklyn-test-support` for use in other projects?


---
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: Performance improvements and othe...

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/790#discussion_r36763131
  
    --- Diff: software/webapp/src/main/java/brooklyn/entity/proxy/nginx/NginxControllerImpl.java ---
    @@ -117,17 +118,24 @@ public void connectSensors() {
                     .poll(new HttpPollConfig<Boolean>(NGINX_URL_ANSWERS_NICELY)
                             // Any response from Nginx is good.
                             .checkSuccess(Predicates.alwaysTrue())
    -                        .onResult(new Function<HttpToolResponse, Boolean>() {
    -                                @Override
    -                                public Boolean apply(HttpToolResponse input) {
    -                                    // Accept any nginx response (don't assert specific version), so that sub-classing
    -                                    // for a custom nginx build is not strict about custom version numbers in headers
    -                                    List<String> actual = input.getHeaderLists().get("Server");
    -                                    return actual != null && actual.size() == 1;
    -                                }})
    -                        .setOnException(false))
    +                        // Accept any nginx response (don't assert specific version), so that sub-classing
    +                        // for a custom nginx build is not strict about custom version numbers in headers
    +                        .onResult(HttpValueFunctions.containsHeader("Server"))
    +                        .setOnException(false)
    +                        .suppressDuplicates(true))
                     .build());
             
    +        // TODO kept anonymous function in case referenced in persisted state
    --- End diff --
    
    Will add a marker here (in the TODO comment) that we can search for.


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36289959
  
    --- Diff: core/src/test/java/brooklyn/test/entity/RecordingSensorEventListener.java ---
    @@ -0,0 +1,137 @@
    +/*
    + * 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.test.entity;
    +
    +import static com.google.common.base.Preconditions.checkNotNull;
    +
    +import java.util.Collection;
    +import java.util.Collections;
    +import java.util.List;
    +
    +import com.google.common.base.Objects;
    +import com.google.common.base.Supplier;
    +import com.google.common.collect.ArrayListMultimap;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.Iterables;
    +import com.google.common.collect.Lists;
    +import com.google.common.collect.Multimap;
    +import com.google.common.collect.Multimaps;
    +
    +import brooklyn.entity.Entity;
    +import brooklyn.event.Sensor;
    +import brooklyn.event.SensorEvent;
    +import brooklyn.event.SensorEventListener;
    +
    +public class RecordingSensorEventListener<T> implements SensorEventListener<T> {
    +    
    +    private final boolean skipDuplicateValues;
    +    private final List<SensorEvent<T>> events = Collections.synchronizedList(Lists.<SensorEvent<T>>newArrayList());
    +    private final List<T> values = Collections.synchronizedList(Lists.<T>newArrayList());
    +    private final Multimap<EntityAndSensor<?>, SensorEvent<T>> eventsBySource = 
    +            Multimaps.synchronizedListMultimap(ArrayListMultimap.<EntityAndSensor<?>, SensorEvent<T>>create());
    +    private final Multimap<EntityAndSensor<?>, T> valuesBySource = 
    +            Multimaps.synchronizedListMultimap(ArrayListMultimap.<EntityAndSensor<?>, T>create());
    +
    +    public RecordingSensorEventListener() {
    +        this(false);
    +    }
    +    
    +    public RecordingSensorEventListener(boolean skipDuplicateValues) {
    +        this.skipDuplicateValues = skipDuplicateValues;
    +    }
    +    
    +    @Override
    +    public void onEvent(SensorEvent<T> event) {
    +        EntityAndSensor<?> source = new EntityAndSensor<T>(event.getSource(),  event.getSensor());
    +        Collection<T> previousValues = valuesBySource.get(source);
    +        if (skipDuplicateValues && !previousValues.isEmpty() && Objects.equal(Iterables.get(previousValues, previousValues.size()-1), event.getValue())) {
    +            // skip
    +        } else {
    +            events.add(event);
    +            values.add(event.getValue());
    +            eventsBySource.put(source, event);
    +            valuesBySource.put(source, event.getValue());
    +        }
    +    }
    +    
    +    public void clear() {
    +        events.clear();
    +        eventsBySource.clear();
    +        valuesBySource.clear();
    +    }
    +
    +    public List<SensorEvent<T>> getEvents() {
    +        return ImmutableList.copyOf(events);
    +    }
    +    
    +    public List<T> getValues(Entity entity, Sensor<?> sensor) {
    +        return ImmutableList.copyOf(valuesBySource.get(new EntityAndSensor(entity, sensor)));
    +    }
    +    
    +    public List<T> getAllValues() {
    +        return ImmutableList.copyOf(values);
    +    }
    +    
    +    public Supplier<List<SensorEvent<T>>> getEventsSupplier() {
    +        return new Supplier<List<SensorEvent<T>>>() {
    +            @Override public List<SensorEvent<T>> get() {
    +                return getEvents();
    +            }};
    +    }
    +    
    +    public Supplier<List<T>> getAllValuesSupplier() {
    --- End diff --
    
    As above: seemed an odd thing to include until I spotted `Asserts.eventually`, which takes a `Supplier`. Definitely worth a `@see` link.


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#issuecomment-128309470
  
    It would be excellent if we could dynamically update `SoftwareProcess.RETRIEVE_USAGE_METRICS` on an entity and have its feeds enabled or disabled automatically. 



---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36395403
  
    --- Diff: software/base/src/main/java/brooklyn/entity/chef/ChefAttributeFeed.java ---
    @@ -199,6 +199,7 @@ protected ChefAttributeFeed(Builder builder) {
     
             Set<ChefAttributePollConfig<?>> polls = Sets.newLinkedHashSet();
             for (ChefAttributePollConfig<?> config : builder.polls) {
    +            if (!config.isEnabled()) continue;
    --- End diff --
    
    As above (easy to forget).


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36396115
  
    --- Diff: software/messaging/src/main/java/brooklyn/entity/messaging/kafka/KafkaBrokerImpl.java ---
    @@ -99,35 +100,43 @@ protected void connectSensors() {
                     .pollAttribute(new JmxAttributePollConfig<Long>(FETCH_REQUEST_COUNT)
                             .objectName(SOCKET_SERVER_STATS_MBEAN)
                             .attributeName("NumFetchRequests")
    -                        .onException(Functions.constant(-1l)))
    +                        .onException(Functions.constant(-1l))
    +                        .enabled(retrieveUsageMetrics))
                     .pollAttribute(new JmxAttributePollConfig<Long>(TOTAL_FETCH_TIME)
                             .objectName(SOCKET_SERVER_STATS_MBEAN)
                             .attributeName("TotalFetchRequestMs")
    -                        .onException(Functions.constant(-1l)))
    +                        .onException(Functions.constant(-1l))
    +                        .enabled(retrieveUsageMetrics))
                     .pollAttribute(new JmxAttributePollConfig<Double>(MAX_FETCH_TIME)
                             .objectName(SOCKET_SERVER_STATS_MBEAN)
                             .attributeName("MaxFetchRequestMs")
    -                        .onException(Functions.constant(-1.0d)))
    +                        .onException(Functions.constant(-1.0d))
    +                        .enabled(retrieveUsageMetrics))
                     .pollAttribute(new JmxAttributePollConfig<Long>(PRODUCE_REQUEST_COUNT)
                             .objectName(SOCKET_SERVER_STATS_MBEAN)
                             .attributeName("NumProduceRequests")
    -                        .onException(Functions.constant(-1l)))
    +                        .onException(Functions.constant(-1l))
    +                        .enabled(retrieveUsageMetrics))
                     .pollAttribute(new JmxAttributePollConfig<Long>(TOTAL_PRODUCE_TIME)
                             .objectName(SOCKET_SERVER_STATS_MBEAN)
                             .attributeName("TotalProduceRequestMs")
    -                        .onException(Functions.constant(-1l)))
    +                        .onException(Functions.constant(-1l))
    +                        .enabled(retrieveUsageMetrics))
                     .pollAttribute(new JmxAttributePollConfig<Double>(MAX_PRODUCE_TIME)
                             .objectName(SOCKET_SERVER_STATS_MBEAN)
                             .attributeName("MaxProduceRequestMs")
    -                        .onException(Functions.constant(-1.0d)))
    +                        .onException(Functions.constant(-1.0d))
    +                        .enabled(retrieveUsageMetrics))
                     .pollAttribute(new JmxAttributePollConfig<Long>(BYTES_RECEIVED)
                             .objectName(SOCKET_SERVER_STATS_MBEAN)
                             .attributeName("TotalBytesRead")
    -                        .onException(Functions.constant(-1l)))
    +                        .onException(Functions.constant(-1l))
    +                        .enabled(retrieveUsageMetrics))
                     .pollAttribute(new JmxAttributePollConfig<Long>(BYTES_SENT)
                             .objectName(SOCKET_SERVER_STATS_MBEAN)
                             .attributeName("TotalBytesWritten")
    -                        .onException(Functions.constant(-1l)))
    +                        .onException(Functions.constant(-1l))
    +                        .enabled(retrieveUsageMetrics))
    --- End diff --
    
    Should we have a global `enabled` option on the feed itself to cater for these cases where all `pollAttributes` are gated on the same condition?


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36289732
  
    --- Diff: core/src/test/java/brooklyn/test/entity/RecordingSensorEventListener.java ---
    @@ -0,0 +1,137 @@
    +/*
    + * 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.test.entity;
    +
    +import static com.google.common.base.Preconditions.checkNotNull;
    +
    +import java.util.Collection;
    +import java.util.Collections;
    +import java.util.List;
    +
    +import com.google.common.base.Objects;
    +import com.google.common.base.Supplier;
    +import com.google.common.collect.ArrayListMultimap;
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.Iterables;
    +import com.google.common.collect.Lists;
    +import com.google.common.collect.Multimap;
    +import com.google.common.collect.Multimaps;
    +
    +import brooklyn.entity.Entity;
    +import brooklyn.event.Sensor;
    +import brooklyn.event.SensorEvent;
    +import brooklyn.event.SensorEventListener;
    +
    +public class RecordingSensorEventListener<T> implements SensorEventListener<T> {
    +    
    +    private final boolean skipDuplicateValues;
    +    private final List<SensorEvent<T>> events = Collections.synchronizedList(Lists.<SensorEvent<T>>newArrayList());
    +    private final List<T> values = Collections.synchronizedList(Lists.<T>newArrayList());
    +    private final Multimap<EntityAndSensor<?>, SensorEvent<T>> eventsBySource = 
    +            Multimaps.synchronizedListMultimap(ArrayListMultimap.<EntityAndSensor<?>, SensorEvent<T>>create());
    +    private final Multimap<EntityAndSensor<?>, T> valuesBySource = 
    +            Multimaps.synchronizedListMultimap(ArrayListMultimap.<EntityAndSensor<?>, T>create());
    +
    +    public RecordingSensorEventListener() {
    +        this(false);
    +    }
    +    
    +    public RecordingSensorEventListener(boolean skipDuplicateValues) {
    +        this.skipDuplicateValues = skipDuplicateValues;
    +    }
    +    
    +    @Override
    +    public void onEvent(SensorEvent<T> event) {
    +        EntityAndSensor<?> source = new EntityAndSensor<T>(event.getSource(),  event.getSensor());
    +        Collection<T> previousValues = valuesBySource.get(source);
    +        if (skipDuplicateValues && !previousValues.isEmpty() && Objects.equal(Iterables.get(previousValues, previousValues.size()-1), event.getValue())) {
    --- End diff --
    
    Also also: worth clarifying that "duplicates" really means "consecutive duplicates".


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36293359
  
    --- Diff: core/src/test/java/brooklyn/event/feed/http/HttpValueFunctionsTest.java ---
    @@ -0,0 +1,82 @@
    +/*
    + * 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.event.feed.http;
    +
    +import static org.testng.Assert.assertEquals;
    +import static org.testng.Assert.assertFalse;
    +import static org.testng.Assert.assertTrue;
    +
    +import org.testng.annotations.BeforeMethod;
    +import org.testng.annotations.Test;
    +
    +import brooklyn.util.http.HttpToolResponse;
    +
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.ImmutableMap;
    +import com.google.gson.JsonElement;
    +import com.google.gson.JsonPrimitive;
    +
    +public class HttpValueFunctionsTest {
    +
    +    private int responseCode = 200;
    +    private long fullLatency = 1000;
    +    private String headerName = "my_header";
    +    private String body = "{mykey:myvalue}";
    +    private long now;
    +    private HttpToolResponse response;
    +    
    +    @BeforeMethod
    +    public void setUp() throws Exception {
    +        now = System.currentTimeMillis();
    +        response = new HttpToolResponse(responseCode, ImmutableMap.of(headerName, ImmutableList.of("myvalue")), body.getBytes(), now-fullLatency, fullLatency/2, fullLatency);
    +    }
    +    
    +    @Test
    +    public void testResponseCode() throws Exception {
    +        assertEquals(HttpValueFunctions.responseCode().apply(response), Integer.valueOf(responseCode));
    +    }
    +
    +    @Test
    +    public void testContainsHeader() throws Exception {
    +        assertTrue(HttpValueFunctions.containsHeader(headerName).apply(response));
    +        assertFalse(HttpValueFunctions.containsHeader("wrong_header").apply(response));
    +    }
    +    
    +    @Test
    +    public void testStringContents() throws Exception {
    +        assertEquals(HttpValueFunctions.stringContentsFunction().apply(response), body);
    +    }
    +
    +    @Test
    +    public void testJsonContents() throws Exception {
    +        JsonElement json = HttpValueFunctions.jsonContents().apply(response);
    +        assertTrue(json.isJsonObject());
    +        assertEquals(json.getAsJsonObject().entrySet(), ImmutableMap.of("mykey", new JsonPrimitive("myvalue")).entrySet());
    +    }
    +
    +    @Test
    +    public void testJsonContentsGettingElement() throws Exception {
    +        assertEquals(HttpValueFunctions.jsonContents("mykey", String.class).apply(response), "myvalue");
    --- End diff --
    
    As above: eliminate magic literals.


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36394348
  
    --- Diff: software/webapp/src/main/java/brooklyn/entity/webapp/ControlledDynamicWebAppCluster.java ---
    @@ -69,6 +69,10 @@
         public static BasicAttributeSensorAndConfigKey<LoadBalancer> CONTROLLER = new BasicAttributeSensorAndConfigKey<LoadBalancer>(
             LoadBalancer.class, "controlleddynamicwebappcluster.controller", "Controller for the cluster; if null a default will created (using controllerSpec)");
     
    +    @SetFromFlag("loadBalancedGroup")
    +    public static BasicAttributeSensorAndConfigKey<Group> LOAD_BALANCED_GROUP = new BasicAttributeSensorAndConfigKey<Group>(
    +        Group.class, "controlleddynamicwebappcluster.loadbalancedgroup", "The group of web servers that the controller should point at; if null, will use the CLUSTER");
    --- End diff --
    
    Maybe stick with consistent nomenclature: `CONTROLLED_GROUP`?


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36293578
  
    --- Diff: core/src/test/java/brooklyn/event/feed/http/HttpValueFunctionsTest.java ---
    @@ -0,0 +1,82 @@
    +/*
    + * 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.event.feed.http;
    +
    +import static org.testng.Assert.assertEquals;
    +import static org.testng.Assert.assertFalse;
    +import static org.testng.Assert.assertTrue;
    +
    +import org.testng.annotations.BeforeMethod;
    +import org.testng.annotations.Test;
    +
    +import brooklyn.util.http.HttpToolResponse;
    +
    +import com.google.common.collect.ImmutableList;
    +import com.google.common.collect.ImmutableMap;
    +import com.google.gson.JsonElement;
    +import com.google.gson.JsonPrimitive;
    +
    +public class HttpValueFunctionsTest {
    +
    +    private int responseCode = 200;
    +    private long fullLatency = 1000;
    +    private String headerName = "my_header";
    +    private String body = "{mykey:myvalue}";
    +    private long now;
    +    private HttpToolResponse response;
    +    
    +    @BeforeMethod
    +    public void setUp() throws Exception {
    +        now = System.currentTimeMillis();
    +        response = new HttpToolResponse(responseCode, ImmutableMap.of(headerName, ImmutableList.of("myvalue")), body.getBytes(), now-fullLatency, fullLatency/2, fullLatency);
    +    }
    +    
    +    @Test
    +    public void testResponseCode() throws Exception {
    +        assertEquals(HttpValueFunctions.responseCode().apply(response), Integer.valueOf(responseCode));
    +    }
    +
    +    @Test
    +    public void testContainsHeader() throws Exception {
    +        assertTrue(HttpValueFunctions.containsHeader(headerName).apply(response));
    +        assertFalse(HttpValueFunctions.containsHeader("wrong_header").apply(response));
    +    }
    +    
    +    @Test
    +    public void testStringContents() throws Exception {
    +        assertEquals(HttpValueFunctions.stringContentsFunction().apply(response), body);
    +    }
    +
    +    @Test
    +    public void testJsonContents() throws Exception {
    +        JsonElement json = HttpValueFunctions.jsonContents().apply(response);
    +        assertTrue(json.isJsonObject());
    +        assertEquals(json.getAsJsonObject().entrySet(), ImmutableMap.of("mykey", new JsonPrimitive("myvalue")).entrySet());
    +    }
    +
    +    @Test
    +    public void testJsonContentsGettingElement() throws Exception {
    +        assertEquals(HttpValueFunctions.jsonContents("mykey", String.class).apply(response), "myvalue");
    --- End diff --
    
    Add assert for absent value.


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36418577
  
    --- Diff: core/src/test/java/brooklyn/test/entity/RecordingSensorEventListener.java ---
    @@ -0,0 +1,137 @@
    +/*
    + * 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.test.entity;
    --- End diff --
    
    Good idea! I still say it should go into `brooklyn-test-support` though.


---
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: Performance improvements and othe...

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

    https://github.com/apache/incubator-brooklyn/pull/790#discussion_r36396340
  
    --- Diff: software/base/src/main/java/brooklyn/entity/basic/SameServerEntityImpl.java ---
    @@ -36,7 +38,8 @@
         @Override
         protected void initEnrichers() {
             super.initEnrichers();
    -        addEnricher(ServiceStateLogic.newEnricherFromChildren());
    +        addEnricher(ServiceStateLogic.newEnricherFromChildren()
    +                .configure(ComputeServiceIndicatorsFromChildrenAndMembers.UP_QUORUM_CHECK, QuorumCheck.QuorumChecks.all()));
    --- End diff --
    
    I had to hunt to find out why this is required; worth a brief comment.


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