You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by sjcorbett <gi...@git.apache.org> on 2016/10/13 10:33:12 UTC

[GitHub] brooklyn-server pull request #382: InvokeEffectorOnSetChange

GitHub user sjcorbett opened a pull request:

    https://github.com/apache/brooklyn-server/pull/382

    InvokeEffectorOnSetChange

    Tracks a collection and invokes effectors when elements are added and removed.

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

    $ git pull https://github.com/sjcorbett/brooklyn-server invoke-set-change

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

    https://github.com/apache/brooklyn-server/pull/382.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 #382
    
----
commit 8bbf68a07f86b06900fcd84cf9b6f71669f97bc5
Author: Sam Corbett <sa...@cloudsoftcorp.com>
Date:   2016-10-13T10:24:15Z

    getEffectorByName returns absent rather than throwing NPE when arg null.

commit 2797e0c23d53ae4e0e3900f8b92e117d1da92783
Author: Sam Corbett <sa...@cloudsoftcorp.com>
Date:   2016-10-13T10:24:35Z

    Add Sensors.newSensor(TypeToken, name)

commit f3017f6a3e129ebf026da89d98113657aa649c61
Author: Sam Corbett <sa...@cloudsoftcorp.com>
Date:   2016-10-13T10:32:43Z

    InvokeEffectorOnSetChange
    
    Tracks a collection and invokes effectors when elements are added and
    removed.

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #382: InvokeEffectorOnSetChange

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

    https://github.com/apache/brooklyn-server/pull/382#discussion_r83260337
  
    --- Diff: core/src/main/java/org/apache/brooklyn/policy/InvokeEffectorOnSetChange.java ---
    @@ -0,0 +1,203 @@
    +/*
    + * 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 org.apache.brooklyn.policy;
    +
    +import static com.google.common.base.Preconditions.checkArgument;
    +import static com.google.common.base.Preconditions.checkNotNull;
    +
    +import java.util.Collection;
    +import java.util.Collections;
    +import java.util.HashSet;
    +import java.util.Map;
    +import java.util.Set;
    +
    +import org.apache.brooklyn.api.effector.Effector;
    +import org.apache.brooklyn.api.entity.EntityLocal;
    +import org.apache.brooklyn.api.sensor.AttributeSensor;
    +import org.apache.brooklyn.api.sensor.Sensor;
    +import org.apache.brooklyn.api.sensor.SensorEvent;
    +import org.apache.brooklyn.api.sensor.SensorEventListener;
    +import org.apache.brooklyn.config.ConfigKey;
    +import org.apache.brooklyn.core.config.ConfigKeys;
    +import org.apache.brooklyn.core.policy.AbstractPolicy;
    +import org.apache.brooklyn.core.sensor.BasicAttributeSensor;
    +import org.apache.brooklyn.util.collections.MutableMap;
    +import org.apache.brooklyn.util.core.flags.TypeCoercions;
    +import org.apache.brooklyn.util.guava.Maybe;
    +import org.apache.brooklyn.util.text.Strings;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import com.google.common.collect.Sets;
    +import com.google.common.reflect.TypeToken;
    +
    +/**
    + * Subscribes to events on a collection {@link AttributeSensor} and invokes the named
    + * effectors for each element that was added and removed.
    + * <p>
    + * The policy only detects <em>replacements</em> of the collection; it does not act on
    + * modifications. If the sensor has value <code>A</code> and an element is added &ndash;
    + * value <code>A'</code> &ndash; the on-added effector is not invoked. If the sensor is
    + * later set to <code>B</code> the delta is made between <code>A</code> and <code>B</code>,
    + * not <code>A'</code> and <code>B</code>.
    + * <p>
    + * To simplify the detection of additions and removals the collection is converted to a
    + * {@link Set}. This means that only a single event will fire for duplicate elements in
    + * the collection.
    + * <p>
    + * The effectors are provided the elements that changed in their parameter map. If the
    + * sensor is a collection of maps the elements are provided with their keys coerced to
    + * strings and their values unchanged. Otherwise the elements are provided in a
    + * single-entry map keyed by the value for {@link #PARAMETER_NAME}.
    + */
    +public class InvokeEffectorOnSetChange extends AbstractPolicy implements SensorEventListener<Collection<?>> {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(InvokeEffectorOnSetChange.class);
    +
    +    public static final ConfigKey<AttributeSensor<? extends Collection<?>>> TRIGGER_SENSOR = ConfigKeys.newConfigKey(
    +            new TypeToken<AttributeSensor<? extends Collection<?>>>() {},
    +            "sensor",
    +            "Sensor to be monitored.");
    +
    +    public static final ConfigKey<String> PREVIOUS_SENSOR_NAME = ConfigKeys.newStringConfigKey(
    +            "previousSensorName",
    +            "The name under which the previous value for the trigger sensor should be published. " +
    +                    "If unset \".previous\" will be appended to the name of the trigger sensor.");
    +
    +    public static final ConfigKey<String> ON_ADDED_EFFECTOR_NAME = ConfigKeys.newStringConfigKey(
    +            "onAdded",
    +            "Name of the effector to invoke when entries are added to the collection.");
    +
    +    public static final ConfigKey<String> ON_REMOVED_EFFECTOR_NAME = ConfigKeys.newStringConfigKey(
    +            "onRemoved",
    +            "Name of the effector to invoke when entries are removed from the collection.");
    +
    +    public static final ConfigKey<String> PARAMETER_NAME = ConfigKeys.newStringConfigKey(
    +            "parameterName",
    +            "The name of the parameter to supply to the effectors",
    +            "value");
    +
    +    /** The previous version of the set against which events will be compared. */
    +    private Set<Object> previous = Collections.emptySet();
    +
    +    /** Guards accesses of previous. */
    +    private final Object[] updateLock = new Object[0];
    +
    +    @Override
    +    public void setEntity(EntityLocal entity) {
    +        super.setEntity(entity);
    +        Sensor<? extends Collection<?>> sensor =
    +                checkNotNull(getConfig(TRIGGER_SENSOR), "Value required for " + TRIGGER_SENSOR.getName());
    +
    +        checkArgument(Strings.isNonBlank(getConfig(PARAMETER_NAME)), "Value required for " + PARAMETER_NAME.getName());
    +
    +        // Fail straight away if neither effector is found.
    +        if (getEffector(getOnAddedEffector()).isAbsentOrNull() &&
    +                getEffector(getOnRemovedEffector()).isAbsentOrNull()) {
    +            throw new IllegalArgumentException("Value required for one or both of " + ON_ADDED_EFFECTOR_NAME.getName() +
    +                    " and " + ON_REMOVED_EFFECTOR_NAME.getName());
    +        }
    +
    +        // Initialise `present` before subscribing.
    +        Collection<?> current = entity.sensors().get(getTriggerSensor());
    +        synchronized (updateLock) {
    +            previous = (current != null) ? new HashSet<>(current) : Collections.emptySet();
    +        }
    +        subscriptions().subscribe(entity, sensor, this);
    +    }
    +
    +    @Override
    +    public void onEvent(SensorEvent<Collection<?>> event) {
    +        final Set<Object> added = new HashSet<>(), removed = new HashSet<>();
    +        Set<Object> newValue = new HashSet<>(event.getValue());
    +        synchronized (updateLock) {
    +            // Not using .immutableCopy() in case either set contains `null`.
    +            Sets.difference(newValue, previous).copyInto(added);
    +            Sets.difference(previous, newValue).copyInto(removed);
    --- End diff --
    
    We have to take the difference with the lock so that we can determine whether the collection is changed and thus should set the `previous` sensor. I'll move the `onAdded` and `onRemoved` calls out of the critical section.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #382: InvokeEffectorOnSetChange

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

    https://github.com/apache/brooklyn-server/pull/382#discussion_r83199736
  
    --- Diff: core/src/main/java/org/apache/brooklyn/policy/InvokeEffectorOnSetChange.java ---
    @@ -0,0 +1,203 @@
    +/*
    + * 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 org.apache.brooklyn.policy;
    +
    +import static com.google.common.base.Preconditions.checkArgument;
    +import static com.google.common.base.Preconditions.checkNotNull;
    +
    +import java.util.Collection;
    +import java.util.Collections;
    +import java.util.HashSet;
    +import java.util.Map;
    +import java.util.Set;
    +
    +import org.apache.brooklyn.api.effector.Effector;
    +import org.apache.brooklyn.api.entity.EntityLocal;
    +import org.apache.brooklyn.api.sensor.AttributeSensor;
    +import org.apache.brooklyn.api.sensor.Sensor;
    +import org.apache.brooklyn.api.sensor.SensorEvent;
    +import org.apache.brooklyn.api.sensor.SensorEventListener;
    +import org.apache.brooklyn.config.ConfigKey;
    +import org.apache.brooklyn.core.config.ConfigKeys;
    +import org.apache.brooklyn.core.policy.AbstractPolicy;
    +import org.apache.brooklyn.core.sensor.BasicAttributeSensor;
    +import org.apache.brooklyn.util.collections.MutableMap;
    +import org.apache.brooklyn.util.core.flags.TypeCoercions;
    +import org.apache.brooklyn.util.guava.Maybe;
    +import org.apache.brooklyn.util.text.Strings;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import com.google.common.collect.Sets;
    +import com.google.common.reflect.TypeToken;
    +
    +/**
    + * Subscribes to events on a collection {@link AttributeSensor} and invokes the named
    + * effectors for each element that was added and removed.
    + * <p>
    + * The policy only detects <em>replacements</em> of the collection; it does not act on
    + * modifications. If the sensor has value <code>A</code> and an element is added &ndash;
    + * value <code>A'</code> &ndash; the on-added effector is not invoked. If the sensor is
    + * later set to <code>B</code> the delta is made between <code>A</code> and <code>B</code>,
    + * not <code>A'</code> and <code>B</code>.
    + * <p>
    + * To simplify the detection of additions and removals the collection is converted to a
    + * {@link Set}. This means that only a single event will fire for duplicate elements in
    + * the collection.
    + * <p>
    + * The effectors are provided the elements that changed in their parameter map. If the
    + * sensor is a collection of maps the elements are provided with their keys coerced to
    + * strings and their values unchanged. Otherwise the elements are provided in a
    + * single-entry map keyed by the value for {@link #PARAMETER_NAME}.
    + */
    +public class InvokeEffectorOnSetChange extends AbstractPolicy implements SensorEventListener<Collection<?>> {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(InvokeEffectorOnSetChange.class);
    +
    +    public static final ConfigKey<AttributeSensor<? extends Collection<?>>> TRIGGER_SENSOR = ConfigKeys.newConfigKey(
    +            new TypeToken<AttributeSensor<? extends Collection<?>>>() {},
    +            "sensor",
    +            "Sensor to be monitored.");
    +
    +    public static final ConfigKey<String> PREVIOUS_SENSOR_NAME = ConfigKeys.newStringConfigKey(
    +            "previousSensorName",
    +            "The name under which the previous value for the trigger sensor should be published. " +
    +                    "If unset \".previous\" will be appended to the name of the trigger sensor.");
    +
    +    public static final ConfigKey<String> ON_ADDED_EFFECTOR_NAME = ConfigKeys.newStringConfigKey(
    +            "onAdded",
    +            "Name of the effector to invoke when entries are added to the collection.");
    +
    +    public static final ConfigKey<String> ON_REMOVED_EFFECTOR_NAME = ConfigKeys.newStringConfigKey(
    +            "onRemoved",
    +            "Name of the effector to invoke when entries are removed from the collection.");
    +
    +    public static final ConfigKey<String> PARAMETER_NAME = ConfigKeys.newStringConfigKey(
    +            "parameterName",
    +            "The name of the parameter to supply to the effectors",
    +            "value");
    +
    +    /** The previous version of the set against which events will be compared. */
    +    private Set<Object> previous = Collections.emptySet();
    +
    +    /** Guards accesses of previous. */
    +    private final Object[] updateLock = new Object[0];
    +
    +    @Override
    +    public void setEntity(EntityLocal entity) {
    +        super.setEntity(entity);
    +        Sensor<? extends Collection<?>> sensor =
    +                checkNotNull(getConfig(TRIGGER_SENSOR), "Value required for " + TRIGGER_SENSOR.getName());
    +
    +        checkArgument(Strings.isNonBlank(getConfig(PARAMETER_NAME)), "Value required for " + PARAMETER_NAME.getName());
    +
    +        // Fail straight away if neither effector is found.
    +        if (getEffector(getOnAddedEffector()).isAbsentOrNull() &&
    +                getEffector(getOnRemovedEffector()).isAbsentOrNull()) {
    +            throw new IllegalArgumentException("Value required for one or both of " + ON_ADDED_EFFECTOR_NAME.getName() +
    +                    " and " + ON_REMOVED_EFFECTOR_NAME.getName());
    +        }
    +
    +        // Initialise `present` before subscribing.
    +        Collection<?> current = entity.sensors().get(getTriggerSensor());
    +        synchronized (updateLock) {
    +            previous = (current != null) ? new HashSet<>(current) : Collections.emptySet();
    +        }
    +        subscriptions().subscribe(entity, sensor, this);
    +    }
    +
    +    @Override
    +    public void onEvent(SensorEvent<Collection<?>> event) {
    +        final Set<Object> added = new HashSet<>(), removed = new HashSet<>();
    +        Set<Object> newValue = new HashSet<>(event.getValue());
    +        synchronized (updateLock) {
    +            // Not using .immutableCopy() in case either set contains `null`.
    +            Sets.difference(newValue, previous).copyInto(added);
    +            Sets.difference(previous, newValue).copyInto(removed);
    +
    +            // Only perform an update if the collections have changed.
    +            if (!added.isEmpty() || !removed.isEmpty()) {
    +                for (Object o : added) {
    +                    onAdded(o);
    +                }
    +                for (Object o : removed) {
    +                    onRemoved(o);
    +                }
    +                entity.sensors().set(getPreviousSensor(), previous);
    +                requestPersist();
    +                // In advance of the next event.
    +                previous = Collections.unmodifiableSet(newValue);
    --- End diff --
    
    Do we really need a separate sensor for `previous`? It's actually always equal to the current value of `TRIGGER_SENSOR` (apart for the very short period after update and before this executes).
    Can initialize it from `TRIGGER_SENSOR` on rebind. That's actually what you already do. So the previous sensor is just for users to look at?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #382: InvokeEffectorOnSetChange

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

    https://github.com/apache/brooklyn-server/pull/382#discussion_r83262396
  
    --- Diff: core/src/main/java/org/apache/brooklyn/policy/InvokeEffectorOnSetChange.java ---
    @@ -0,0 +1,203 @@
    +/*
    + * 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 org.apache.brooklyn.policy;
    +
    +import static com.google.common.base.Preconditions.checkArgument;
    +import static com.google.common.base.Preconditions.checkNotNull;
    +
    +import java.util.Collection;
    +import java.util.Collections;
    +import java.util.HashSet;
    +import java.util.Map;
    +import java.util.Set;
    +
    +import org.apache.brooklyn.api.effector.Effector;
    +import org.apache.brooklyn.api.entity.EntityLocal;
    +import org.apache.brooklyn.api.sensor.AttributeSensor;
    +import org.apache.brooklyn.api.sensor.Sensor;
    +import org.apache.brooklyn.api.sensor.SensorEvent;
    +import org.apache.brooklyn.api.sensor.SensorEventListener;
    +import org.apache.brooklyn.config.ConfigKey;
    +import org.apache.brooklyn.core.config.ConfigKeys;
    +import org.apache.brooklyn.core.policy.AbstractPolicy;
    +import org.apache.brooklyn.core.sensor.BasicAttributeSensor;
    +import org.apache.brooklyn.util.collections.MutableMap;
    +import org.apache.brooklyn.util.core.flags.TypeCoercions;
    +import org.apache.brooklyn.util.guava.Maybe;
    +import org.apache.brooklyn.util.text.Strings;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import com.google.common.collect.Sets;
    +import com.google.common.reflect.TypeToken;
    +
    +/**
    + * Subscribes to events on a collection {@link AttributeSensor} and invokes the named
    + * effectors for each element that was added and removed.
    + * <p>
    + * The policy only detects <em>replacements</em> of the collection; it does not act on
    + * modifications. If the sensor has value <code>A</code> and an element is added &ndash;
    + * value <code>A'</code> &ndash; the on-added effector is not invoked. If the sensor is
    + * later set to <code>B</code> the delta is made between <code>A</code> and <code>B</code>,
    + * not <code>A'</code> and <code>B</code>.
    + * <p>
    + * To simplify the detection of additions and removals the collection is converted to a
    + * {@link Set}. This means that only a single event will fire for duplicate elements in
    + * the collection.
    + * <p>
    + * The effectors are provided the elements that changed in their parameter map. If the
    + * sensor is a collection of maps the elements are provided with their keys coerced to
    + * strings and their values unchanged. Otherwise the elements are provided in a
    + * single-entry map keyed by the value for {@link #PARAMETER_NAME}.
    + */
    +public class InvokeEffectorOnSetChange extends AbstractPolicy implements SensorEventListener<Collection<?>> {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(InvokeEffectorOnSetChange.class);
    +
    +    public static final ConfigKey<AttributeSensor<? extends Collection<?>>> TRIGGER_SENSOR = ConfigKeys.newConfigKey(
    +            new TypeToken<AttributeSensor<? extends Collection<?>>>() {},
    +            "sensor",
    +            "Sensor to be monitored.");
    +
    +    public static final ConfigKey<String> PREVIOUS_SENSOR_NAME = ConfigKeys.newStringConfigKey(
    +            "previousSensorName",
    +            "The name under which the previous value for the trigger sensor should be published. " +
    +                    "If unset \".previous\" will be appended to the name of the trigger sensor.");
    +
    +    public static final ConfigKey<String> ON_ADDED_EFFECTOR_NAME = ConfigKeys.newStringConfigKey(
    +            "onAdded",
    +            "Name of the effector to invoke when entries are added to the collection.");
    +
    +    public static final ConfigKey<String> ON_REMOVED_EFFECTOR_NAME = ConfigKeys.newStringConfigKey(
    +            "onRemoved",
    +            "Name of the effector to invoke when entries are removed from the collection.");
    +
    +    public static final ConfigKey<String> PARAMETER_NAME = ConfigKeys.newStringConfigKey(
    +            "parameterName",
    +            "The name of the parameter to supply to the effectors",
    +            "value");
    +
    +    /** The previous version of the set against which events will be compared. */
    +    private Set<Object> previous = Collections.emptySet();
    +
    +    /** Guards accesses of previous. */
    +    private final Object[] updateLock = new Object[0];
    +
    +    @Override
    +    public void setEntity(EntityLocal entity) {
    +        super.setEntity(entity);
    +        Sensor<? extends Collection<?>> sensor =
    +                checkNotNull(getConfig(TRIGGER_SENSOR), "Value required for " + TRIGGER_SENSOR.getName());
    +
    +        checkArgument(Strings.isNonBlank(getConfig(PARAMETER_NAME)), "Value required for " + PARAMETER_NAME.getName());
    +
    +        // Fail straight away if neither effector is found.
    +        if (getEffector(getOnAddedEffector()).isAbsentOrNull() &&
    +                getEffector(getOnRemovedEffector()).isAbsentOrNull()) {
    +            throw new IllegalArgumentException("Value required for one or both of " + ON_ADDED_EFFECTOR_NAME.getName() +
    +                    " and " + ON_REMOVED_EFFECTOR_NAME.getName());
    +        }
    +
    +        // Initialise `present` before subscribing.
    +        Collection<?> current = entity.sensors().get(getTriggerSensor());
    +        synchronized (updateLock) {
    +            previous = (current != null) ? new HashSet<>(current) : Collections.emptySet();
    +        }
    +        subscriptions().subscribe(entity, sensor, this);
    +    }
    +
    +    @Override
    +    public void onEvent(SensorEvent<Collection<?>> event) {
    +        final Set<Object> added = new HashSet<>(), removed = new HashSet<>();
    +        Set<Object> newValue = new HashSet<>(event.getValue());
    +        synchronized (updateLock) {
    +            // Not using .immutableCopy() in case either set contains `null`.
    +            Sets.difference(newValue, previous).copyInto(added);
    +            Sets.difference(previous, newValue).copyInto(removed);
    --- End diff --
    
    If we move `onAdded` and `onRemoved` do we risk having this kind of race? 
    ```
    previous value of sensor is []
    Thread A calls onEvent with [1]
    Thread B calls onEvent with []
    A obtains updateLock and calculates added and removed.
    A releases updateLock.
    A pauses.
    B obtains updateLock and calculates added and removed
    B releases updateLock
    B invokes onRemoved(1)
    A wakes up and invokes onAdded(1)
    ```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #382: InvokeEffectorOnSetChange

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

    https://github.com/apache/brooklyn-server/pull/382#discussion_r83198850
  
    --- Diff: core/src/main/java/org/apache/brooklyn/policy/InvokeEffectorOnSetChange.java ---
    @@ -0,0 +1,203 @@
    +/*
    + * 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 org.apache.brooklyn.policy;
    +
    +import static com.google.common.base.Preconditions.checkArgument;
    +import static com.google.common.base.Preconditions.checkNotNull;
    +
    +import java.util.Collection;
    +import java.util.Collections;
    +import java.util.HashSet;
    +import java.util.Map;
    +import java.util.Set;
    +
    +import org.apache.brooklyn.api.effector.Effector;
    +import org.apache.brooklyn.api.entity.EntityLocal;
    +import org.apache.brooklyn.api.sensor.AttributeSensor;
    +import org.apache.brooklyn.api.sensor.Sensor;
    +import org.apache.brooklyn.api.sensor.SensorEvent;
    +import org.apache.brooklyn.api.sensor.SensorEventListener;
    +import org.apache.brooklyn.config.ConfigKey;
    +import org.apache.brooklyn.core.config.ConfigKeys;
    +import org.apache.brooklyn.core.policy.AbstractPolicy;
    +import org.apache.brooklyn.core.sensor.BasicAttributeSensor;
    +import org.apache.brooklyn.util.collections.MutableMap;
    +import org.apache.brooklyn.util.core.flags.TypeCoercions;
    +import org.apache.brooklyn.util.guava.Maybe;
    +import org.apache.brooklyn.util.text.Strings;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import com.google.common.collect.Sets;
    +import com.google.common.reflect.TypeToken;
    +
    +/**
    + * Subscribes to events on a collection {@link AttributeSensor} and invokes the named
    + * effectors for each element that was added and removed.
    + * <p>
    + * The policy only detects <em>replacements</em> of the collection; it does not act on
    + * modifications. If the sensor has value <code>A</code> and an element is added &ndash;
    + * value <code>A'</code> &ndash; the on-added effector is not invoked. If the sensor is
    + * later set to <code>B</code> the delta is made between <code>A</code> and <code>B</code>,
    + * not <code>A'</code> and <code>B</code>.
    + * <p>
    + * To simplify the detection of additions and removals the collection is converted to a
    + * {@link Set}. This means that only a single event will fire for duplicate elements in
    + * the collection.
    + * <p>
    + * The effectors are provided the elements that changed in their parameter map. If the
    + * sensor is a collection of maps the elements are provided with their keys coerced to
    + * strings and their values unchanged. Otherwise the elements are provided in a
    + * single-entry map keyed by the value for {@link #PARAMETER_NAME}.
    + */
    +public class InvokeEffectorOnSetChange extends AbstractPolicy implements SensorEventListener<Collection<?>> {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(InvokeEffectorOnSetChange.class);
    +
    +    public static final ConfigKey<AttributeSensor<? extends Collection<?>>> TRIGGER_SENSOR = ConfigKeys.newConfigKey(
    +            new TypeToken<AttributeSensor<? extends Collection<?>>>() {},
    +            "sensor",
    +            "Sensor to be monitored.");
    +
    +    public static final ConfigKey<String> PREVIOUS_SENSOR_NAME = ConfigKeys.newStringConfigKey(
    +            "previousSensorName",
    +            "The name under which the previous value for the trigger sensor should be published. " +
    +                    "If unset \".previous\" will be appended to the name of the trigger sensor.");
    +
    +    public static final ConfigKey<String> ON_ADDED_EFFECTOR_NAME = ConfigKeys.newStringConfigKey(
    +            "onAdded",
    +            "Name of the effector to invoke when entries are added to the collection.");
    +
    +    public static final ConfigKey<String> ON_REMOVED_EFFECTOR_NAME = ConfigKeys.newStringConfigKey(
    +            "onRemoved",
    +            "Name of the effector to invoke when entries are removed from the collection.");
    +
    +    public static final ConfigKey<String> PARAMETER_NAME = ConfigKeys.newStringConfigKey(
    +            "parameterName",
    +            "The name of the parameter to supply to the effectors",
    +            "value");
    +
    +    /** The previous version of the set against which events will be compared. */
    +    private Set<Object> previous = Collections.emptySet();
    +
    +    /** Guards accesses of previous. */
    +    private final Object[] updateLock = new Object[0];
    +
    +    @Override
    +    public void setEntity(EntityLocal entity) {
    +        super.setEntity(entity);
    +        Sensor<? extends Collection<?>> sensor =
    +                checkNotNull(getConfig(TRIGGER_SENSOR), "Value required for " + TRIGGER_SENSOR.getName());
    +
    +        checkArgument(Strings.isNonBlank(getConfig(PARAMETER_NAME)), "Value required for " + PARAMETER_NAME.getName());
    +
    +        // Fail straight away if neither effector is found.
    +        if (getEffector(getOnAddedEffector()).isAbsentOrNull() &&
    +                getEffector(getOnRemovedEffector()).isAbsentOrNull()) {
    +            throw new IllegalArgumentException("Value required for one or both of " + ON_ADDED_EFFECTOR_NAME.getName() +
    +                    " and " + ON_REMOVED_EFFECTOR_NAME.getName());
    +        }
    +
    +        // Initialise `present` before subscribing.
    +        Collection<?> current = entity.sensors().get(getTriggerSensor());
    +        synchronized (updateLock) {
    +            previous = (current != null) ? new HashSet<>(current) : Collections.emptySet();
    +        }
    +        subscriptions().subscribe(entity, sensor, this);
    +    }
    +
    +    @Override
    +    public void onEvent(SensorEvent<Collection<?>> event) {
    +        final Set<Object> added = new HashSet<>(), removed = new HashSet<>();
    +        Set<Object> newValue = new HashSet<>(event.getValue());
    +        synchronized (updateLock) {
    +            // Not using .immutableCopy() in case either set contains `null`.
    +            Sets.difference(newValue, previous).copyInto(added);
    +            Sets.difference(previous, newValue).copyInto(removed);
    --- End diff --
    
    You can hold the lock just for the following actions.
      * copying the `previous` reference in a local var
      * updating `previous` to `newValue`
      * moving `entity.sensors().set(getPreviousSensor(), previous);` up here
      * using the local variable to work out the differences


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server issue #382: InvokeEffectorOnCollectionSensorChange

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

    https://github.com/apache/brooklyn-server/pull/382
  
    I will merge this.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #382: InvokeEffectorOnCollectionSensorChange

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

    https://github.com/apache/brooklyn-server/pull/382#discussion_r83282121
  
    --- Diff: core/src/main/java/org/apache/brooklyn/policy/InvokeEffectorOnSetChange.java ---
    @@ -0,0 +1,203 @@
    +/*
    + * 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 org.apache.brooklyn.policy;
    +
    +import static com.google.common.base.Preconditions.checkArgument;
    +import static com.google.common.base.Preconditions.checkNotNull;
    +
    +import java.util.Collection;
    +import java.util.Collections;
    +import java.util.HashSet;
    +import java.util.Map;
    +import java.util.Set;
    +
    +import org.apache.brooklyn.api.effector.Effector;
    +import org.apache.brooklyn.api.entity.EntityLocal;
    +import org.apache.brooklyn.api.sensor.AttributeSensor;
    +import org.apache.brooklyn.api.sensor.Sensor;
    +import org.apache.brooklyn.api.sensor.SensorEvent;
    +import org.apache.brooklyn.api.sensor.SensorEventListener;
    +import org.apache.brooklyn.config.ConfigKey;
    +import org.apache.brooklyn.core.config.ConfigKeys;
    +import org.apache.brooklyn.core.policy.AbstractPolicy;
    +import org.apache.brooklyn.core.sensor.BasicAttributeSensor;
    +import org.apache.brooklyn.util.collections.MutableMap;
    +import org.apache.brooklyn.util.core.flags.TypeCoercions;
    +import org.apache.brooklyn.util.guava.Maybe;
    +import org.apache.brooklyn.util.text.Strings;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import com.google.common.collect.Sets;
    +import com.google.common.reflect.TypeToken;
    +
    +/**
    + * Subscribes to events on a collection {@link AttributeSensor} and invokes the named
    + * effectors for each element that was added and removed.
    + * <p>
    + * The policy only detects <em>replacements</em> of the collection; it does not act on
    + * modifications. If the sensor has value <code>A</code> and an element is added &ndash;
    + * value <code>A'</code> &ndash; the on-added effector is not invoked. If the sensor is
    + * later set to <code>B</code> the delta is made between <code>A</code> and <code>B</code>,
    + * not <code>A'</code> and <code>B</code>.
    + * <p>
    + * To simplify the detection of additions and removals the collection is converted to a
    + * {@link Set}. This means that only a single event will fire for duplicate elements in
    + * the collection.
    + * <p>
    + * The effectors are provided the elements that changed in their parameter map. If the
    + * sensor is a collection of maps the elements are provided with their keys coerced to
    + * strings and their values unchanged. Otherwise the elements are provided in a
    + * single-entry map keyed by the value for {@link #PARAMETER_NAME}.
    + */
    +public class InvokeEffectorOnSetChange extends AbstractPolicy implements SensorEventListener<Collection<?>> {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(InvokeEffectorOnSetChange.class);
    +
    +    public static final ConfigKey<AttributeSensor<? extends Collection<?>>> TRIGGER_SENSOR = ConfigKeys.newConfigKey(
    +            new TypeToken<AttributeSensor<? extends Collection<?>>>() {},
    +            "sensor",
    +            "Sensor to be monitored.");
    +
    +    public static final ConfigKey<String> PREVIOUS_SENSOR_NAME = ConfigKeys.newStringConfigKey(
    +            "previousSensorName",
    +            "The name under which the previous value for the trigger sensor should be published. " +
    +                    "If unset \".previous\" will be appended to the name of the trigger sensor.");
    +
    +    public static final ConfigKey<String> ON_ADDED_EFFECTOR_NAME = ConfigKeys.newStringConfigKey(
    +            "onAdded",
    +            "Name of the effector to invoke when entries are added to the collection.");
    +
    +    public static final ConfigKey<String> ON_REMOVED_EFFECTOR_NAME = ConfigKeys.newStringConfigKey(
    +            "onRemoved",
    +            "Name of the effector to invoke when entries are removed from the collection.");
    +
    +    public static final ConfigKey<String> PARAMETER_NAME = ConfigKeys.newStringConfigKey(
    +            "parameterName",
    +            "The name of the parameter to supply to the effectors",
    +            "value");
    +
    +    /** The previous version of the set against which events will be compared. */
    +    private Set<Object> previous = Collections.emptySet();
    +
    +    /** Guards accesses of previous. */
    +    private final Object[] updateLock = new Object[0];
    +
    +    @Override
    +    public void setEntity(EntityLocal entity) {
    +        super.setEntity(entity);
    +        Sensor<? extends Collection<?>> sensor =
    +                checkNotNull(getConfig(TRIGGER_SENSOR), "Value required for " + TRIGGER_SENSOR.getName());
    +
    +        checkArgument(Strings.isNonBlank(getConfig(PARAMETER_NAME)), "Value required for " + PARAMETER_NAME.getName());
    +
    +        // Fail straight away if neither effector is found.
    +        if (getEffector(getOnAddedEffector()).isAbsentOrNull() &&
    +                getEffector(getOnRemovedEffector()).isAbsentOrNull()) {
    +            throw new IllegalArgumentException("Value required for one or both of " + ON_ADDED_EFFECTOR_NAME.getName() +
    +                    " and " + ON_REMOVED_EFFECTOR_NAME.getName());
    +        }
    +
    +        // Initialise `present` before subscribing.
    +        Collection<?> current = entity.sensors().get(getTriggerSensor());
    +        synchronized (updateLock) {
    +            previous = (current != null) ? new HashSet<>(current) : Collections.emptySet();
    +        }
    +        subscriptions().subscribe(entity, sensor, this);
    +    }
    +
    +    @Override
    +    public void onEvent(SensorEvent<Collection<?>> event) {
    +        final Set<Object> added = new HashSet<>(), removed = new HashSet<>();
    +        Set<Object> newValue = new HashSet<>(event.getValue());
    +        synchronized (updateLock) {
    +            // Not using .immutableCopy() in case either set contains `null`.
    +            Sets.difference(newValue, previous).copyInto(added);
    +            Sets.difference(previous, newValue).copyInto(removed);
    --- End diff --
    
    Though to be honest since effectors are asynchronous this will just be a problem further down the line, won't it? 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #382: InvokeEffectorOnSetChange

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

    https://github.com/apache/brooklyn-server/pull/382#discussion_r83199029
  
    --- Diff: core/src/main/java/org/apache/brooklyn/policy/InvokeEffectorOnSetChange.java ---
    @@ -0,0 +1,203 @@
    +/*
    + * 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 org.apache.brooklyn.policy;
    +
    +import static com.google.common.base.Preconditions.checkArgument;
    +import static com.google.common.base.Preconditions.checkNotNull;
    +
    +import java.util.Collection;
    +import java.util.Collections;
    +import java.util.HashSet;
    +import java.util.Map;
    +import java.util.Set;
    +
    +import org.apache.brooklyn.api.effector.Effector;
    +import org.apache.brooklyn.api.entity.EntityLocal;
    +import org.apache.brooklyn.api.sensor.AttributeSensor;
    +import org.apache.brooklyn.api.sensor.Sensor;
    +import org.apache.brooklyn.api.sensor.SensorEvent;
    +import org.apache.brooklyn.api.sensor.SensorEventListener;
    +import org.apache.brooklyn.config.ConfigKey;
    +import org.apache.brooklyn.core.config.ConfigKeys;
    +import org.apache.brooklyn.core.policy.AbstractPolicy;
    +import org.apache.brooklyn.core.sensor.BasicAttributeSensor;
    +import org.apache.brooklyn.util.collections.MutableMap;
    +import org.apache.brooklyn.util.core.flags.TypeCoercions;
    +import org.apache.brooklyn.util.guava.Maybe;
    +import org.apache.brooklyn.util.text.Strings;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import com.google.common.collect.Sets;
    +import com.google.common.reflect.TypeToken;
    +
    +/**
    + * Subscribes to events on a collection {@link AttributeSensor} and invokes the named
    + * effectors for each element that was added and removed.
    + * <p>
    + * The policy only detects <em>replacements</em> of the collection; it does not act on
    + * modifications. If the sensor has value <code>A</code> and an element is added &ndash;
    + * value <code>A'</code> &ndash; the on-added effector is not invoked. If the sensor is
    + * later set to <code>B</code> the delta is made between <code>A</code> and <code>B</code>,
    + * not <code>A'</code> and <code>B</code>.
    + * <p>
    + * To simplify the detection of additions and removals the collection is converted to a
    + * {@link Set}. This means that only a single event will fire for duplicate elements in
    + * the collection.
    + * <p>
    + * The effectors are provided the elements that changed in their parameter map. If the
    + * sensor is a collection of maps the elements are provided with their keys coerced to
    + * strings and their values unchanged. Otherwise the elements are provided in a
    + * single-entry map keyed by the value for {@link #PARAMETER_NAME}.
    + */
    +public class InvokeEffectorOnSetChange extends AbstractPolicy implements SensorEventListener<Collection<?>> {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(InvokeEffectorOnSetChange.class);
    +
    +    public static final ConfigKey<AttributeSensor<? extends Collection<?>>> TRIGGER_SENSOR = ConfigKeys.newConfigKey(
    +            new TypeToken<AttributeSensor<? extends Collection<?>>>() {},
    +            "sensor",
    +            "Sensor to be monitored.");
    +
    +    public static final ConfigKey<String> PREVIOUS_SENSOR_NAME = ConfigKeys.newStringConfigKey(
    +            "previousSensorName",
    +            "The name under which the previous value for the trigger sensor should be published. " +
    +                    "If unset \".previous\" will be appended to the name of the trigger sensor.");
    +
    +    public static final ConfigKey<String> ON_ADDED_EFFECTOR_NAME = ConfigKeys.newStringConfigKey(
    +            "onAdded",
    +            "Name of the effector to invoke when entries are added to the collection.");
    +
    +    public static final ConfigKey<String> ON_REMOVED_EFFECTOR_NAME = ConfigKeys.newStringConfigKey(
    +            "onRemoved",
    +            "Name of the effector to invoke when entries are removed from the collection.");
    +
    +    public static final ConfigKey<String> PARAMETER_NAME = ConfigKeys.newStringConfigKey(
    +            "parameterName",
    +            "The name of the parameter to supply to the effectors",
    +            "value");
    +
    +    /** The previous version of the set against which events will be compared. */
    +    private Set<Object> previous = Collections.emptySet();
    +
    +    /** Guards accesses of previous. */
    +    private final Object[] updateLock = new Object[0];
    +
    +    @Override
    +    public void setEntity(EntityLocal entity) {
    +        super.setEntity(entity);
    +        Sensor<? extends Collection<?>> sensor =
    +                checkNotNull(getConfig(TRIGGER_SENSOR), "Value required for " + TRIGGER_SENSOR.getName());
    +
    +        checkArgument(Strings.isNonBlank(getConfig(PARAMETER_NAME)), "Value required for " + PARAMETER_NAME.getName());
    +
    +        // Fail straight away if neither effector is found.
    +        if (getEffector(getOnAddedEffector()).isAbsentOrNull() &&
    +                getEffector(getOnRemovedEffector()).isAbsentOrNull()) {
    +            throw new IllegalArgumentException("Value required for one or both of " + ON_ADDED_EFFECTOR_NAME.getName() +
    +                    " and " + ON_REMOVED_EFFECTOR_NAME.getName());
    +        }
    +
    +        // Initialise `present` before subscribing.
    +        Collection<?> current = entity.sensors().get(getTriggerSensor());
    +        synchronized (updateLock) {
    +            previous = (current != null) ? new HashSet<>(current) : Collections.emptySet();
    +        }
    +        subscriptions().subscribe(entity, sensor, this);
    +    }
    +
    +    @Override
    +    public void onEvent(SensorEvent<Collection<?>> event) {
    +        final Set<Object> added = new HashSet<>(), removed = new HashSet<>();
    +        Set<Object> newValue = new HashSet<>(event.getValue());
    +        synchronized (updateLock) {
    +            // Not using .immutableCopy() in case either set contains `null`.
    +            Sets.difference(newValue, previous).copyInto(added);
    +            Sets.difference(previous, newValue).copyInto(removed);
    +
    +            // Only perform an update if the collections have changed.
    +            if (!added.isEmpty() || !removed.isEmpty()) {
    +                for (Object o : added) {
    +                    onAdded(o);
    +                }
    +                for (Object o : removed) {
    +                    onRemoved(o);
    +                }
    +                entity.sensors().set(getPreviousSensor(), previous);
    +                requestPersist();
    +                // In advance of the next event.
    +                previous = Collections.unmodifiableSet(newValue);
    --- End diff --
    
    The sensor value for `previous` is one iteration behind the value hold in the field. Wouldn't this be a problem after rebind?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #382: InvokeEffectorOnSetChange

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

    https://github.com/apache/brooklyn-server/pull/382#discussion_r83200686
  
    --- Diff: core/src/main/java/org/apache/brooklyn/policy/InvokeEffectorOnSetChange.java ---
    @@ -0,0 +1,203 @@
    +/*
    + * 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 org.apache.brooklyn.policy;
    +
    +import static com.google.common.base.Preconditions.checkArgument;
    +import static com.google.common.base.Preconditions.checkNotNull;
    +
    +import java.util.Collection;
    +import java.util.Collections;
    +import java.util.HashSet;
    +import java.util.Map;
    +import java.util.Set;
    +
    +import org.apache.brooklyn.api.effector.Effector;
    +import org.apache.brooklyn.api.entity.EntityLocal;
    +import org.apache.brooklyn.api.sensor.AttributeSensor;
    +import org.apache.brooklyn.api.sensor.Sensor;
    +import org.apache.brooklyn.api.sensor.SensorEvent;
    +import org.apache.brooklyn.api.sensor.SensorEventListener;
    +import org.apache.brooklyn.config.ConfigKey;
    +import org.apache.brooklyn.core.config.ConfigKeys;
    +import org.apache.brooklyn.core.policy.AbstractPolicy;
    +import org.apache.brooklyn.core.sensor.BasicAttributeSensor;
    +import org.apache.brooklyn.util.collections.MutableMap;
    +import org.apache.brooklyn.util.core.flags.TypeCoercions;
    +import org.apache.brooklyn.util.guava.Maybe;
    +import org.apache.brooklyn.util.text.Strings;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import com.google.common.collect.Sets;
    +import com.google.common.reflect.TypeToken;
    +
    +/**
    + * Subscribes to events on a collection {@link AttributeSensor} and invokes the named
    + * effectors for each element that was added and removed.
    + * <p>
    + * The policy only detects <em>replacements</em> of the collection; it does not act on
    + * modifications. If the sensor has value <code>A</code> and an element is added &ndash;
    + * value <code>A'</code> &ndash; the on-added effector is not invoked. If the sensor is
    + * later set to <code>B</code> the delta is made between <code>A</code> and <code>B</code>,
    + * not <code>A'</code> and <code>B</code>.
    + * <p>
    + * To simplify the detection of additions and removals the collection is converted to a
    + * {@link Set}. This means that only a single event will fire for duplicate elements in
    + * the collection.
    + * <p>
    + * The effectors are provided the elements that changed in their parameter map. If the
    + * sensor is a collection of maps the elements are provided with their keys coerced to
    + * strings and their values unchanged. Otherwise the elements are provided in a
    + * single-entry map keyed by the value for {@link #PARAMETER_NAME}.
    + */
    +public class InvokeEffectorOnSetChange extends AbstractPolicy implements SensorEventListener<Collection<?>> {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(InvokeEffectorOnSetChange.class);
    +
    +    public static final ConfigKey<AttributeSensor<? extends Collection<?>>> TRIGGER_SENSOR = ConfigKeys.newConfigKey(
    +            new TypeToken<AttributeSensor<? extends Collection<?>>>() {},
    +            "sensor",
    +            "Sensor to be monitored.");
    +
    +    public static final ConfigKey<String> PREVIOUS_SENSOR_NAME = ConfigKeys.newStringConfigKey(
    +            "previousSensorName",
    +            "The name under which the previous value for the trigger sensor should be published. " +
    +                    "If unset \".previous\" will be appended to the name of the trigger sensor.");
    +
    +    public static final ConfigKey<String> ON_ADDED_EFFECTOR_NAME = ConfigKeys.newStringConfigKey(
    +            "onAdded",
    +            "Name of the effector to invoke when entries are added to the collection.");
    +
    +    public static final ConfigKey<String> ON_REMOVED_EFFECTOR_NAME = ConfigKeys.newStringConfigKey(
    +            "onRemoved",
    +            "Name of the effector to invoke when entries are removed from the collection.");
    +
    +    public static final ConfigKey<String> PARAMETER_NAME = ConfigKeys.newStringConfigKey(
    +            "parameterName",
    +            "The name of the parameter to supply to the effectors",
    +            "value");
    +
    +    /** The previous version of the set against which events will be compared. */
    +    private Set<Object> previous = Collections.emptySet();
    +
    +    /** Guards accesses of previous. */
    +    private final Object[] updateLock = new Object[0];
    +
    +    @Override
    +    public void setEntity(EntityLocal entity) {
    +        super.setEntity(entity);
    +        Sensor<? extends Collection<?>> sensor =
    +                checkNotNull(getConfig(TRIGGER_SENSOR), "Value required for " + TRIGGER_SENSOR.getName());
    +
    +        checkArgument(Strings.isNonBlank(getConfig(PARAMETER_NAME)), "Value required for " + PARAMETER_NAME.getName());
    +
    +        // Fail straight away if neither effector is found.
    +        if (getEffector(getOnAddedEffector()).isAbsentOrNull() &&
    +                getEffector(getOnRemovedEffector()).isAbsentOrNull()) {
    +            throw new IllegalArgumentException("Value required for one or both of " + ON_ADDED_EFFECTOR_NAME.getName() +
    +                    " and " + ON_REMOVED_EFFECTOR_NAME.getName());
    +        }
    +
    +        // Initialise `present` before subscribing.
    +        Collection<?> current = entity.sensors().get(getTriggerSensor());
    +        synchronized (updateLock) {
    +            previous = (current != null) ? new HashSet<>(current) : Collections.emptySet();
    +        }
    +        subscriptions().subscribe(entity, sensor, this);
    +    }
    +
    +    @Override
    +    public void onEvent(SensorEvent<Collection<?>> event) {
    +        final Set<Object> added = new HashSet<>(), removed = new HashSet<>();
    +        Set<Object> newValue = new HashSet<>(event.getValue());
    +        synchronized (updateLock) {
    +            // Not using .immutableCopy() in case either set contains `null`.
    +            Sets.difference(newValue, previous).copyInto(added);
    +            Sets.difference(previous, newValue).copyInto(removed);
    +
    +            // Only perform an update if the collections have changed.
    +            if (!added.isEmpty() || !removed.isEmpty()) {
    +                for (Object o : added) {
    +                    onAdded(o);
    +                }
    +                for (Object o : removed) {
    +                    onRemoved(o);
    +                }
    +                entity.sensors().set(getPreviousSensor(), previous);
    +                requestPersist();
    --- End diff --
    
    Shouldn't be needed, all important info is stored in sensors (which will trigger update on change).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #382: InvokeEffectorOnCollectionSensorChange

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

    https://github.com/apache/brooklyn-server/pull/382


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server issue #382: InvokeEffectorOnSetChange

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

    https://github.com/apache/brooklyn-server/pull/382
  
    @neykov thank you for your comments. I've addressed everything I didn't comment on. I've also renamed the policy to `InvokeEffectorOnCollectionSensorChange` to be consistent with the existing `InvokeEffectorOnSensorChange`, but no strong feelings there really.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #382: InvokeEffectorOnSetChange

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

    https://github.com/apache/brooklyn-server/pull/382#discussion_r83197672
  
    --- Diff: core/src/main/java/org/apache/brooklyn/policy/InvokeEffectorOnSetChange.java ---
    @@ -0,0 +1,203 @@
    +/*
    + * 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 org.apache.brooklyn.policy;
    +
    +import static com.google.common.base.Preconditions.checkArgument;
    +import static com.google.common.base.Preconditions.checkNotNull;
    +
    +import java.util.Collection;
    +import java.util.Collections;
    +import java.util.HashSet;
    +import java.util.Map;
    +import java.util.Set;
    +
    +import org.apache.brooklyn.api.effector.Effector;
    +import org.apache.brooklyn.api.entity.EntityLocal;
    +import org.apache.brooklyn.api.sensor.AttributeSensor;
    +import org.apache.brooklyn.api.sensor.Sensor;
    +import org.apache.brooklyn.api.sensor.SensorEvent;
    +import org.apache.brooklyn.api.sensor.SensorEventListener;
    +import org.apache.brooklyn.config.ConfigKey;
    +import org.apache.brooklyn.core.config.ConfigKeys;
    +import org.apache.brooklyn.core.policy.AbstractPolicy;
    +import org.apache.brooklyn.core.sensor.BasicAttributeSensor;
    +import org.apache.brooklyn.util.collections.MutableMap;
    +import org.apache.brooklyn.util.core.flags.TypeCoercions;
    +import org.apache.brooklyn.util.guava.Maybe;
    +import org.apache.brooklyn.util.text.Strings;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import com.google.common.collect.Sets;
    +import com.google.common.reflect.TypeToken;
    +
    +/**
    + * Subscribes to events on a collection {@link AttributeSensor} and invokes the named
    + * effectors for each element that was added and removed.
    + * <p>
    + * The policy only detects <em>replacements</em> of the collection; it does not act on
    + * modifications. If the sensor has value <code>A</code> and an element is added &ndash;
    + * value <code>A'</code> &ndash; the on-added effector is not invoked. If the sensor is
    + * later set to <code>B</code> the delta is made between <code>A</code> and <code>B</code>,
    + * not <code>A'</code> and <code>B</code>.
    + * <p>
    + * To simplify the detection of additions and removals the collection is converted to a
    + * {@link Set}. This means that only a single event will fire for duplicate elements in
    + * the collection.
    + * <p>
    + * The effectors are provided the elements that changed in their parameter map. If the
    + * sensor is a collection of maps the elements are provided with their keys coerced to
    + * strings and their values unchanged. Otherwise the elements are provided in a
    + * single-entry map keyed by the value for {@link #PARAMETER_NAME}.
    + */
    +public class InvokeEffectorOnSetChange extends AbstractPolicy implements SensorEventListener<Collection<?>> {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(InvokeEffectorOnSetChange.class);
    +
    +    public static final ConfigKey<AttributeSensor<? extends Collection<?>>> TRIGGER_SENSOR = ConfigKeys.newConfigKey(
    +            new TypeToken<AttributeSensor<? extends Collection<?>>>() {},
    +            "sensor",
    +            "Sensor to be monitored.");
    +
    +    public static final ConfigKey<String> PREVIOUS_SENSOR_NAME = ConfigKeys.newStringConfigKey(
    +            "previousSensorName",
    +            "The name under which the previous value for the trigger sensor should be published. " +
    +                    "If unset \".previous\" will be appended to the name of the trigger sensor.");
    +
    +    public static final ConfigKey<String> ON_ADDED_EFFECTOR_NAME = ConfigKeys.newStringConfigKey(
    +            "onAdded",
    +            "Name of the effector to invoke when entries are added to the collection.");
    +
    +    public static final ConfigKey<String> ON_REMOVED_EFFECTOR_NAME = ConfigKeys.newStringConfigKey(
    +            "onRemoved",
    +            "Name of the effector to invoke when entries are removed from the collection.");
    +
    +    public static final ConfigKey<String> PARAMETER_NAME = ConfigKeys.newStringConfigKey(
    +            "parameterName",
    +            "The name of the parameter to supply to the effectors",
    +            "value");
    +
    +    /** The previous version of the set against which events will be compared. */
    +    private Set<Object> previous = Collections.emptySet();
    +
    +    /** Guards accesses of previous. */
    +    private final Object[] updateLock = new Object[0];
    +
    +    @Override
    +    public void setEntity(EntityLocal entity) {
    +        super.setEntity(entity);
    +        Sensor<? extends Collection<?>> sensor =
    +                checkNotNull(getConfig(TRIGGER_SENSOR), "Value required for " + TRIGGER_SENSOR.getName());
    +
    +        checkArgument(Strings.isNonBlank(getConfig(PARAMETER_NAME)), "Value required for " + PARAMETER_NAME.getName());
    +
    +        // Fail straight away if neither effector is found.
    +        if (getEffector(getOnAddedEffector()).isAbsentOrNull() &&
    +                getEffector(getOnRemovedEffector()).isAbsentOrNull()) {
    +            throw new IllegalArgumentException("Value required for one or both of " + ON_ADDED_EFFECTOR_NAME.getName() +
    +                    " and " + ON_REMOVED_EFFECTOR_NAME.getName());
    +        }
    +
    +        // Initialise `present` before subscribing.
    +        Collection<?> current = entity.sensors().get(getTriggerSensor());
    +        synchronized (updateLock) {
    +            previous = (current != null) ? new HashSet<>(current) : Collections.emptySet();
    +        }
    +        subscriptions().subscribe(entity, sensor, this);
    +    }
    +
    +    @Override
    +    public void onEvent(SensorEvent<Collection<?>> event) {
    +        final Set<Object> added = new HashSet<>(), removed = new HashSet<>();
    +        Set<Object> newValue = new HashSet<>(event.getValue());
    --- End diff --
    
    new value could be null


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #382: InvokeEffectorOnSetChange

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

    https://github.com/apache/brooklyn-server/pull/382#discussion_r83198053
  
    --- Diff: core/src/main/java/org/apache/brooklyn/policy/InvokeEffectorOnSetChange.java ---
    @@ -0,0 +1,203 @@
    +/*
    + * 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 org.apache.brooklyn.policy;
    +
    +import static com.google.common.base.Preconditions.checkArgument;
    +import static com.google.common.base.Preconditions.checkNotNull;
    +
    +import java.util.Collection;
    +import java.util.Collections;
    +import java.util.HashSet;
    +import java.util.Map;
    +import java.util.Set;
    +
    +import org.apache.brooklyn.api.effector.Effector;
    +import org.apache.brooklyn.api.entity.EntityLocal;
    +import org.apache.brooklyn.api.sensor.AttributeSensor;
    +import org.apache.brooklyn.api.sensor.Sensor;
    +import org.apache.brooklyn.api.sensor.SensorEvent;
    +import org.apache.brooklyn.api.sensor.SensorEventListener;
    +import org.apache.brooklyn.config.ConfigKey;
    +import org.apache.brooklyn.core.config.ConfigKeys;
    +import org.apache.brooklyn.core.policy.AbstractPolicy;
    +import org.apache.brooklyn.core.sensor.BasicAttributeSensor;
    +import org.apache.brooklyn.util.collections.MutableMap;
    +import org.apache.brooklyn.util.core.flags.TypeCoercions;
    +import org.apache.brooklyn.util.guava.Maybe;
    +import org.apache.brooklyn.util.text.Strings;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import com.google.common.collect.Sets;
    +import com.google.common.reflect.TypeToken;
    +
    +/**
    + * Subscribes to events on a collection {@link AttributeSensor} and invokes the named
    + * effectors for each element that was added and removed.
    + * <p>
    + * The policy only detects <em>replacements</em> of the collection; it does not act on
    + * modifications. If the sensor has value <code>A</code> and an element is added &ndash;
    + * value <code>A'</code> &ndash; the on-added effector is not invoked. If the sensor is
    + * later set to <code>B</code> the delta is made between <code>A</code> and <code>B</code>,
    + * not <code>A'</code> and <code>B</code>.
    + * <p>
    + * To simplify the detection of additions and removals the collection is converted to a
    + * {@link Set}. This means that only a single event will fire for duplicate elements in
    + * the collection.
    + * <p>
    + * The effectors are provided the elements that changed in their parameter map. If the
    + * sensor is a collection of maps the elements are provided with their keys coerced to
    + * strings and their values unchanged. Otherwise the elements are provided in a
    + * single-entry map keyed by the value for {@link #PARAMETER_NAME}.
    + */
    +public class InvokeEffectorOnSetChange extends AbstractPolicy implements SensorEventListener<Collection<?>> {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(InvokeEffectorOnSetChange.class);
    +
    +    public static final ConfigKey<AttributeSensor<? extends Collection<?>>> TRIGGER_SENSOR = ConfigKeys.newConfigKey(
    +            new TypeToken<AttributeSensor<? extends Collection<?>>>() {},
    +            "sensor",
    +            "Sensor to be monitored.");
    +
    +    public static final ConfigKey<String> PREVIOUS_SENSOR_NAME = ConfigKeys.newStringConfigKey(
    +            "previousSensorName",
    +            "The name under which the previous value for the trigger sensor should be published. " +
    +                    "If unset \".previous\" will be appended to the name of the trigger sensor.");
    +
    +    public static final ConfigKey<String> ON_ADDED_EFFECTOR_NAME = ConfigKeys.newStringConfigKey(
    +            "onAdded",
    +            "Name of the effector to invoke when entries are added to the collection.");
    +
    +    public static final ConfigKey<String> ON_REMOVED_EFFECTOR_NAME = ConfigKeys.newStringConfigKey(
    +            "onRemoved",
    +            "Name of the effector to invoke when entries are removed from the collection.");
    +
    +    public static final ConfigKey<String> PARAMETER_NAME = ConfigKeys.newStringConfigKey(
    +            "parameterName",
    +            "The name of the parameter to supply to the effectors",
    +            "value");
    +
    +    /** The previous version of the set against which events will be compared. */
    +    private Set<Object> previous = Collections.emptySet();
    +
    +    /** Guards accesses of previous. */
    +    private final Object[] updateLock = new Object[0];
    +
    +    @Override
    +    public void setEntity(EntityLocal entity) {
    +        super.setEntity(entity);
    +        Sensor<? extends Collection<?>> sensor =
    +                checkNotNull(getConfig(TRIGGER_SENSOR), "Value required for " + TRIGGER_SENSOR.getName());
    +
    +        checkArgument(Strings.isNonBlank(getConfig(PARAMETER_NAME)), "Value required for " + PARAMETER_NAME.getName());
    +
    +        // Fail straight away if neither effector is found.
    +        if (getEffector(getOnAddedEffector()).isAbsentOrNull() &&
    +                getEffector(getOnRemovedEffector()).isAbsentOrNull()) {
    +            throw new IllegalArgumentException("Value required for one or both of " + ON_ADDED_EFFECTOR_NAME.getName() +
    +                    " and " + ON_REMOVED_EFFECTOR_NAME.getName());
    +        }
    +
    +        // Initialise `present` before subscribing.
    +        Collection<?> current = entity.sensors().get(getTriggerSensor());
    +        synchronized (updateLock) {
    +            previous = (current != null) ? new HashSet<>(current) : Collections.emptySet();
    +        }
    +        subscriptions().subscribe(entity, sensor, this);
    +    }
    +
    +    @Override
    +    public void onEvent(SensorEvent<Collection<?>> event) {
    +        final Set<Object> added = new HashSet<>(), removed = new HashSet<>();
    +        Set<Object> newValue = new HashSet<>(event.getValue());
    --- End diff --
    
    Could use linked set to keep iteration order.


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