[GitHub] brooklyn-server pull request: Configurable removal strategies

[asfgit]

Github user asfgit closed the pull request at:

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


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request: Configurable removal strategies

[neykov]

Github user neykov commented on the pull request:

https://github.com/apache/brooklyn-server/pull/112#issuecomment-218084476
  
@nakomis Didn't realise this can be used with $broolyn:object. That's 
enough YAML friendly for me :).


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request: Configurable removal strategies

[nakomis]

Github user nakomis commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/112#discussion_r62474640
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/entity/group/FirstFromRemovalStrategy.java
 ---
@@ -0,0 +1,53 @@
+/*
+ * 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.entity.group;
+
+import java.util.Collection;
+import java.util.List;
+
+import javax.annotation.Nullable;
+
+import org.apache.brooklyn.api.entity.Entity;
+import org.apache.brooklyn.config.ConfigKey;
+import org.apache.brooklyn.core.config.ConfigKeys;
+
+import com.google.common.collect.Iterables;
+import com.google.common.reflect.TypeToken;
+
+public class FirstFromRemovalStrategy extends RemovalStrategy {
+
+public static final ConfigKey STRATEGIES = 
ConfigKeys.newConfigKey(new TypeToken() {}, 
"firstfrom.strategies",
+"An ordered list of removal strategies to be used to determine 
which entity to remove");
+
+@Nullable
+@Override
+public Entity apply(@Nullable Collection input) {
+List strategies = config().get(STRATEGIES);
--- End diff --

The benefit of using `BasicConfigurableObject` is that the configuration is 
the configuration (i.e. setting of ConfigKeys) is managed automatically by the 
`ConfigurationSupport`, rather than implementors having to read the values 
directly from the properties passed to the constructor


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request: Configurable removal strategies

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/112#discussion_r62239950
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/entity/group/FirstFromRemovalStrategy.java
 ---
@@ -0,0 +1,53 @@
+/*
+ * 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.entity.group;
+
+import java.util.Collection;
+import java.util.List;
+
+import javax.annotation.Nullable;
+
+import org.apache.brooklyn.api.entity.Entity;
+import org.apache.brooklyn.config.ConfigKey;
+import org.apache.brooklyn.core.config.ConfigKeys;
+
+import com.google.common.collect.Iterables;
+import com.google.common.reflect.TypeToken;
+
+public class FirstFromRemovalStrategy extends RemovalStrategy {
+
+public static final ConfigKey STRATEGIES = 
ConfigKeys.newConfigKey(new TypeToken() {}, 
"firstfrom.strategies",
+"An ordered list of removal strategies to be used to determine 
which entity to remove");
+
+@Nullable
+@Override
+public Entity apply(@Nullable Collection input) {
+List strategies = config().get(STRATEGIES);
--- End diff --

Following up on @neykov's point - from a yaml perspective, how would one 
use this currently? What is the advantage of having `RemovalStrategy extends 
BasicConfigurableObject`? Are you expecting people to use (in yaml) the 
`$brooklyn:object` syntax to instantiate the removal strategy? (if so, they can 
use `brooklyn.config` within the `$brooklyn:object:`). I still don't see the 
benefit of that over a pojo.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request: Configurable removal strategies

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/112#discussion_r62237872
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/entity/group/SensorMatchingRemovalStrategy.java
 ---
@@ -0,0 +1,50 @@
+/*
+ * 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.entity.group;
+
+import java.util.Collection;
+import java.util.Objects;
+
+import javax.annotation.Nullable;
+
+import org.apache.brooklyn.api.entity.Entity;
+import org.apache.brooklyn.api.sensor.AttributeSensor;
+import org.apache.brooklyn.config.ConfigKey;
+import org.apache.brooklyn.core.config.ConfigKeys;
+
+import com.google.common.reflect.TypeToken;
+
+public class SensorMatchingRemovalStrategy extends RemovalStrategy {
+public static final ConfigKey SENSOR = 
ConfigKeys.newConfigKey(new TypeToken() {}, 
"sensor.matching.sensor");
+// Would be nice to use ConfigKey, but TypeToken cannot be 
instantiated at runtime
+public static final ConfigKey DESIRED_VALUE = 
ConfigKeys.newConfigKey(new TypeToken() {}, "sensor.matching.value");
--- End diff --

Ah, it's a brand new class - in which case yes, change to just 
`AttributeSensor.class` and `Object.class`.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request: Configurable removal strategies

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/112#discussion_r62227937
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/entity/group/SensorMatchingRemovalStrategy.java
 ---
@@ -0,0 +1,50 @@
+/*
+ * 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.entity.group;
+
+import java.util.Collection;
+import java.util.Objects;
+
+import javax.annotation.Nullable;
+
+import org.apache.brooklyn.api.entity.Entity;
+import org.apache.brooklyn.api.sensor.AttributeSensor;
+import org.apache.brooklyn.config.ConfigKey;
+import org.apache.brooklyn.core.config.ConfigKeys;
+
+import com.google.common.reflect.TypeToken;
+
+public class SensorMatchingRemovalStrategy extends RemovalStrategy {
+public static final ConfigKey SENSOR = 
ConfigKeys.newConfigKey(new TypeToken() {}, 
"sensor.matching.sensor");
+// Would be nice to use ConfigKey, but TypeToken cannot be 
instantiated at runtime
+public static final ConfigKey DESIRED_VALUE = 
ConfigKeys.newConfigKey(new TypeToken() {}, "sensor.matching.value");
--- End diff --

@neykov scarily, changing the use of TypeToken will renumber all the 
anonymous inner classes below it! If anyone has persisted those, it will cause 
problems.

We really do need to get to be stricter by default, that we won't accept 
persisting anonymous inner classes!


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request: Configurable removal strategies

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/112#discussion_r62227490
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/entity/group/DynamicClusterImpl.java ---
@@ -151,12 +151,14 @@ public ZoneFailureDetector apply(final String input) {
  */
 protected final Object mutex = new Object[0];
 
-private static final Function 
defaultRemovalStrategy = new Function() {
-@Override public Entity apply(Collection contenders) {
--- End diff --

Agreed, let's be paranoid. Mark it as deprecated, with a comment saying why 
we have not deleted it.

There are two ways changing your anonymous inner class can break 
persistence: one is if someone has persisted it; two is if deleted or moving 
the class changes the classnames of other anonymous inner classes (there are 5 
of them in DynamicClusterImpl)!


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request: Configurable removal strategies

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/112#discussion_r59550901
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/entity/group/DynamicClusterImpl.java ---
@@ -151,12 +151,14 @@ public ZoneFailureDetector apply(final String input) {
  */
 protected final Object mutex = new Object[0];
 
-private static final Function 
defaultRemovalStrategy = new Function() {
-@Override public Entity apply(Collection contenders) {
--- End diff --

Rule of thumb is that anything that goes into a config or sensor (or is 
nested in such an object) will get persisted. Looking at the code this is not 
the case, but can never know what an extending class might be doing, so better 
be on the safe side.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request: Configurable removal strategies

[sjcorbett]

Github user sjcorbett commented on the pull request:

https://github.com/apache/brooklyn-server/pull/112#issuecomment-208918399
  
Looks much cleaner. I'm wary that turning `defaultRemovalStrategy` from an 
anonymous inner class to a named inner class will be problematic for anyone 
with persisted state (because the `$1`, `$2` etc. numbering will change). Given 
that it's not imperative for this feature are you ok to revert that particular 
portion?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request: Configurable removal strategies

[nakomis]

Github user nakomis commented on the pull request:

https://github.com/apache/brooklyn-server/pull/112#issuecomment-208879479
  
Hadn't twigged that I wouldn't need to change the config key. Much nice 
without all the deprecation. Have close 
https://github.com/apache/brooklyn-library/pull/31 as it's no longer needed


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request: Configurable removal strategies

[sjcorbett]

Github user sjcorbett commented on the pull request:

https://github.com/apache/brooklyn-server/pull/112#issuecomment-208845961
  
Thinking about this a bit more closely, you've deprecated a 
`ConfigKey>` in favour of a 
`ConfigKey`, but the `RemovalStrategy` class implements 
`Function`. The 
result is identical, no?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request: Configurable removal strategies

[sjcorbett]

Github user sjcorbett commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/112#discussion_r59355280
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/entity/group/DynamicCluster.java ---
@@ -148,6 +148,11 @@
 ConfigKey FACTORY = ConfigKeys.newConfigKey(
 EntityFactory.class, "dynamiccluster.factory", "factory for 
creating new cluster members", null);
 
+
+@SetFromFlag("removalStrategy")
--- End diff --

`@SetFromFlag("removalStrategy")` is on both this and the deprecated key. 
Which one wins?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request: Configurable removal strategies

[sjcorbett]

Github user sjcorbett commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/112#discussion_r59355178
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/entity/group/DynamicClusterImpl.java ---
@@ -151,12 +151,14 @@ public ZoneFailureDetector apply(final String input) {
  */
 protected final Object mutex = new Object[0];
 
-private static final Function 
defaultRemovalStrategy = new Function() {
-@Override public Entity apply(Collection contenders) {
--- End diff --

Again on persistence. @aledsage is deleting this anonymous inner class 
safe? (seems a little silly to have to ask!)


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request: Configurable removal strategies

[sjcorbett]

Github user sjcorbett commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/112#discussion_r59354661
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/entity/group/DynamicCluster.java ---
@@ -148,6 +148,11 @@
 ConfigKey FACTORY = ConfigKeys.newConfigKey(
 EntityFactory.class, "dynamiccluster.factory", "factory for 
creating new cluster members", null);
 
+
+@SetFromFlag("removalStrategy")
+ConfigKey CONFIGURABLE_REMOVAL_STRATEGY = 
ConfigKeys.newConfigKey(RemovalStrategy.class, 
"dynamiccluster.configurable.removalStrategy");
+
+@Deprecated /** @deprecated since 0.9.0; use {@link 
CONFIGURABLE_REMOVAL_STRATEGY instead} */
--- End diff --

`{@link CONFIGURABLE_REMOVAL_STRATEGY instead}` should be `{@link 
CONFIGURABLE_REMOVAL_STRATEGY} instead`.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request: Configurable removal strategies

[nakomis]

Github user nakomis commented on the pull request:

https://github.com/apache/brooklyn-server/pull/112#issuecomment-208827153
  
Depended on by https://github.com/apache/brooklyn-library/pull/31 to be 
merged at the same time


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request: Configurable removal strategies

[nakomis]

GitHub user nakomis opened a pull request:

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

Configurable removal strategies



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

$ git pull https://github.com/nakomis/brooklyn-server 
configurable-removal-strategies

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

https://github.com/apache/brooklyn-server/pull/112.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 #112


commit 8caad30d762be992d918b8b2eba9c34a99f7d217
Author: Martin Harris 
Date:   2016-04-11T15:03:19Z

Adds configurable removal strategies, deprecates old removal strategy

commit d3a58ff00108ae75408963fab503526fb5e4a5f3
Author: Martin Harris 
Date:   2016-04-11T15:15:53Z

Makes default configurable removal strategy a public class, so it can be 
referred to in YAML

commit 56638365b7fdc7516545aed64125dfe7eb777fc0
Author: Martin Harris 
Date:   2016-04-12T09:57:51Z

Adds test for removal strategies




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---