You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by ahgittin <gi...@git.apache.org> on 2016/11/24 17:17:34 UTC
[GitHub] brooklyn-server pull request #462: Inherit config default values
GitHub user ahgittin opened a pull request:
https://github.com/apache/brooklyn-server/pull/462
Inherit config default values
resolve https://issues.apache.org/jira/browse/BROOKLYN-267
also during deserializing migrate to non-deprecated classes
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ahgittin/brooklyn-server inherit-config-default-values
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/462.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 #462
----
commit 9fb6f35e8b2c694168c06a39b7fcc922b0ece8fc
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-11-24T10:56:46Z
migrate config inheritance to new classes
pioneers use of `readResolve` that actually works brilliantly out of the box due to xstream
also tidying `BasicConfigInheritance` and adding a placeholder (not used yet)
for resolving ancestor defaults
includes tests for config inheritance serialization
commit 1d5cafca0195275758518e7bba3541ff29298c89
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-11-24T16:29:21Z
document better config inheritance semantics
commit 79bb11957be95e525f126d9c7c301635a9fa6404
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-11-17T15:12:09Z
implement inheritance of config default values
test and code changes to respect the additional inheritance argument added in the previous request:
resolves https://issues.apache.org/jira/browse/BROOKLYN-267
----
---
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 #462: Inherit config default values
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r89772773
--- Diff: core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindConfigInheritanceTest.java ---
@@ -0,0 +1,145 @@
+/*
+ * 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.core.mgmt.rebind;
+
+import java.io.File;
+import java.io.FileReader;
+
+import org.apache.brooklyn.api.entity.Application;
+import org.apache.brooklyn.api.entity.EntitySpec;
+import org.apache.brooklyn.config.ConfigInheritance;
+import org.apache.brooklyn.config.ConfigKey;
+import org.apache.brooklyn.core.config.BasicConfigInheritance;
+import org.apache.brooklyn.core.config.ConfigKeys;
+import org.apache.brooklyn.core.config.ConfigKeys.InheritanceContext;
+import org.apache.brooklyn.core.config.ConfigPredicates;
+import org.apache.brooklyn.core.entity.EntityAsserts;
+import org.apache.brooklyn.core.test.entity.TestEntity;
+import org.apache.brooklyn.test.Asserts;
+import org.apache.brooklyn.util.os.Os;
+import org.apache.brooklyn.util.stream.Streams;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.testng.Assert;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import com.google.common.base.Charsets;
+import com.google.common.base.Joiner;
+import com.google.common.collect.Iterables;
+import com.google.common.io.Files;
+
+public class RebindConfigInheritanceTest extends RebindTestFixtureWithApp {
+
+ private static final Logger log = LoggerFactory.getLogger(RebindConfigInheritanceTest.class);
+
+ ConfigKey<String> key1 = ConfigKeys.builder(String.class, "key1").runtimeInheritance(BasicConfigInheritance.NEVER_INHERITED).build();
+ String origMemento, newMemento;
+ Application rebindedApp;
+
+ @Override
+ @BeforeMethod(alwaysRun=true)
+ public void setUp() throws Exception {
+ super.setUp();
+ }
+
+ protected void doReadConfigInheritance(String label, String entityId) throws Exception {
+ String mementoFilename = "config-inheritance-"+label+"-"+entityId;
+ origMemento = Streams.readFullyString(getClass().getResourceAsStream(mementoFilename));
+
+ File persistedEntityFile = new File(mementoDir, Os.mergePaths("entities", entityId));
+ Files.write(origMemento.getBytes(), persistedEntityFile);
+
+ // we'll make assertions on what we've loaded
+ rebind();
+ rebindedApp = (Application) newManagementContext.lookup(entityId);
+
+ // we'll also make assertions on the contents written out
+ RebindTestUtils.waitForPersisted(mgmt());
+ newMemento = Joiner.on("\n").join(Files.readLines(persistedEntityFile, Charsets.UTF_8));
+ }
+
+ /** also see {@link RebindWithDeserializingClassRenamesTest} */
+ @Test
+ public void testPreBasicConfigInheritance_2016_07() throws Exception {
+ doReadConfigInheritance("prebasic-2016-07", "toruf2wxg4");
+
+ ConfigKey<?> k = Iterables.getOnlyElement( rebindedApp.config().findKeys(ConfigPredicates.nameEqualTo("my.config.inheritanceMerged")) );
+
+ Asserts.assertStringContains(origMemento, "<parentInheritance class=\"org.apache.brooklyn.config.ConfigInheritance$Merged\"/>");
+ Asserts.assertStringDoesNotContain(origMemento, BasicConfigInheritance.DEEP_MERGE.getClass().getName());
+
+ // should now convert it to BasicConfigInheritance.DEEP_MERGE
+ Asserts.assertStringDoesNotContain(newMemento, "ConfigInheritance$Merged");
+ Asserts.assertStringDoesNotContain(newMemento, "ConfigInheritance$Legacy$Merged");
+ Asserts.assertStringContains(newMemento, BasicConfigInheritance.DEEP_MERGE.getClass().getName());
+
+ ConfigInheritance inh = k.getInheritanceByContext(InheritanceContext.RUNTIME_MANAGEMENT);
+ Assert.assertEquals(inh, BasicConfigInheritance.DEEP_MERGE);
+ }
+
+ @Test
+ public void testBasicConfigInheritance_2016_10() throws Exception {
+ doReadConfigInheritance("basic-2016-10", "wj5s8u9h73");
+
+ checkNewAppNonInheritingKey1(rebindedApp);
+
+ Asserts.assertStringContains(origMemento, "isReinherited");
+ Asserts.assertStringDoesNotContain(origMemento, "NEVER_INHERITED");
+ Asserts.assertStringDoesNotContain(origMemento, "NeverInherited");
+
+ // should write it out as NeverInherited
+ Asserts.assertStringDoesNotContain(newMemento, "isReinherited");
+ Asserts.assertStringContains(newMemento, "NeverInherited");
+ }
+
+ @Test
+ public void testReadConfigInheritance_2016_11() throws Exception {
+ doReadConfigInheritance("basic-2016-11", "kmpez5fznt");
+ checkNewAppNonInheritingKey1(rebindedApp);
+
+ String origMementoWithoutLicenseHeader = origMemento.substring(origMemento.indexOf("<entity>"));
+ Asserts.assertEquals(origMementoWithoutLicenseHeader, newMemento);
+ }
+
+ @Test
+ public void testBasicConfigInheritanceProgrammatic() throws Exception {
+ origApp.config().set(key1, "1");
+
+ RebindOptions opts = RebindOptions.create();
+ opts.keepSameInstance = true;
+
+ rebind(opts);
+
+ String entityFile = Streams.readFully(new FileReader(new File(opts.mementoDir, "entities/"+origApp.getApplicationId())));
--- End diff --
You can just use `new File(mementoDir, ...)` (i.e. using the field), rather than using `keepSameInstance`. I'd favour deleting support for that. It feels like an anti-pattern, which combined with a `create(...)` method.
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r90568817
--- Diff: core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindConfigInheritanceTest.java ---
@@ -0,0 +1,145 @@
+/*
+ * 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.core.mgmt.rebind;
+
+import java.io.File;
+import java.io.FileReader;
+
+import org.apache.brooklyn.api.entity.Application;
+import org.apache.brooklyn.api.entity.EntitySpec;
+import org.apache.brooklyn.config.ConfigInheritance;
+import org.apache.brooklyn.config.ConfigKey;
+import org.apache.brooklyn.core.config.BasicConfigInheritance;
+import org.apache.brooklyn.core.config.ConfigKeys;
+import org.apache.brooklyn.core.config.ConfigKeys.InheritanceContext;
+import org.apache.brooklyn.core.config.ConfigPredicates;
+import org.apache.brooklyn.core.entity.EntityAsserts;
+import org.apache.brooklyn.core.test.entity.TestEntity;
+import org.apache.brooklyn.test.Asserts;
+import org.apache.brooklyn.util.os.Os;
+import org.apache.brooklyn.util.stream.Streams;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.testng.Assert;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import com.google.common.base.Charsets;
+import com.google.common.base.Joiner;
+import com.google.common.collect.Iterables;
+import com.google.common.io.Files;
+
+public class RebindConfigInheritanceTest extends RebindTestFixtureWithApp {
+
+ private static final Logger log = LoggerFactory.getLogger(RebindConfigInheritanceTest.class);
+
+ ConfigKey<String> key1 = ConfigKeys.builder(String.class, "key1").runtimeInheritance(BasicConfigInheritance.NEVER_INHERITED).build();
+ String origMemento, newMemento;
+ Application rebindedApp;
+
+ @Override
+ @BeforeMethod(alwaysRun=true)
+ public void setUp() throws Exception {
+ super.setUp();
+ }
+
+ protected void doReadConfigInheritance(String label, String entityId) throws Exception {
+ String mementoFilename = "config-inheritance-"+label+"-"+entityId;
+ origMemento = Streams.readFullyString(getClass().getResourceAsStream(mementoFilename));
+
+ File persistedEntityFile = new File(mementoDir, Os.mergePaths("entities", entityId));
+ Files.write(origMemento.getBytes(), persistedEntityFile);
+
+ // we'll make assertions on what we've loaded
+ rebind();
+ rebindedApp = (Application) newManagementContext.lookup(entityId);
+
+ // we'll also make assertions on the contents written out
+ RebindTestUtils.waitForPersisted(mgmt());
+ newMemento = Joiner.on("\n").join(Files.readLines(persistedEntityFile, Charsets.UTF_8));
+ }
+
+ /** also see {@link RebindWithDeserializingClassRenamesTest} */
+ @Test
+ public void testPreBasicConfigInheritance_2016_07() throws Exception {
+ doReadConfigInheritance("prebasic-2016-07", "toruf2wxg4");
+
+ ConfigKey<?> k = Iterables.getOnlyElement( rebindedApp.config().findKeys(ConfigPredicates.nameEqualTo("my.config.inheritanceMerged")) );
+
+ Asserts.assertStringContains(origMemento, "<parentInheritance class=\"org.apache.brooklyn.config.ConfigInheritance$Merged\"/>");
+ Asserts.assertStringDoesNotContain(origMemento, BasicConfigInheritance.DEEP_MERGE.getClass().getName());
+
+ // should now convert it to BasicConfigInheritance.DEEP_MERGE
+ Asserts.assertStringDoesNotContain(newMemento, "ConfigInheritance$Merged");
+ Asserts.assertStringDoesNotContain(newMemento, "ConfigInheritance$Legacy$Merged");
+ Asserts.assertStringContains(newMemento, BasicConfigInheritance.DEEP_MERGE.getClass().getName());
+
+ ConfigInheritance inh = k.getInheritanceByContext(InheritanceContext.RUNTIME_MANAGEMENT);
+ Assert.assertEquals(inh, BasicConfigInheritance.DEEP_MERGE);
+ }
+
+ @Test
+ public void testBasicConfigInheritance_2016_10() throws Exception {
+ doReadConfigInheritance("basic-2016-10", "wj5s8u9h73");
+
+ checkNewAppNonInheritingKey1(rebindedApp);
+
+ Asserts.assertStringContains(origMemento, "isReinherited");
+ Asserts.assertStringDoesNotContain(origMemento, "NEVER_INHERITED");
+ Asserts.assertStringDoesNotContain(origMemento, "NeverInherited");
+
+ // should write it out as NeverInherited
+ Asserts.assertStringDoesNotContain(newMemento, "isReinherited");
+ Asserts.assertStringContains(newMemento, "NeverInherited");
+ }
+
+ @Test
+ public void testReadConfigInheritance_2016_11() throws Exception {
+ doReadConfigInheritance("basic-2016-11", "kmpez5fznt");
+ checkNewAppNonInheritingKey1(rebindedApp);
+
+ String origMementoWithoutLicenseHeader = origMemento.substring(origMemento.indexOf("<entity>"));
+ Asserts.assertEquals(origMementoWithoutLicenseHeader, newMemento);
+ }
+
+ @Test
+ public void testBasicConfigInheritanceProgrammatic() throws Exception {
+ origApp.config().set(key1, "1");
+
+ RebindOptions opts = RebindOptions.create();
+ opts.keepSameInstance = true;
+
+ rebind(opts);
+
+ String entityFile = Streams.readFully(new FileReader(new File(opts.mementoDir, "entities/"+origApp.getApplicationId())));
--- End diff --
d'oh. thought it was awfully hard to find that! agree, will remove keepSameInstance.
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100786370
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java ---
@@ -397,48 +397,83 @@ protected ConfigInheritance getDefaultRuntimeInheritance() {
final InheritanceContext context = InheritanceContext.RUNTIME_MANAGEMENT;
ConfigInheritance currentInheritance = ConfigInheritances.findInheritance(queryKey, context, getDefaultRuntimeInheritance());
-
+
BasicConfigValueAtContainer<TContainer, Object> last = null;
-
+
while (c!=null) {
Maybe<Object> v = getRawValueAtContainer(c, queryKey);
BasicConfigValueAtContainer<TContainer, Object> next = new BasicConfigValueAtContainer<TContainer, Object>(c, getKeyAtContainer(c, queryKey), v);
-
+
if (last!=null && !currentInheritance.considerParent(last, next, context)) break;
-
+
ConfigInheritance currentInheritanceExplicit = ConfigInheritances.findInheritance(next.getKey(), InheritanceContext.RUNTIME_MANAGEMENT, null);
if (currentInheritanceExplicit!=null) {
if (count>0 && !currentInheritanceExplicit.isReinheritable(next, context)) break;
currentInheritance = currentInheritanceExplicit;
}
-
+
if (next.isValueExplicitlySet()) result.add(0, next);
last = next;
c = getParentOfContainer(c);
count++;
}
-
+
return result;
}
-
- @Override
+
+ @Override @Deprecated
public Set<ConfigKey<?>> findKeys(Predicate<? super ConfigKey<?>> filter) {
+ return findKeys(filter, KeyFindingMode.PRESENT_NOT_RESOLVED);
+ }
+
+ @Override
+ public Set<ConfigKey<?>> findKeysDeclared(Predicate<? super ConfigKey<?>> filter) {
+ return findKeys(filter, KeyFindingMode.DECLARED_OR_PRESENT);
+ }
+
+ @Override
+ public Set<ConfigKey<?>> findKeysPresent(Predicate<? super ConfigKey<?>> filter) {
+ return findKeys(filter, KeyFindingMode.PRESENT_BUT_RESOLVED);
+ }
+
+ protected enum KeyFindingMode { DECLARED_OR_PRESENT, PRESENT_BUT_RESOLVED, PRESENT_NOT_RESOLVED }
--- End diff --
agree
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin closed the pull request at:
https://github.com/apache/brooklyn-server/pull/462
---
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 #462: Inherit config default values
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/462#discussion_r94785113
--- Diff: camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/SpecParameterUnwrappingTest.java ---
@@ -146,8 +151,9 @@ public void testRootParametersUnwrapped() {
CatalogItem<?, ?> item = catalog.getCatalogItem(SYMBOLIC_NAME, TEST_VERSION);
AbstractBrooklynObjectSpec<?,?> spec = catalog.peekSpec(item);
List<SpecParameter<?>> params = spec.getParameters();
- assertEquals(params.size(), NUM_APP_DEFAULT_CONFIG_KEYS + 1, "params="+params);
assertTrue(Iterables.tryFind(params, nameEqualTo("simple")).isPresent());
+ assertTrue(Iterables.tryFind(params, nameEqualTo(SHARED_CONFIG.getName())).isPresent());
+ assertEquals(params.size(), NUM_APP_DEFAULT_CONFIG_KEYS + 2, "params="+params);
--- End diff --
Unrelated, but in the case of unwrapping the spec because we are expecting a non-app spec it's surprising to inherit the wrapper app config (and anything else from 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 #462: Inherit config default values
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r94440253
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java ---
@@ -28,51 +32,188 @@
import org.apache.brooklyn.config.ConfigKey;
import org.apache.brooklyn.config.ConfigValueAtContainer;
import org.apache.brooklyn.util.collections.CollectionMerger;
+import org.apache.brooklyn.util.exceptions.Exceptions;
import org.apache.brooklyn.util.exceptions.ReferenceWithError;
import org.apache.brooklyn.util.guava.Maybe;
+import org.apache.brooklyn.util.text.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+@SuppressWarnings("serial")
public class BasicConfigInheritance implements ConfigInheritance {
+ private static final Logger log = LoggerFactory.getLogger(BasicConfigInheritance.class);
+
private static final long serialVersionUID = -5916548049057961051L;
public static final String CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE = "deep_merge";
public static final String CONFLICT_RESOLUTION_STRATEGY_OVERWRITE = "overwrite";
+ public static abstract class DelegatingConfigInheritance implements ConfigInheritance {
+ protected abstract ConfigInheritance getDelegate();
+
+ @Override @Deprecated public InheritanceMode isInherited(ConfigKey<?> key, Object from, Object to) {
+ return getDelegate().isInherited(key, from, to);
+ }
+ @Override public <TContainer, TValue> boolean isReinheritable(ConfigValueAtContainer<TContainer, TValue> parent, ConfigInheritanceContext context) {
+ return getDelegate().isReinheritable(parent, context);
+ }
+ @Override public <TContainer, TValue> boolean considerParent(ConfigValueAtContainer<TContainer, TValue> local, ConfigValueAtContainer<TContainer, TValue> parent, ConfigInheritanceContext context) {
+ return getDelegate().considerParent(local, parent, context);
+ }
+ @Override
+ public <TContainer, TValue> ReferenceWithError<ConfigValueAtContainer<TContainer, TValue>> resolveWithParent(ConfigValueAtContainer<TContainer, TValue> local, ConfigValueAtContainer<TContainer, TValue> resolvedParent, ConfigInheritanceContext context) {
+ return getDelegate().resolveWithParent(local, resolvedParent, context);
+ }
+ @Override
+ public boolean equals(Object obj) {
+ return super.equals(obj) || getDelegate().equals(obj);
+ }
+
+ // standard deserialization method
+ protected ConfigInheritance readResolve() {
+ return returnEquivalentConstant(this);
+ }
+ }
+
+ private static ConfigInheritance returnEquivalentConstant(ConfigInheritance candidate) {
+ for (ConfigInheritance knownMode: Arrays.asList(
+ NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, NEVER_INHERITED, OVERWRITE, BasicConfigInheritance.DEEP_MERGE)) {
+ if (candidate.equals(knownMode)) return knownMode;
+ }
+ if (candidate.equals(new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, true))) {
+ // ignore the ancestor flag for this mode
+ return NEVER_INHERITED;
+ }
+ return candidate;
+ }
+
+ /*
+ * use of delegate is so that stateless classes can be defined to make the serialization nice,
+ * both the name and hiding the implementation detail (also making it easier for that detail to change);
+ * with aliased type names the field names here could even be aliases for the type names.
+ * (we could alternatively have an "alias-for-final-instance" mode in serialization,
+ * to optimize where we know that a java instance is final.)
+ */
+
+ private static class NotReinherited extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** Indicates that a config key value should not be passed down from a container where it is defined.
* Unlike {@link #NEVER_INHERITED} these values can be passed down if set as anonymous keys at a container
* (ie the container does not expect it) to a container which does expect it, but it will not be passed down further.
* If the inheritor also defines a value the parent's value is ignored irrespective
* (as in {@link #OVERWRITE}; see {@link #NOT_REINHERITED_ELSE_DEEP_MERGE} if merging is desired). */
- public static BasicConfigInheritance NOT_REINHERITED = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_OVERWRITE,false);
+ public static ConfigInheritance NOT_REINHERITED = new NotReinherited();
+
+ private static class NotReinheritedElseDeepMerge extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** As {@link #NOT_REINHERITED} but in cases where a value is inherited because a parent did not recognize it,
* if the inheritor also defines a value the two values should be merged. */
- public static BasicConfigInheritance NOT_REINHERITED_ELSE_DEEP_MERGE = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE,false);
- /** Indicates that a key's value should never be inherited, even if defined on a container that does not know the key.
- * Most usages will prefer {@link #NOT_REINHERITED}. */
- public static BasicConfigInheritance NEVER_INHERITED = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_OVERWRITE,true);
+ public static ConfigInheritance NOT_REINHERITED_ELSE_DEEP_MERGE = new NotReinheritedElseDeepMerge();
+
+ private static class NeverInherited extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, false);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
+ /** Indicates that a key's value should never be inherited, even if inherited from a value set on a container that does not know the key.
+ * (Most usages will prefer {@link #NOT_REINHERITED}.) */
+ public static ConfigInheritance NEVER_INHERITED = new NeverInherited();
+
+ private static class Overwrite extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(true, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** Indicates that if a key has a value at both an ancestor and a descendant, the descendant and his descendants
* will prefer the value at the descendant. */
- public static BasicConfigInheritance OVERWRITE = new BasicConfigInheritance(true,CONFLICT_RESOLUTION_STRATEGY_OVERWRITE,false);
+ public static ConfigInheritance OVERWRITE = new Overwrite();
+
+ private static class DeepMerge extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(true, CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** Indicates that if a key has a value at both an ancestor and a descendant, the descendant and his descendants
- * should attempt to merge the values. If the values are not mergable behaviour is undefined
+ * should attempt to merge the values. If the values are not mergeable behaviour is undefined
* (and often the descendant's value will simply overwrite). */
- public static BasicConfigInheritance DEEP_MERGE = new BasicConfigInheritance(true,CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE,false);
+ public static ConfigInheritance DEEP_MERGE = new DeepMerge();
+
+ // support conversion from these legacy fields
+ @SuppressWarnings("deprecation")
+ private static void registerReplacements() {
+ ConfigInheritance.Legacy.registerReplacement(ConfigInheritance.DEEP_MERGE, DEEP_MERGE);
+ ConfigInheritance.Legacy.registerReplacement(ConfigInheritance.ALWAYS, OVERWRITE);
+ ConfigInheritance.Legacy.registerReplacement(ConfigInheritance.NONE, NOT_REINHERITED);
+ }
+ static { registerReplacements(); }
- /** reinheritable? true/false; if false, children/descendants/inheritors will never see it; default true */
+ /** whether a value on a key defined locally should be inheritable by descendants.
+ * if false at a point where a key is defined,
+ * children/descendants/inheritors will not be able to see its value, whether explicit or default.
+ * default true: things are normally reinherited.
+ * <p>
+ * note that this only takes effect where a key is defined locally.
+ * if a key is not defined at an ancestor, a descendant setting this value false will not prevent it
+ * from inheriting values from ancestors.
+ * <p>
+ * typical use case for setting this is false is where a key is consumed and descendants should not
+ * "reconsume" it. for example setting files to install on a VM need only be applied once,
+ * and if it has <b>runtime management</b> hierarchy descendants which also understand that field they
+ * should not install the same files.
+ * (there is normally no reason to set this false in the context of <b>type hierarchy</b> inheritance because
+ * an ancestor type cannot "consume" a value.) */
protected final boolean isReinherited;
- /** conflict-resolution-strategy? in {@link BasicConfigInheritance} supported values are
+
+ /** a symbol indicating a conflict-resolution-strategy understood by the implementation.
+ * in {@link BasicConfigInheritance} supported values are
* {@link #CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE} and {@link #CONFLICT_RESOLUTION_STRATEGY_OVERWRITE}.
- * subclasses may pass null if they provide a custom implementaton of {@link #resolveWithParentCustomStrategy(ConfigValueAtContainer, ConfigValueAtContainer, org.apache.brooklyn.config.ConfigInheritance.ConfigInheritanceContext)} */
+ * subclasses may pass null or a different string if they provide a custom implementaton
+ * of {@link #resolveWithParentCustomStrategy(ConfigValueAtContainer, ConfigValueAtContainer, org.apache.brooklyn.config.ConfigInheritance.ConfigInheritanceContext)} */
@Nullable protected final String conflictResolutionStrategy;
- /** use-local-default-value? true/false; if true, overwrite above means "always ignore (even if null)"; default false
- * whereas merge means "use local default (if non-null)" */
- protected final boolean useLocalDefaultValue;
+
+ /** @deprecated since 0.10.0 when this was introduced, now renamed {@link #localDefaultResolvesWithAncestorValue} */
+ @Deprecated protected final Boolean useLocalDefaultValue;
+ /** whether a local default value should be considered for resolution in the presence of an ancestor value.
+ * can use true with overwrite to mean don't inherit, or true with merge to mean local default merged on top of inherited
+ * (but be careful here, if local default is null in a merge it will delete ancestor values).
+ * <p>
+ * in most cases this is false, meaning a default value is ignored if the parent has a value.
+ * <p>
+ * null should not be used. a boxed object is taken (as opposed to a primitive boolean) only in order to support migration.
+ */
+ @Nonnull
+ protected final Boolean localDefaultResolvesWithAncestorValue;
- protected BasicConfigInheritance(boolean isReinherited, @Nullable String conflictResolutionStrategy, boolean useLocalDefaultValue) {
+ /** whether a default set in an ancestor container's key definition will be considered as the
+ * local default value at descendants who don't define any other value (nothing set locally and local default is null);
+ * <p>
+ * if true (now the usual behaviour), if an ancestor defines a default and a descendant doesn't, the ancestor's value will be taken as a default.
+ * if it is also the case that localDefaultResolvesWithAncestorValue is true at the <i>ancestor</i> then a descendant who
+ * defines a local default value (with this field true) will have its conflict resolution strategy
+ * applied with the ancestor's default value.
+ * <p>
+ * if this is false, ancestor defaults are completely ignored; prior to 0.10.0 this was the normal behaviour,
+ * but it caused surprises where default values in parameters did not take effect.
+ * <p>
+ * null should not be used. a boxed object is taken (as opposed to a primitive boolean) only in order to support migration.
+ */
+ @Nonnull
+ protected final Boolean ancestorDefaultInheritable;
--- End diff --
Is this really needed? Is this condition actually an independent dimension of the behaviour of the class (i.e. it makes sense to specify it as an independently settable parameter), or is it really an "emergent" behaviour that can be inferred from other requirements?
Looking at how it's actually used in the strategies above, it turns out that it is always the logical inverse of `localDefaultResolvesWithAncestorValue`. How about we use Occam's Razor and do without this parameter, but instead replace its use with `!getLocalDefaultResolvesWithAncestorValue()`, and then tidy up the logic?
It's a small change to the code, but it does avoid introducing the overhead of another concept for developers to have to consider (config inheritance is complicated enough already!). The only changes that would be needed, aside from trivial simplifications to `readResolve` and `equals`, would be a couple of lines in `resolveWithParent`, where line 268 becomes slightly simpler with the loss of one test, and at line 283 you just replace the condition.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100846590
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void addParameters(AbstractBrooklynObjectSpec<?, ?> spec, List<? e
spec.parametersAdd(BasicSpecParameter.fromSpec(loader.getManagementContext(), spec));
}
if (explicitParams.size() > 0) {
- spec.parametersAdd(explicitParams);
+ spec.parametersReplace(resolveParameters(explicitParams, spec.getParameters(), spec));
+ }
+ }
+
+ /** merge parameters against other parameters and known and type-inherited config keys */
+ static Collection<SpecParameter<?>> resolveParameters(Collection<? extends SpecParameter<?>> newParams, Collection<? extends SpecParameter<?>> existingReferenceParamsToKeep, AbstractBrooklynObjectSpec<?,?> spec) {
+ Map<String,SpecParameter<?>> existingToKeep = MutableMap.of();
+ if (existingReferenceParamsToKeep!=null) {
+ for (SpecParameter<?> p: existingReferenceParamsToKeep) {
+ existingToKeep.put(p.getConfigKey().getName(), p);
+ }
+ }
+
+ List<SpecParameter<?>> result = MutableList.<SpecParameter<?>>of();
+ for (SpecParameter<?> p: newParams) {
+ if (p instanceof SpecParameterForInheritance) {
+ SpecParameter<?> existingP = existingToKeep.remove(p.getConfigKey().getName());
+ if (existingP!=null) {
+ p = ((SpecParameterForInheritance<?>)p).resolveWithAncestor(existingP);
+ } else {
+ // TODO find config keys on the type (not available as parameters)
+ /* we don't currently do this due to low priority; all it means if there is a config key in java,
+ * and a user wishes to expose it as a parameter, they have to redeclare everything;
+ * nothing from the config key in java will be inherited */
+ p = ((SpecParameterForInheritance<?>)p).resolveWithAncestor((ConfigKey<?>)null);
+ }
+ result.add(p);
+ } else {
+ existingToKeep.remove(p.getConfigKey().getName());
+ result.add(p);
+ }
+ }
+ result.addAll(existingToKeep.values());
+ return result;
+ }
+
+ /** instance of {@link SpecParameter} which includes information about what fields are explicitly set,
+ * for use with a subsequent merge */
+ @SuppressWarnings("serial")
+ @Beta
+ static class SpecParameterForInheritance<T> extends BasicSpecParameter<T> {
+
+ private final boolean hasType, hasLabelSet, hasPinnedSet, hasDefaultValue, hasConstraints, hasRuntimeInheritance, hasTypeInheritance;
+
+ private <SensorType> SpecParameterForInheritance(String label, Boolean pinned, ConfigKey<T> config, AttributeSensor<SensorType> sensor,
+ boolean hasType, boolean hasDefaultValue, boolean hasConstraints, boolean hasRuntimeInheritance, boolean hasTypeInheritance) {
+ super(Preconditions.checkNotNull(label!=null ? label : config.getName(), "label or config name must be set"),
+ pinned==null ? true : pinned, config, sensor);
+ this.hasType = hasType;
+ this.hasLabelSet = label!=null;
+ this.hasPinnedSet = pinned!=null;
+ this.hasDefaultValue = hasDefaultValue;
+ this.hasConstraints = hasConstraints;
+ this.hasRuntimeInheritance = hasRuntimeInheritance;
+ this.hasTypeInheritance = hasTypeInheritance;
+ }
+
+ /** used if yaml specifies a parameter, but possibly incomplete, and a spec supertype has a parameter */
+ SpecParameter<?> resolveWithAncestor(SpecParameter<?> ancestor) {
+ if (ancestor==null) return new BasicSpecParameter<>(getLabel(), isPinned(), getConfigKey(), getSensor());
+
+ return new BasicSpecParameter<>(
--- End diff --
yes, should always be top-down, and the `SpecParameterForInheritance` items replaced immediately. i'll have another look.
---
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 #462: Inherit config default values
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/462#discussion_r94787693
--- Diff: camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/SpecParameterUnwrappingTest.java ---
@@ -309,7 +298,7 @@ public void testCatalogConfigOverridesParameters() {
CatalogItem<?, ?> item = catalog.getCatalogItem(SYMBOLIC_NAME, TEST_VERSION);
AbstractBrooklynObjectSpec<?,?> spec = catalog.peekSpec(item);
List<SpecParameter<?>> params = spec.getParameters();
- assertEquals(params.size(), 3);
+ assertEquals(params.size(), NUM_ENTITY_DEFAULT_CONFIG_KEYS + 2);
--- End diff --
Hmm now that we are merging parameters with config keys I'd expect this should behave the same as `testRootParametersUnwrapped` (again unrelated to the PR).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #462: Inherit config default values
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/462#discussion_r94780176
--- Diff: camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java ---
@@ -403,8 +408,10 @@ protected ConfigInheritance getDefaultConfigInheritance() {
// name "B" the "val2" then applies to both!
//
// I plan to propose a change on dev@brooklyn, to replace `@SetFromFlag`!
-
- Set<FlagConfigKeyAndValueRecord> allKeys = MutableSet.of();
+
+ // need to de-dupe? (can't use Set bc FCKVR doesn't impl equals/hashcode)
--- End diff --
It's `FCKAVR`.
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100810442
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java ---
@@ -28,51 +32,188 @@
import org.apache.brooklyn.config.ConfigKey;
import org.apache.brooklyn.config.ConfigValueAtContainer;
import org.apache.brooklyn.util.collections.CollectionMerger;
+import org.apache.brooklyn.util.exceptions.Exceptions;
import org.apache.brooklyn.util.exceptions.ReferenceWithError;
import org.apache.brooklyn.util.guava.Maybe;
+import org.apache.brooklyn.util.text.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+@SuppressWarnings("serial")
public class BasicConfigInheritance implements ConfigInheritance {
+ private static final Logger log = LoggerFactory.getLogger(BasicConfigInheritance.class);
+
private static final long serialVersionUID = -5916548049057961051L;
public static final String CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE = "deep_merge";
public static final String CONFLICT_RESOLUTION_STRATEGY_OVERWRITE = "overwrite";
+ public static abstract class DelegatingConfigInheritance implements ConfigInheritance {
+ protected abstract ConfigInheritance getDelegate();
+
+ @Override @Deprecated public InheritanceMode isInherited(ConfigKey<?> key, Object from, Object to) {
+ return getDelegate().isInherited(key, from, to);
+ }
+ @Override public <TContainer, TValue> boolean isReinheritable(ConfigValueAtContainer<TContainer, TValue> parent, ConfigInheritanceContext context) {
+ return getDelegate().isReinheritable(parent, context);
+ }
+ @Override public <TContainer, TValue> boolean considerParent(ConfigValueAtContainer<TContainer, TValue> local, ConfigValueAtContainer<TContainer, TValue> parent, ConfigInheritanceContext context) {
+ return getDelegate().considerParent(local, parent, context);
+ }
+ @Override
+ public <TContainer, TValue> ReferenceWithError<ConfigValueAtContainer<TContainer, TValue>> resolveWithParent(ConfigValueAtContainer<TContainer, TValue> local, ConfigValueAtContainer<TContainer, TValue> resolvedParent, ConfigInheritanceContext context) {
+ return getDelegate().resolveWithParent(local, resolvedParent, context);
+ }
+ @Override
+ public boolean equals(Object obj) {
+ return super.equals(obj) || getDelegate().equals(obj);
+ }
+
+ // standard deserialization method
+ protected ConfigInheritance readResolve() {
+ return returnEquivalentConstant(this);
+ }
+ }
+
+ private static ConfigInheritance returnEquivalentConstant(ConfigInheritance candidate) {
+ for (ConfigInheritance knownMode: Arrays.asList(
+ NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, NEVER_INHERITED, OVERWRITE, BasicConfigInheritance.DEEP_MERGE)) {
+ if (candidate.equals(knownMode)) return knownMode;
+ }
+ if (candidate.equals(new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, true))) {
+ // ignore the ancestor flag for this mode
+ return NEVER_INHERITED;
+ }
+ return candidate;
+ }
+
+ /*
+ * use of delegate is so that stateless classes can be defined to make the serialization nice,
+ * both the name and hiding the implementation detail (also making it easier for that detail to change);
+ * with aliased type names the field names here could even be aliases for the type names.
+ * (we could alternatively have an "alias-for-final-instance" mode in serialization,
+ * to optimize where we know that a java instance is final.)
+ */
+
+ private static class NotReinherited extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** Indicates that a config key value should not be passed down from a container where it is defined.
* Unlike {@link #NEVER_INHERITED} these values can be passed down if set as anonymous keys at a container
* (ie the container does not expect it) to a container which does expect it, but it will not be passed down further.
* If the inheritor also defines a value the parent's value is ignored irrespective
* (as in {@link #OVERWRITE}; see {@link #NOT_REINHERITED_ELSE_DEEP_MERGE} if merging is desired). */
- public static BasicConfigInheritance NOT_REINHERITED = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_OVERWRITE,false);
+ public static ConfigInheritance NOT_REINHERITED = new NotReinherited();
+
+ private static class NotReinheritedElseDeepMerge extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** As {@link #NOT_REINHERITED} but in cases where a value is inherited because a parent did not recognize it,
* if the inheritor also defines a value the two values should be merged. */
- public static BasicConfigInheritance NOT_REINHERITED_ELSE_DEEP_MERGE = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE,false);
- /** Indicates that a key's value should never be inherited, even if defined on a container that does not know the key.
- * Most usages will prefer {@link #NOT_REINHERITED}. */
- public static BasicConfigInheritance NEVER_INHERITED = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_OVERWRITE,true);
+ public static ConfigInheritance NOT_REINHERITED_ELSE_DEEP_MERGE = new NotReinheritedElseDeepMerge();
--- End diff --
egads yes absolutely should be `static final` !
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/462
Thanks for such great comments. Nearly done, should finish today. Sorry I dropped the ball on 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 #462: Inherit config default values
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/462#discussion_r101306815
--- Diff: utils/common/src/main/java/org/apache/brooklyn/util/guava/Maybe.java ---
@@ -457,6 +457,8 @@ public boolean equals(Object obj) {
Maybe<?> other = (Maybe<?>)obj;
if (!isPresent())
return !other.isPresent();
+ if (!other.isPresent())
+ return false;
--- End diff --
Don't mind me, somehow missed the previous check where two absents will be equal.
My impression was exactly the opposite, that with the change two absents are never equal, even for `this.equals(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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100806157
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java ---
@@ -28,51 +32,188 @@
import org.apache.brooklyn.config.ConfigKey;
import org.apache.brooklyn.config.ConfigValueAtContainer;
import org.apache.brooklyn.util.collections.CollectionMerger;
+import org.apache.brooklyn.util.exceptions.Exceptions;
import org.apache.brooklyn.util.exceptions.ReferenceWithError;
import org.apache.brooklyn.util.guava.Maybe;
+import org.apache.brooklyn.util.text.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+@SuppressWarnings("serial")
public class BasicConfigInheritance implements ConfigInheritance {
+ private static final Logger log = LoggerFactory.getLogger(BasicConfigInheritance.class);
+
private static final long serialVersionUID = -5916548049057961051L;
public static final String CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE = "deep_merge";
public static final String CONFLICT_RESOLUTION_STRATEGY_OVERWRITE = "overwrite";
+ public static abstract class DelegatingConfigInheritance implements ConfigInheritance {
+ protected abstract ConfigInheritance getDelegate();
+
+ @Override @Deprecated public InheritanceMode isInherited(ConfigKey<?> key, Object from, Object to) {
+ return getDelegate().isInherited(key, from, to);
+ }
+ @Override public <TContainer, TValue> boolean isReinheritable(ConfigValueAtContainer<TContainer, TValue> parent, ConfigInheritanceContext context) {
+ return getDelegate().isReinheritable(parent, context);
+ }
+ @Override public <TContainer, TValue> boolean considerParent(ConfigValueAtContainer<TContainer, TValue> local, ConfigValueAtContainer<TContainer, TValue> parent, ConfigInheritanceContext context) {
+ return getDelegate().considerParent(local, parent, context);
+ }
+ @Override
+ public <TContainer, TValue> ReferenceWithError<ConfigValueAtContainer<TContainer, TValue>> resolveWithParent(ConfigValueAtContainer<TContainer, TValue> local, ConfigValueAtContainer<TContainer, TValue> resolvedParent, ConfigInheritanceContext context) {
+ return getDelegate().resolveWithParent(local, resolvedParent, context);
+ }
+ @Override
+ public boolean equals(Object obj) {
+ return super.equals(obj) || getDelegate().equals(obj);
+ }
+
+ // standard deserialization method
+ protected ConfigInheritance readResolve() {
+ return returnEquivalentConstant(this);
+ }
+ }
+
+ private static ConfigInheritance returnEquivalentConstant(ConfigInheritance candidate) {
+ for (ConfigInheritance knownMode: Arrays.asList(
+ NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, NEVER_INHERITED, OVERWRITE, BasicConfigInheritance.DEEP_MERGE)) {
+ if (candidate.equals(knownMode)) return knownMode;
+ }
+ if (candidate.equals(new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, true))) {
+ // ignore the ancestor flag for this mode
+ return NEVER_INHERITED;
+ }
+ return candidate;
+ }
+
+ /*
+ * use of delegate is so that stateless classes can be defined to make the serialization nice,
+ * both the name and hiding the implementation detail (also making it easier for that detail to change);
+ * with aliased type names the field names here could even be aliases for the type names.
+ * (we could alternatively have an "alias-for-final-instance" mode in serialization,
+ * to optimize where we know that a java instance is final.)
+ */
+
+ private static class NotReinherited extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** Indicates that a config key value should not be passed down from a container where it is defined.
* Unlike {@link #NEVER_INHERITED} these values can be passed down if set as anonymous keys at a container
* (ie the container does not expect it) to a container which does expect it, but it will not be passed down further.
* If the inheritor also defines a value the parent's value is ignored irrespective
* (as in {@link #OVERWRITE}; see {@link #NOT_REINHERITED_ELSE_DEEP_MERGE} if merging is desired). */
- public static BasicConfigInheritance NOT_REINHERITED = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_OVERWRITE,false);
+ public static ConfigInheritance NOT_REINHERITED = new NotReinherited();
--- End diff --
@neykov my idea was for the class to such that if serialized it just writes the class name; won't your variant write all the fields passed to `BCI` ?
---
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 #462: Inherit config default values
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r89771711
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java ---
@@ -170,12 +306,88 @@ public String getConflictResolutionStrategy() {
return conflictResolutionStrategy;
}
+ @Deprecated /** @deprecated since 0.10.0 when it was introduced, prefer {@link #getLocalDefaultResolvesWithAncestorValue()} */
public boolean getUseLocalDefaultValue() {
- return useLocalDefaultValue;
+ return getLocalDefaultResolvesWithAncestorValue();
+ }
+
+ /** see {@link #localDefaultResolvesWithAncestorValue} */
+ public boolean getLocalDefaultResolvesWithAncestorValue() {
+ if (localDefaultResolvesWithAncestorValue==null) {
+ // in case some legacy path is using an improperly deserialized object
+ log.warn("Encountered legacy "+this+" with null localDefaultResolvesWithAncestorValue; transforming", new Throwable("stack trace for legacy "+this));
+ readResolve();
+ }
+ return localDefaultResolvesWithAncestorValue;
+ }
+
+ public boolean getAncestorDefaultInheritable() {
+ if (ancestorDefaultInheritable==null) {
+ log.warn("Encountered legacy "+this+" with null ancestorDefaultInheritable; transforming", new Throwable("stack trace for legacy "+this));
+ readResolve();
+ }
+ return ancestorDefaultInheritable;
}
+ // standard deserialization method
+ private ConfigInheritance readResolve() {
+ try {
+ if (useLocalDefaultValue!=null) {
+ // move away from useLocalDefaultValue to localDefaultResolvesWithAncestorValue
+
+ Field fNew = getClass().getDeclaredField("localDefaultResolvesWithAncestorValue");
+ fNew.setAccessible(true);
+ Field fOld = getClass().getDeclaredField("useLocalDefaultValue");
+ fOld.setAccessible(true);
+
+ if (fNew.get(this)==null) {
+ fNew.set(this, useLocalDefaultValue);
+ } else {
+ if (!fNew.get(this).equals(useLocalDefaultValue)) {
+ throw new IllegalStateException("Incompatible values detected for "+fOld+" ("+fOld.get(this)+") and "+fNew+" ("+fNew.get(this)+")");
+ }
+ }
+ fOld.set(this, null);
+ }
+
+ if (ancestorDefaultInheritable==null) {
+ Field f = getClass().getDeclaredField("ancestorDefaultInheritable");
+ f.setAccessible(true);
+ f.set(this, true);
+ }
+
+ } catch (Exception e) {
+ throw Exceptions.propagate(e);
+ }
+
+ for (ConfigInheritance knownMode: Arrays.asList(
+ NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, NEVER_INHERITED, OVERWRITE, DEEP_MERGE)) {
+ if (equals(knownMode)) return knownMode;
--- End diff --
I don't understand how this works. I added a simple test to `XmlMementoSerializerTest`, which fails:
```
@Test
public void testConfigInheritanceVals() throws Exception {
ConfigInheritance val = BasicConfigInheritance.NEVER_INHERITED;
ConfigInheritance newVal = assertSerializeAndDeserialize(val); // TODO this line fails
assertSame(val, newVal);
}
```
It fails because the deserialized object has `null` for its `delegate` field. It feels to me like the `equals(knownMode)` will always return false because the deserialized object's `delegate` field will never have been set.
---
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 #462: Inherit config default values
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/462#discussion_r94927466
--- Diff: core/src/main/java/org/apache/brooklyn/core/mgmt/EntityManagementUtils.java ---
@@ -255,8 +255,9 @@ private static void mergeWrapperParentSpecToChildEntity(EntitySpec<? extends App
wrappedChild.locationSpecs(wrapperParent.getLocationSpecs());
wrappedChild.locations(wrapperParent.getLocations());
- if (!wrapperParent.getParameters().isEmpty())
- wrappedChild.parametersReplace(wrapperParent.getParameters());
+ if (!wrapperParent.getParameters().isEmpty()) {
+ wrappedChild.parametersAdd(wrapperParent.getParameters());
--- End diff --
Remove [TODO](https://github.com/apache/brooklyn-server/blob/master/core/src/main/java/org/apache/brooklyn/core/mgmt/EntityManagementUtils.java#L320). Or is it still TBD?
---
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 #462: Inherit config default values
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/462#discussion_r94975515
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java ---
@@ -28,51 +32,188 @@
import org.apache.brooklyn.config.ConfigKey;
import org.apache.brooklyn.config.ConfigValueAtContainer;
import org.apache.brooklyn.util.collections.CollectionMerger;
+import org.apache.brooklyn.util.exceptions.Exceptions;
import org.apache.brooklyn.util.exceptions.ReferenceWithError;
import org.apache.brooklyn.util.guava.Maybe;
+import org.apache.brooklyn.util.text.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+@SuppressWarnings("serial")
public class BasicConfigInheritance implements ConfigInheritance {
+ private static final Logger log = LoggerFactory.getLogger(BasicConfigInheritance.class);
+
private static final long serialVersionUID = -5916548049057961051L;
public static final String CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE = "deep_merge";
public static final String CONFLICT_RESOLUTION_STRATEGY_OVERWRITE = "overwrite";
+ public static abstract class DelegatingConfigInheritance implements ConfigInheritance {
+ protected abstract ConfigInheritance getDelegate();
+
+ @Override @Deprecated public InheritanceMode isInherited(ConfigKey<?> key, Object from, Object to) {
+ return getDelegate().isInherited(key, from, to);
+ }
+ @Override public <TContainer, TValue> boolean isReinheritable(ConfigValueAtContainer<TContainer, TValue> parent, ConfigInheritanceContext context) {
+ return getDelegate().isReinheritable(parent, context);
+ }
+ @Override public <TContainer, TValue> boolean considerParent(ConfigValueAtContainer<TContainer, TValue> local, ConfigValueAtContainer<TContainer, TValue> parent, ConfigInheritanceContext context) {
+ return getDelegate().considerParent(local, parent, context);
+ }
+ @Override
+ public <TContainer, TValue> ReferenceWithError<ConfigValueAtContainer<TContainer, TValue>> resolveWithParent(ConfigValueAtContainer<TContainer, TValue> local, ConfigValueAtContainer<TContainer, TValue> resolvedParent, ConfigInheritanceContext context) {
+ return getDelegate().resolveWithParent(local, resolvedParent, context);
+ }
+ @Override
+ public boolean equals(Object obj) {
+ return super.equals(obj) || getDelegate().equals(obj);
+ }
+
+ // standard deserialization method
+ protected ConfigInheritance readResolve() {
+ return returnEquivalentConstant(this);
+ }
+ }
+
+ private static ConfigInheritance returnEquivalentConstant(ConfigInheritance candidate) {
+ for (ConfigInheritance knownMode: Arrays.asList(
+ NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, NEVER_INHERITED, OVERWRITE, BasicConfigInheritance.DEEP_MERGE)) {
+ if (candidate.equals(knownMode)) return knownMode;
+ }
+ if (candidate.equals(new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, true))) {
--- End diff --
Could make this a constant.
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100789705
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void addParameters(AbstractBrooklynObjectSpec<?, ?> spec, List<? e
spec.parametersAdd(BasicSpecParameter.fromSpec(loader.getManagementContext(), spec));
}
if (explicitParams.size() > 0) {
- spec.parametersAdd(explicitParams);
+ spec.parametersReplace(resolveParameters(explicitParams, spec.getParameters(), spec));
+ }
+ }
+
+ /** merge parameters against other parameters and known and type-inherited config keys */
+ static Collection<SpecParameter<?>> resolveParameters(Collection<? extends SpecParameter<?>> newParams, Collection<? extends SpecParameter<?>> existingReferenceParamsToKeep, AbstractBrooklynObjectSpec<?,?> spec) {
+ Map<String,SpecParameter<?>> existingToKeep = MutableMap.of();
+ if (existingReferenceParamsToKeep!=null) {
+ for (SpecParameter<?> p: existingReferenceParamsToKeep) {
+ existingToKeep.put(p.getConfigKey().getName(), p);
+ }
+ }
+
+ List<SpecParameter<?>> result = MutableList.<SpecParameter<?>>of();
+ for (SpecParameter<?> p: newParams) {
+ if (p instanceof SpecParameterForInheritance) {
--- End diff --
that would work but i put the extra branch in to show the logic that should be applied; have used your cleanup to extract `existingP` just once, and expanded the placeholder code so it's clearer
---
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 #462: Inherit config default values
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r90756006
--- Diff: core/src/main/java/org/apache/brooklyn/util/core/xstream/ClassRenamingMapper.java ---
@@ -42,7 +42,7 @@ public ClassRenamingMapper(Mapper wrapped, Map<String, String> nameToType) {
@Override
public Class<?> realClass(String elementName) {
- Optional<String> elementNameOpt = Reflections.tryFindMappedName(nameToType, elementName);
+ Maybe<String> elementNameOpt = Reflections.findMappedNameMaybe(nameToType, elementName);
--- End diff --
My question about renaming wasn't so much about why change to return a `Maybe`. It's why change the method name? I prefer `tryFindMappedName` because it follows the guava naming convention. We use that naming convention in quite a few other places in Brooklyn as well.
I see now why - the previous `tryFindMappedName` is deprecated in favour of `findMappedNameMaybe` so that it can have a different return type. That's really annoying (that we're creating a different method naming convention so as to change the return type). But I'm not sure how better to tackle that while still preserving backwards compatibility!
As for null, you've changed the contract. Maybe it's fine that the super throws the `NullPointerException` rather than us checking so not a big deal. But on balance, I'd prefer to report a null present value here, rather than calling `super.realClass(null)`, which could get into completely different code before an NPE is thrown. No strong feelings though.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100936874
--- Diff: camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/SpecParameterUnwrappingTest.java ---
@@ -146,8 +151,9 @@ public void testRootParametersUnwrapped() {
CatalogItem<?, ?> item = catalog.getCatalogItem(SYMBOLIC_NAME, TEST_VERSION);
AbstractBrooklynObjectSpec<?,?> spec = catalog.peekSpec(item);
List<SpecParameter<?>> params = spec.getParameters();
- assertEquals(params.size(), NUM_APP_DEFAULT_CONFIG_KEYS + 1, "params="+params);
assertTrue(Iterables.tryFind(params, nameEqualTo("simple")).isPresent());
+ assertTrue(Iterables.tryFind(params, nameEqualTo(SHARED_CONFIG.getName())).isPresent());
+ assertEquals(params.size(), NUM_APP_DEFAULT_CONFIG_KEYS + 2, "params="+params);
--- End diff --
nice idea to use constants for all the counts. have also added comment about unwrapping, agree it's a bit wasteful, but doesn't seem to do any harm.
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/462
Good spot. Will fix and merge.
---
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 #462: Inherit config default values
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100526488
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java ---
@@ -28,51 +32,188 @@
import org.apache.brooklyn.config.ConfigKey;
import org.apache.brooklyn.config.ConfigValueAtContainer;
import org.apache.brooklyn.util.collections.CollectionMerger;
+import org.apache.brooklyn.util.exceptions.Exceptions;
import org.apache.brooklyn.util.exceptions.ReferenceWithError;
import org.apache.brooklyn.util.guava.Maybe;
+import org.apache.brooklyn.util.text.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+@SuppressWarnings("serial")
public class BasicConfigInheritance implements ConfigInheritance {
+ private static final Logger log = LoggerFactory.getLogger(BasicConfigInheritance.class);
+
private static final long serialVersionUID = -5916548049057961051L;
public static final String CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE = "deep_merge";
public static final String CONFLICT_RESOLUTION_STRATEGY_OVERWRITE = "overwrite";
+ public static abstract class DelegatingConfigInheritance implements ConfigInheritance {
+ protected abstract ConfigInheritance getDelegate();
+
+ @Override @Deprecated public InheritanceMode isInherited(ConfigKey<?> key, Object from, Object to) {
+ return getDelegate().isInherited(key, from, to);
+ }
+ @Override public <TContainer, TValue> boolean isReinheritable(ConfigValueAtContainer<TContainer, TValue> parent, ConfigInheritanceContext context) {
+ return getDelegate().isReinheritable(parent, context);
+ }
+ @Override public <TContainer, TValue> boolean considerParent(ConfigValueAtContainer<TContainer, TValue> local, ConfigValueAtContainer<TContainer, TValue> parent, ConfigInheritanceContext context) {
+ return getDelegate().considerParent(local, parent, context);
+ }
+ @Override
+ public <TContainer, TValue> ReferenceWithError<ConfigValueAtContainer<TContainer, TValue>> resolveWithParent(ConfigValueAtContainer<TContainer, TValue> local, ConfigValueAtContainer<TContainer, TValue> resolvedParent, ConfigInheritanceContext context) {
+ return getDelegate().resolveWithParent(local, resolvedParent, context);
+ }
+ @Override
+ public boolean equals(Object obj) {
+ return super.equals(obj) || getDelegate().equals(obj);
+ }
+
+ // standard deserialization method
+ protected ConfigInheritance readResolve() {
+ return returnEquivalentConstant(this);
+ }
+ }
+
+ private static ConfigInheritance returnEquivalentConstant(ConfigInheritance candidate) {
+ for (ConfigInheritance knownMode: Arrays.asList(
+ NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, NEVER_INHERITED, OVERWRITE, BasicConfigInheritance.DEEP_MERGE)) {
+ if (candidate.equals(knownMode)) return knownMode;
+ }
+ if (candidate.equals(new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, true))) {
+ // ignore the ancestor flag for this mode
+ return NEVER_INHERITED;
+ }
+ return candidate;
+ }
+
+ /*
+ * use of delegate is so that stateless classes can be defined to make the serialization nice,
+ * both the name and hiding the implementation detail (also making it easier for that detail to change);
+ * with aliased type names the field names here could even be aliases for the type names.
+ * (we could alternatively have an "alias-for-final-instance" mode in serialization,
+ * to optimize where we know that a java instance is final.)
+ */
+
+ private static class NotReinherited extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** Indicates that a config key value should not be passed down from a container where it is defined.
* Unlike {@link #NEVER_INHERITED} these values can be passed down if set as anonymous keys at a container
* (ie the container does not expect it) to a container which does expect it, but it will not be passed down further.
* If the inheritor also defines a value the parent's value is ignored irrespective
* (as in {@link #OVERWRITE}; see {@link #NOT_REINHERITED_ELSE_DEEP_MERGE} if merging is desired). */
- public static BasicConfigInheritance NOT_REINHERITED = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_OVERWRITE,false);
+ public static ConfigInheritance NOT_REINHERITED = new NotReinherited();
+
+ private static class NotReinheritedElseDeepMerge extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** As {@link #NOT_REINHERITED} but in cases where a value is inherited because a parent did not recognize it,
* if the inheritor also defines a value the two values should be merged. */
- public static BasicConfigInheritance NOT_REINHERITED_ELSE_DEEP_MERGE = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE,false);
- /** Indicates that a key's value should never be inherited, even if defined on a container that does not know the key.
- * Most usages will prefer {@link #NOT_REINHERITED}. */
- public static BasicConfigInheritance NEVER_INHERITED = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_OVERWRITE,true);
+ public static ConfigInheritance NOT_REINHERITED_ELSE_DEEP_MERGE = new NotReinheritedElseDeepMerge();
+
+ private static class NeverInherited extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, false);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
+ /** Indicates that a key's value should never be inherited, even if inherited from a value set on a container that does not know the key.
+ * (Most usages will prefer {@link #NOT_REINHERITED}.) */
+ public static ConfigInheritance NEVER_INHERITED = new NeverInherited();
+
+ private static class Overwrite extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(true, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** Indicates that if a key has a value at both an ancestor and a descendant, the descendant and his descendants
* will prefer the value at the descendant. */
- public static BasicConfigInheritance OVERWRITE = new BasicConfigInheritance(true,CONFLICT_RESOLUTION_STRATEGY_OVERWRITE,false);
+ public static ConfigInheritance OVERWRITE = new Overwrite();
+
+ private static class DeepMerge extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(true, CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** Indicates that if a key has a value at both an ancestor and a descendant, the descendant and his descendants
- * should attempt to merge the values. If the values are not mergable behaviour is undefined
+ * should attempt to merge the values. If the values are not mergeable behaviour is undefined
--- End diff --
+1 agree nothing needs done in this PR
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #462: Inherit config default values
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/462#discussion_r94952205
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -227,13 +234,30 @@ public String toString() {
String label = (String)inputDef.get("label");
String description = (String)inputDef.get("description");
String type = (String)inputDef.get("type");
- Object defaultValue = inputDef.get("default");
Boolean pinned = (Boolean)inputDef.get("pinned");
+ boolean hasDefaultValue = inputDef.containsKey("default");
+ Object defaultValue = inputDef.get("default");
if (specialFlagTransformer != null) {
defaultValue = specialFlagTransformer.apply(defaultValue);
}
+ boolean hasConstraints = inputDef.containsKey("constraints");
Predicate<?> constraint = parseConstraints(inputDef.get("constraints"), loader);
- ConfigInheritance parentInheritance = parseInheritance(inputDef.get("inheritance.parent"), loader);
+
+ ConfigInheritance runtimeInheritance;
+ boolean hasRuntimeInheritance;
+ if (inputDef.containsKey("inheritance.runtime")) {
+ hasRuntimeInheritance = true;
+ runtimeInheritance = parseInheritance(inputDef.get("inheritance.runtime"), loader);
+ } else if (inputDef.containsKey("inheritance.parent")) {
+ // this alias will be deprecated
--- End diff --
+1
Log a warning with a deprecation message.
---
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 #462: Inherit config default values
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r94396058
--- Diff: core/src/test/java/org/apache/brooklyn/core/mgmt/persist/XmlMementoSerializerTest.java ---
@@ -792,4 +794,13 @@ private Bundle installBundle(ManagementContext mgmt, String bundleUrl) throws Ex
Framework framework = osgiManager.getFramework();
return Osgis.install(framework, bundleUrl);
}
+
+ @Test
+ public void testConfigInheritanceVals() throws Exception {
+ ConfigInheritance val = BasicConfigInheritance.NEVER_INHERITED;
+
+ ConfigInheritance newVal = assertSerializeAndDeserialize(val); // TODO this line fails
--- End diff --
the comment can be removed now
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #462: Inherit config default values
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on the issue:
https://github.com/apache/brooklyn-server/pull/462
Wrote a few extra tests to try out the change. The only one that didn't perform as I'd expected was
```java
@Test
public void testConfigDefaultIsNotInheritedWith_LocalDefaultResolvesWithAncestorValue_SetToTrue() throws Exception {
addCatalogItems(
"brooklyn.catalog:",
" itemType: entity",
" items:",
" - id: entity-with-keys",
" item:",
" type: "+TestEntity.class.getName(),
" brooklyn.parameters:",
" - name: my.param.key",
" type: string",
" inheritance.parent: never",
" description: description one",
" default: myDefaultVal",
" brooklyn.config:",
" my.other.key: $brooklyn:config(\"my.param.key\")");
addCatalogItems(
"brooklyn.catalog:",
" itemType: entity",
" items:",
" - id: wrapper-entity",
" item:",
" brooklyn.parameters:",
" - name: my.param.key",
" description: description two",
" type: entity-with-keys");
String yaml = Joiner.on("\n").join(
"services:",
"- type: wrapper-entity");
Entity app = createStartWaitAndLogApplication(yaml);
final TestEntity entity = (TestEntity) Iterables.getOnlyElement(app.getChildren());
LOG.info("Config keys declared on "+entity+": "+entity.config().findKeysDeclared(Predicates.alwaysTrue()));
ConfigKey<?> key = Iterables.getOnlyElement( entity.config().findKeysDeclared(ConfigPredicates.nameEqualTo("my.param.key")) );
assertEquals(key.getDescription(), "description two");
assertEquals(entity.config().get(key), null);
}
```
noting that the last assertion above expects the value to be null because the "inheritance.parent"
has been set to "never". Actually the failure is
```
java.lang.AssertionError: expected [null] but found [myDefaultVal]
Expected :null
Actual :myDefaultVal
<Click to see difference>
```
I debugged to see what goes on here, and found that the BasicConfigInheritance class is
doing what I expected, and the default value is *not* inherited. Instead, the default
is set higher up in the stack, in AbstractConfigurationSupportInternal#getNonBlockingResolvingSimple,
at the [line](https://github.com/apache/brooklyn-server/blob/master/core/src/main/java/org/apache/brooklyn/core/objs/AbstractConfigurationSupportInternal.java#L139)
```
Object unresolved = getRaw(key).or(key.getDefaultValue());
```
so in this case the higher level code is ignoring and overriding the instructions of the
way the inheritance is configured on the key. Maybe now that the inheritance code is
written this line and any others like it should be changed, to avoid "second-guessing"
the specified inheritance behaviour.
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
GitHub user ahgittin reopened a pull request:
https://github.com/apache/brooklyn-server/pull/462
Inherit config default values
resolve https://issues.apache.org/jira/browse/BROOKLYN-267
also during deserializing migrate to non-deprecated classes
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ahgittin/brooklyn-server inherit-config-default-values
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/462.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 #462
----
commit 9fb6f35e8b2c694168c06a39b7fcc922b0ece8fc
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-11-24T10:56:46Z
migrate config inheritance to new classes
pioneers use of `readResolve` that actually works brilliantly out of the box due to xstream
also tidying `BasicConfigInheritance` and adding a placeholder (not used yet)
for resolving ancestor defaults
includes tests for config inheritance serialization
commit 1d5cafca0195275758518e7bba3541ff29298c89
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-11-24T16:29:21Z
document better config inheritance semantics
commit 79bb11957be95e525f126d9c7c301635a9fa6404
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-11-17T15:12:09Z
implement inheritance of config default values
test and code changes to respect the additional inheritance argument added in the previous request:
resolves https://issues.apache.org/jira/browse/BROOKLYN-267
commit 2bb1346ee59d0b1c3bb8ec638f38af1cf5a62e9e
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2016-11-24T17:34:10Z
uncomment another assertion about inherited config
----
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/462
@neykov @geomacy @aledsage Done. Hurrah. I've fixed the "declared keys" exclusions -- in the end @geomacy's suggestion re cleaning up the `resolveParameters` method was on point, and the missing case @neykov asked about was guaranteed not to occur, so there are a few other cleanups also. Apologies for the complexity of this all (I wish we could move to YOML and do away with `Entity` subclasses!). The main new PRs to review are the last two. The others are mostly dull fixes.
---
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 #462: Inherit config default values
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100509157
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java ---
@@ -28,51 +32,188 @@
import org.apache.brooklyn.config.ConfigKey;
import org.apache.brooklyn.config.ConfigValueAtContainer;
import org.apache.brooklyn.util.collections.CollectionMerger;
+import org.apache.brooklyn.util.exceptions.Exceptions;
import org.apache.brooklyn.util.exceptions.ReferenceWithError;
import org.apache.brooklyn.util.guava.Maybe;
+import org.apache.brooklyn.util.text.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+@SuppressWarnings("serial")
public class BasicConfigInheritance implements ConfigInheritance {
+ private static final Logger log = LoggerFactory.getLogger(BasicConfigInheritance.class);
+
private static final long serialVersionUID = -5916548049057961051L;
public static final String CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE = "deep_merge";
public static final String CONFLICT_RESOLUTION_STRATEGY_OVERWRITE = "overwrite";
+ public static abstract class DelegatingConfigInheritance implements ConfigInheritance {
+ protected abstract ConfigInheritance getDelegate();
+
+ @Override @Deprecated public InheritanceMode isInherited(ConfigKey<?> key, Object from, Object to) {
+ return getDelegate().isInherited(key, from, to);
+ }
+ @Override public <TContainer, TValue> boolean isReinheritable(ConfigValueAtContainer<TContainer, TValue> parent, ConfigInheritanceContext context) {
+ return getDelegate().isReinheritable(parent, context);
+ }
+ @Override public <TContainer, TValue> boolean considerParent(ConfigValueAtContainer<TContainer, TValue> local, ConfigValueAtContainer<TContainer, TValue> parent, ConfigInheritanceContext context) {
+ return getDelegate().considerParent(local, parent, context);
+ }
+ @Override
+ public <TContainer, TValue> ReferenceWithError<ConfigValueAtContainer<TContainer, TValue>> resolveWithParent(ConfigValueAtContainer<TContainer, TValue> local, ConfigValueAtContainer<TContainer, TValue> resolvedParent, ConfigInheritanceContext context) {
+ return getDelegate().resolveWithParent(local, resolvedParent, context);
+ }
+ @Override
+ public boolean equals(Object obj) {
+ return super.equals(obj) || getDelegate().equals(obj);
+ }
+
+ // standard deserialization method
+ protected ConfigInheritance readResolve() {
+ return returnEquivalentConstant(this);
+ }
+ }
+
+ private static ConfigInheritance returnEquivalentConstant(ConfigInheritance candidate) {
+ for (ConfigInheritance knownMode: Arrays.asList(
+ NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, NEVER_INHERITED, OVERWRITE, BasicConfigInheritance.DEEP_MERGE)) {
+ if (candidate.equals(knownMode)) return knownMode;
+ }
+ if (candidate.equals(new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, true))) {
+ // ignore the ancestor flag for this mode
+ return NEVER_INHERITED;
+ }
+ return candidate;
+ }
+
+ /*
+ * use of delegate is so that stateless classes can be defined to make the serialization nice,
+ * both the name and hiding the implementation detail (also making it easier for that detail to change);
+ * with aliased type names the field names here could even be aliases for the type names.
+ * (we could alternatively have an "alias-for-final-instance" mode in serialization,
+ * to optimize where we know that a java instance is final.)
+ */
+
+ private static class NotReinherited extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** Indicates that a config key value should not be passed down from a container where it is defined.
* Unlike {@link #NEVER_INHERITED} these values can be passed down if set as anonymous keys at a container
* (ie the container does not expect it) to a container which does expect it, but it will not be passed down further.
* If the inheritor also defines a value the parent's value is ignored irrespective
* (as in {@link #OVERWRITE}; see {@link #NOT_REINHERITED_ELSE_DEEP_MERGE} if merging is desired). */
- public static BasicConfigInheritance NOT_REINHERITED = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_OVERWRITE,false);
+ public static ConfigInheritance NOT_REINHERITED = new NotReinherited();
--- End diff --
Very interested in @neykov's suggested alternative here, for a "less mind-bending, delegate-free implementation". Not sure I fully understand the implications (especially relating to the comment above about "use of delegate is so that stateless classes can be defined to make the serialization nice..."). I've not spent long enough bending my mind during this review :-)
---
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 #462: Inherit config default values
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r93286450
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void addParameters(AbstractBrooklynObjectSpec<?, ?> spec, List<? e
spec.parametersAdd(BasicSpecParameter.fromSpec(loader.getManagementContext(), spec));
}
if (explicitParams.size() > 0) {
- spec.parametersAdd(explicitParams);
+ spec.parametersReplace(resolveParameters(explicitParams, spec.getParameters(), spec));
+ }
+ }
+
+ /** merge parameters against other parameters and known and type-inherited config keys */
+ static Collection<SpecParameter<?>> resolveParameters(Collection<? extends SpecParameter<?>> newParams, Collection<? extends SpecParameter<?>> existingReferenceParamsToKeep, AbstractBrooklynObjectSpec<?,?> spec) {
--- End diff --
The argument `spec` is never used in this method, should it be?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100810106
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java ---
@@ -170,12 +324,103 @@ public String getConflictResolutionStrategy() {
return conflictResolutionStrategy;
}
+ @Deprecated /** @deprecated since 0.10.0 when it was introduced, prefer {@link #getLocalDefaultResolvesWithAncestorValue()} */
public boolean getUseLocalDefaultValue() {
- return useLocalDefaultValue;
+ return getLocalDefaultResolvesWithAncestorValue();
+ }
+
+ /** see {@link #localDefaultResolvesWithAncestorValue} */
+ public boolean getLocalDefaultResolvesWithAncestorValue() {
+ if (localDefaultResolvesWithAncestorValue==null) {
+ // in case some legacy path is using an improperly deserialized object
+ log.warn("Encountered legacy "+this+" with null localDefaultResolvesWithAncestorValue; transforming", new Throwable("stack trace for legacy "+this));
+ readResolve();
+ }
+ return localDefaultResolvesWithAncestorValue;
+ }
+
+ public boolean getAncestorDefaultInheritable() {
+ if (ancestorDefaultInheritable==null) {
+ log.warn("Encountered legacy "+this+" with null ancestorDefaultInheritable; transforming", new Throwable("stack trace for legacy "+this));
+ readResolve();
+ }
+ return ancestorDefaultInheritable;
+ }
+
+ // standard deserialization method
+ private ConfigInheritance readResolve() {
+ try {
+ if (useLocalDefaultValue!=null) {
+ // move away from useLocalDefaultValue to localDefaultResolvesWithAncestorValue
+
+ Field fNew = getClass().getDeclaredField("localDefaultResolvesWithAncestorValue");
+ fNew.setAccessible(true);
+ Field fOld = getClass().getDeclaredField("useLocalDefaultValue");
+ fOld.setAccessible(true);
+
+ if (fNew.get(this)==null) {
+ fNew.set(this, useLocalDefaultValue);
+ } else {
+ if (!fNew.get(this).equals(useLocalDefaultValue)) {
+ throw new IllegalStateException("Incompatible values detected for "+fOld+" ("+fOld.get(this)+") and "+fNew+" ("+fNew.get(this)+")");
+ }
+ }
+ fOld.set(this, null);
+ }
+
+ if (ancestorDefaultInheritable==null) {
+ Field f = getClass().getDeclaredField("ancestorDefaultInheritable");
+ f.setAccessible(true);
+ f.set(this, true);
+ }
+
+ } catch (Exception e) {
+ throw Exceptions.propagate(e);
+ }
+
+ return returnEquivalentConstant(this);
}
@Override
+ public boolean equals(Object obj) {
+ if (obj==null) return false;
+ if (obj instanceof DelegatingConfigInheritance) return equals( ((DelegatingConfigInheritance)obj).getDelegate() );
+ if (obj.getClass().equals(BasicConfigInheritance.class)) {
--- End diff --
we don't have any subclasses so it doesn't matter; have switched to instance, assuming if it is subclasses then the author will extend the equals if needed. also refactored so easier to read and if subclassed easier to extend the check. (you're right, `instanceof` is better as it permits the caller to reuse some of the logic.)
---
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 #462: Inherit config default values
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/462#discussion_r94782620
--- Diff: camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/SpecParameterUnwrappingTest.java ---
@@ -146,8 +151,9 @@ public void testRootParametersUnwrapped() {
CatalogItem<?, ?> item = catalog.getCatalogItem(SYMBOLIC_NAME, TEST_VERSION);
AbstractBrooklynObjectSpec<?,?> spec = catalog.peekSpec(item);
List<SpecParameter<?>> params = spec.getParameters();
- assertEquals(params.size(), NUM_APP_DEFAULT_CONFIG_KEYS + 1, "params="+params);
assertTrue(Iterables.tryFind(params, nameEqualTo("simple")).isPresent());
+ assertTrue(Iterables.tryFind(params, nameEqualTo(SHARED_CONFIG.getName())).isPresent());
+ assertEquals(params.size(), NUM_APP_DEFAULT_CONFIG_KEYS + 2, "params="+params);
--- End diff --
Could introduce `NUM_CONFIG_ENTITY_FOR_TEST_CONFIG_KEYS` and change this to `NUM_APP_DEFAULT_CONFIG_KEYS + NUM_CONFIG_ENTITY_FOR_TEST_CONFIG_KEYS + 1`, not clear why the `+2` otherwise (same for following changes).
---
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 #462: Inherit config default values
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r89772102
--- Diff: core/src/main/java/org/apache/brooklyn/util/core/xstream/ClassRenamingMapper.java ---
@@ -42,7 +42,7 @@ public ClassRenamingMapper(Mapper wrapped, Map<String, String> nameToType) {
@Override
public Class<?> realClass(String elementName) {
- Optional<String> elementNameOpt = Reflections.tryFindMappedName(nameToType, elementName);
+ Maybe<String> elementNameOpt = Reflections.findMappedNameMaybe(nameToType, elementName);
--- End diff --
Why rename this? It was previously following the same naming conventions as guava's `Iterables` (e.g. `tryFind` versus `find`).
Also, if changing it to a `Maybe` then should we explicitly check for it returning `Maybe.of(null)`, rather than risking a NPE later?
---
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 #462: Inherit config default values
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/462#discussion_r94980522
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java ---
@@ -28,51 +32,188 @@
import org.apache.brooklyn.config.ConfigKey;
import org.apache.brooklyn.config.ConfigValueAtContainer;
import org.apache.brooklyn.util.collections.CollectionMerger;
+import org.apache.brooklyn.util.exceptions.Exceptions;
import org.apache.brooklyn.util.exceptions.ReferenceWithError;
import org.apache.brooklyn.util.guava.Maybe;
+import org.apache.brooklyn.util.text.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+@SuppressWarnings("serial")
public class BasicConfigInheritance implements ConfigInheritance {
+ private static final Logger log = LoggerFactory.getLogger(BasicConfigInheritance.class);
+
private static final long serialVersionUID = -5916548049057961051L;
public static final String CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE = "deep_merge";
public static final String CONFLICT_RESOLUTION_STRATEGY_OVERWRITE = "overwrite";
+ public static abstract class DelegatingConfigInheritance implements ConfigInheritance {
+ protected abstract ConfigInheritance getDelegate();
+
+ @Override @Deprecated public InheritanceMode isInherited(ConfigKey<?> key, Object from, Object to) {
+ return getDelegate().isInherited(key, from, to);
+ }
+ @Override public <TContainer, TValue> boolean isReinheritable(ConfigValueAtContainer<TContainer, TValue> parent, ConfigInheritanceContext context) {
+ return getDelegate().isReinheritable(parent, context);
+ }
+ @Override public <TContainer, TValue> boolean considerParent(ConfigValueAtContainer<TContainer, TValue> local, ConfigValueAtContainer<TContainer, TValue> parent, ConfigInheritanceContext context) {
+ return getDelegate().considerParent(local, parent, context);
+ }
+ @Override
+ public <TContainer, TValue> ReferenceWithError<ConfigValueAtContainer<TContainer, TValue>> resolveWithParent(ConfigValueAtContainer<TContainer, TValue> local, ConfigValueAtContainer<TContainer, TValue> resolvedParent, ConfigInheritanceContext context) {
+ return getDelegate().resolveWithParent(local, resolvedParent, context);
+ }
+ @Override
+ public boolean equals(Object obj) {
+ return super.equals(obj) || getDelegate().equals(obj);
+ }
+
+ // standard deserialization method
+ protected ConfigInheritance readResolve() {
+ return returnEquivalentConstant(this);
+ }
+ }
+
+ private static ConfigInheritance returnEquivalentConstant(ConfigInheritance candidate) {
+ for (ConfigInheritance knownMode: Arrays.asList(
+ NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, NEVER_INHERITED, OVERWRITE, BasicConfigInheritance.DEEP_MERGE)) {
+ if (candidate.equals(knownMode)) return knownMode;
+ }
+ if (candidate.equals(new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, true))) {
+ // ignore the ancestor flag for this mode
+ return NEVER_INHERITED;
+ }
+ return candidate;
+ }
+
+ /*
+ * use of delegate is so that stateless classes can be defined to make the serialization nice,
+ * both the name and hiding the implementation detail (also making it easier for that detail to change);
+ * with aliased type names the field names here could even be aliases for the type names.
+ * (we could alternatively have an "alias-for-final-instance" mode in serialization,
+ * to optimize where we know that a java instance is final.)
+ */
+
+ private static class NotReinherited extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** Indicates that a config key value should not be passed down from a container where it is defined.
* Unlike {@link #NEVER_INHERITED} these values can be passed down if set as anonymous keys at a container
* (ie the container does not expect it) to a container which does expect it, but it will not be passed down further.
* If the inheritor also defines a value the parent's value is ignored irrespective
* (as in {@link #OVERWRITE}; see {@link #NOT_REINHERITED_ELSE_DEEP_MERGE} if merging is desired). */
- public static BasicConfigInheritance NOT_REINHERITED = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_OVERWRITE,false);
+ public static ConfigInheritance NOT_REINHERITED = new NotReinherited();
--- End diff --
Suggestion for a less mind-bending, delegate-free implementation:
```
private static class NotReinherited extends BasicConfigInheritance {
public NotReinherited() {super(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, true);}
@SuppressWarnings("unused") private ConfigInheritance readResolve() {return NOT_REINHERITED;};
}
public static ConfigInheritance NOT_REINHERITED = new NotReinherited();
```
---
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 #462: Inherit config default values
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/462#discussion_r94981371
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java ---
@@ -28,51 +32,188 @@
import org.apache.brooklyn.config.ConfigKey;
import org.apache.brooklyn.config.ConfigValueAtContainer;
import org.apache.brooklyn.util.collections.CollectionMerger;
+import org.apache.brooklyn.util.exceptions.Exceptions;
import org.apache.brooklyn.util.exceptions.ReferenceWithError;
import org.apache.brooklyn.util.guava.Maybe;
+import org.apache.brooklyn.util.text.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+@SuppressWarnings("serial")
public class BasicConfigInheritance implements ConfigInheritance {
+ private static final Logger log = LoggerFactory.getLogger(BasicConfigInheritance.class);
+
private static final long serialVersionUID = -5916548049057961051L;
public static final String CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE = "deep_merge";
public static final String CONFLICT_RESOLUTION_STRATEGY_OVERWRITE = "overwrite";
+ public static abstract class DelegatingConfigInheritance implements ConfigInheritance {
+ protected abstract ConfigInheritance getDelegate();
+
+ @Override @Deprecated public InheritanceMode isInherited(ConfigKey<?> key, Object from, Object to) {
+ return getDelegate().isInherited(key, from, to);
+ }
+ @Override public <TContainer, TValue> boolean isReinheritable(ConfigValueAtContainer<TContainer, TValue> parent, ConfigInheritanceContext context) {
+ return getDelegate().isReinheritable(parent, context);
+ }
+ @Override public <TContainer, TValue> boolean considerParent(ConfigValueAtContainer<TContainer, TValue> local, ConfigValueAtContainer<TContainer, TValue> parent, ConfigInheritanceContext context) {
+ return getDelegate().considerParent(local, parent, context);
+ }
+ @Override
+ public <TContainer, TValue> ReferenceWithError<ConfigValueAtContainer<TContainer, TValue>> resolveWithParent(ConfigValueAtContainer<TContainer, TValue> local, ConfigValueAtContainer<TContainer, TValue> resolvedParent, ConfigInheritanceContext context) {
+ return getDelegate().resolveWithParent(local, resolvedParent, context);
+ }
+ @Override
+ public boolean equals(Object obj) {
+ return super.equals(obj) || getDelegate().equals(obj);
+ }
+
+ // standard deserialization method
+ protected ConfigInheritance readResolve() {
+ return returnEquivalentConstant(this);
+ }
+ }
+
+ private static ConfigInheritance returnEquivalentConstant(ConfigInheritance candidate) {
+ for (ConfigInheritance knownMode: Arrays.asList(
+ NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, NEVER_INHERITED, OVERWRITE, BasicConfigInheritance.DEEP_MERGE)) {
+ if (candidate.equals(knownMode)) return knownMode;
+ }
+ if (candidate.equals(new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, true))) {
+ // ignore the ancestor flag for this mode
+ return NEVER_INHERITED;
+ }
+ return candidate;
+ }
+
+ /*
+ * use of delegate is so that stateless classes can be defined to make the serialization nice,
+ * both the name and hiding the implementation detail (also making it easier for that detail to change);
+ * with aliased type names the field names here could even be aliases for the type names.
+ * (we could alternatively have an "alias-for-final-instance" mode in serialization,
+ * to optimize where we know that a java instance is final.)
+ */
+
+ private static class NotReinherited extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** Indicates that a config key value should not be passed down from a container where it is defined.
* Unlike {@link #NEVER_INHERITED} these values can be passed down if set as anonymous keys at a container
* (ie the container does not expect it) to a container which does expect it, but it will not be passed down further.
* If the inheritor also defines a value the parent's value is ignored irrespective
* (as in {@link #OVERWRITE}; see {@link #NOT_REINHERITED_ELSE_DEEP_MERGE} if merging is desired). */
- public static BasicConfigInheritance NOT_REINHERITED = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_OVERWRITE,false);
+ public static ConfigInheritance NOT_REINHERITED = new NotReinherited();
+
+ private static class NotReinheritedElseDeepMerge extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** As {@link #NOT_REINHERITED} but in cases where a value is inherited because a parent did not recognize it,
* if the inheritor also defines a value the two values should be merged. */
- public static BasicConfigInheritance NOT_REINHERITED_ELSE_DEEP_MERGE = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE,false);
- /** Indicates that a key's value should never be inherited, even if defined on a container that does not know the key.
- * Most usages will prefer {@link #NOT_REINHERITED}. */
- public static BasicConfigInheritance NEVER_INHERITED = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_OVERWRITE,true);
+ public static ConfigInheritance NOT_REINHERITED_ELSE_DEEP_MERGE = new NotReinheritedElseDeepMerge();
+
+ private static class NeverInherited extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, false);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
+ /** Indicates that a key's value should never be inherited, even if inherited from a value set on a container that does not know the key.
+ * (Most usages will prefer {@link #NOT_REINHERITED}.) */
+ public static ConfigInheritance NEVER_INHERITED = new NeverInherited();
+
+ private static class Overwrite extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(true, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** Indicates that if a key has a value at both an ancestor and a descendant, the descendant and his descendants
* will prefer the value at the descendant. */
- public static BasicConfigInheritance OVERWRITE = new BasicConfigInheritance(true,CONFLICT_RESOLUTION_STRATEGY_OVERWRITE,false);
+ public static ConfigInheritance OVERWRITE = new Overwrite();
+
+ private static class DeepMerge extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(true, CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** Indicates that if a key has a value at both an ancestor and a descendant, the descendant and his descendants
- * should attempt to merge the values. If the values are not mergable behaviour is undefined
+ * should attempt to merge the values. If the values are not mergeable behaviour is undefined
* (and often the descendant's value will simply overwrite). */
- public static BasicConfigInheritance DEEP_MERGE = new BasicConfigInheritance(true,CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE,false);
+ public static ConfigInheritance DEEP_MERGE = new DeepMerge();
+
+ // support conversion from these legacy fields
+ @SuppressWarnings("deprecation")
+ private static void registerReplacements() {
+ ConfigInheritance.Legacy.registerReplacement(ConfigInheritance.DEEP_MERGE, DEEP_MERGE);
+ ConfigInheritance.Legacy.registerReplacement(ConfigInheritance.ALWAYS, OVERWRITE);
+ ConfigInheritance.Legacy.registerReplacement(ConfigInheritance.NONE, NOT_REINHERITED);
+ }
+ static { registerReplacements(); }
- /** reinheritable? true/false; if false, children/descendants/inheritors will never see it; default true */
+ /** whether a value on a key defined locally should be inheritable by descendants.
+ * if false at a point where a key is defined,
+ * children/descendants/inheritors will not be able to see its value, whether explicit or default.
+ * default true: things are normally reinherited.
+ * <p>
+ * note that this only takes effect where a key is defined locally.
+ * if a key is not defined at an ancestor, a descendant setting this value false will not prevent it
+ * from inheriting values from ancestors.
+ * <p>
+ * typical use case for setting this is false is where a key is consumed and descendants should not
+ * "reconsume" it. for example setting files to install on a VM need only be applied once,
+ * and if it has <b>runtime management</b> hierarchy descendants which also understand that field they
+ * should not install the same files.
+ * (there is normally no reason to set this false in the context of <b>type hierarchy</b> inheritance because
+ * an ancestor type cannot "consume" a value.) */
protected final boolean isReinherited;
- /** conflict-resolution-strategy? in {@link BasicConfigInheritance} supported values are
+
+ /** a symbol indicating a conflict-resolution-strategy understood by the implementation.
+ * in {@link BasicConfigInheritance} supported values are
* {@link #CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE} and {@link #CONFLICT_RESOLUTION_STRATEGY_OVERWRITE}.
- * subclasses may pass null if they provide a custom implementaton of {@link #resolveWithParentCustomStrategy(ConfigValueAtContainer, ConfigValueAtContainer, org.apache.brooklyn.config.ConfigInheritance.ConfigInheritanceContext)} */
+ * subclasses may pass null or a different string if they provide a custom implementaton
--- End diff --
typo `implementaton`
---
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 #462: Inherit config default values
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r94415792
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java ---
@@ -28,51 +32,188 @@
import org.apache.brooklyn.config.ConfigKey;
import org.apache.brooklyn.config.ConfigValueAtContainer;
import org.apache.brooklyn.util.collections.CollectionMerger;
+import org.apache.brooklyn.util.exceptions.Exceptions;
import org.apache.brooklyn.util.exceptions.ReferenceWithError;
import org.apache.brooklyn.util.guava.Maybe;
+import org.apache.brooklyn.util.text.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+@SuppressWarnings("serial")
public class BasicConfigInheritance implements ConfigInheritance {
+ private static final Logger log = LoggerFactory.getLogger(BasicConfigInheritance.class);
+
private static final long serialVersionUID = -5916548049057961051L;
public static final String CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE = "deep_merge";
public static final String CONFLICT_RESOLUTION_STRATEGY_OVERWRITE = "overwrite";
+ public static abstract class DelegatingConfigInheritance implements ConfigInheritance {
+ protected abstract ConfigInheritance getDelegate();
+
+ @Override @Deprecated public InheritanceMode isInherited(ConfigKey<?> key, Object from, Object to) {
+ return getDelegate().isInherited(key, from, to);
+ }
+ @Override public <TContainer, TValue> boolean isReinheritable(ConfigValueAtContainer<TContainer, TValue> parent, ConfigInheritanceContext context) {
+ return getDelegate().isReinheritable(parent, context);
+ }
+ @Override public <TContainer, TValue> boolean considerParent(ConfigValueAtContainer<TContainer, TValue> local, ConfigValueAtContainer<TContainer, TValue> parent, ConfigInheritanceContext context) {
+ return getDelegate().considerParent(local, parent, context);
+ }
+ @Override
+ public <TContainer, TValue> ReferenceWithError<ConfigValueAtContainer<TContainer, TValue>> resolveWithParent(ConfigValueAtContainer<TContainer, TValue> local, ConfigValueAtContainer<TContainer, TValue> resolvedParent, ConfigInheritanceContext context) {
+ return getDelegate().resolveWithParent(local, resolvedParent, context);
+ }
+ @Override
+ public boolean equals(Object obj) {
+ return super.equals(obj) || getDelegate().equals(obj);
+ }
+
+ // standard deserialization method
+ protected ConfigInheritance readResolve() {
+ return returnEquivalentConstant(this);
+ }
+ }
+
+ private static ConfigInheritance returnEquivalentConstant(ConfigInheritance candidate) {
+ for (ConfigInheritance knownMode: Arrays.asList(
+ NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, NEVER_INHERITED, OVERWRITE, BasicConfigInheritance.DEEP_MERGE)) {
+ if (candidate.equals(knownMode)) return knownMode;
+ }
+ if (candidate.equals(new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, true))) {
+ // ignore the ancestor flag for this mode
+ return NEVER_INHERITED;
+ }
+ return candidate;
+ }
+
+ /*
+ * use of delegate is so that stateless classes can be defined to make the serialization nice,
+ * both the name and hiding the implementation detail (also making it easier for that detail to change);
+ * with aliased type names the field names here could even be aliases for the type names.
+ * (we could alternatively have an "alias-for-final-instance" mode in serialization,
+ * to optimize where we know that a java instance is final.)
+ */
+
+ private static class NotReinherited extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** Indicates that a config key value should not be passed down from a container where it is defined.
* Unlike {@link #NEVER_INHERITED} these values can be passed down if set as anonymous keys at a container
* (ie the container does not expect it) to a container which does expect it, but it will not be passed down further.
* If the inheritor also defines a value the parent's value is ignored irrespective
* (as in {@link #OVERWRITE}; see {@link #NOT_REINHERITED_ELSE_DEEP_MERGE} if merging is desired). */
- public static BasicConfigInheritance NOT_REINHERITED = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_OVERWRITE,false);
+ public static ConfigInheritance NOT_REINHERITED = new NotReinherited();
+
+ private static class NotReinheritedElseDeepMerge extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** As {@link #NOT_REINHERITED} but in cases where a value is inherited because a parent did not recognize it,
* if the inheritor also defines a value the two values should be merged. */
- public static BasicConfigInheritance NOT_REINHERITED_ELSE_DEEP_MERGE = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE,false);
- /** Indicates that a key's value should never be inherited, even if defined on a container that does not know the key.
- * Most usages will prefer {@link #NOT_REINHERITED}. */
- public static BasicConfigInheritance NEVER_INHERITED = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_OVERWRITE,true);
+ public static ConfigInheritance NOT_REINHERITED_ELSE_DEEP_MERGE = new NotReinheritedElseDeepMerge();
+
+ private static class NeverInherited extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, false);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
+ /** Indicates that a key's value should never be inherited, even if inherited from a value set on a container that does not know the key.
+ * (Most usages will prefer {@link #NOT_REINHERITED}.) */
+ public static ConfigInheritance NEVER_INHERITED = new NeverInherited();
+
+ private static class Overwrite extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(true, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** Indicates that if a key has a value at both an ancestor and a descendant, the descendant and his descendants
* will prefer the value at the descendant. */
- public static BasicConfigInheritance OVERWRITE = new BasicConfigInheritance(true,CONFLICT_RESOLUTION_STRATEGY_OVERWRITE,false);
+ public static ConfigInheritance OVERWRITE = new Overwrite();
+
+ private static class DeepMerge extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(true, CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** Indicates that if a key has a value at both an ancestor and a descendant, the descendant and his descendants
- * should attempt to merge the values. If the values are not mergable behaviour is undefined
+ * should attempt to merge the values. If the values are not mergeable behaviour is undefined
--- End diff --
I know the "behaviour is undefined" wasn't introduced in this PR, but reading it now makes me question whether this is a desirable state of affairs, or whether this introduces a potential source of confusing and inconsistent behaviours of different implementations. Could we use this PR as an opportunity to replace this "behaviour is undefined" with a clearer behaviour - either overwrite, or maybe throw a well known exception in order to highlight the merge problem?
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/462
failure appears non-deterministic and i think unrelated
```
Tests run: 2036, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 328.296 sec <<< FAILURE! - in TestSuite
(org.apache.brooklyn.core.entity.ApplicationLifecycleStateTest) Time elapsed: 0.183 sec <<< FAILURE!
java.lang.AssertionError: entity=Application[1naq8bil]; attribute=Sensor: service.state (org.apache.brooklyn.core.entity.lifecycle.Lifecycle) expected [running] but found [on-fire]
at org.apache.brooklyn.core.entity.ApplicationLifecycleStateTest.assertHealthContinually(ApplicationLifecycleStateTest.java:196)
at org.apache.brooklyn.core.entity.ApplicationLifecycleStateTest.testStartsThenChildFailsButWithQuorumCausesAppToSucceed(ApplicationLifecycleStateTest.java:170)
```
and
```
Failed tests:
org.apache.brooklyn.core.entity.ApplicationLifecycleStateTest.(org.apache.brooklyn.core.entity.ApplicationLifecycleStateTest)
Run 1: PASS
Run 2: PASS
Run 3: PASS
Run 4: PASS
Run 5: PASS
Run 6: PASS
Run 7: PASS
Run 8: ApplicationLifecycleStateTest.testStartsThenChildFailsButWithQuorumCausesAppToSucceed:170->assertHealthContinually:196 entity=Application[1naq8bil]; attribute=Sensor: service.state (org.apache.brooklyn.core.entity.lifecycle.Lifecycle) expected [running] but found [on-fire]
Run 9: PASS
Run 10: PASS
```
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/462
^^ and neykov's comments re `SpecParameterForInheritance`
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r90567752
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java ---
@@ -170,12 +306,88 @@ public String getConflictResolutionStrategy() {
return conflictResolutionStrategy;
}
+ @Deprecated /** @deprecated since 0.10.0 when it was introduced, prefer {@link #getLocalDefaultResolvesWithAncestorValue()} */
public boolean getUseLocalDefaultValue() {
- return useLocalDefaultValue;
+ return getLocalDefaultResolvesWithAncestorValue();
+ }
+
+ /** see {@link #localDefaultResolvesWithAncestorValue} */
+ public boolean getLocalDefaultResolvesWithAncestorValue() {
+ if (localDefaultResolvesWithAncestorValue==null) {
+ // in case some legacy path is using an improperly deserialized object
+ log.warn("Encountered legacy "+this+" with null localDefaultResolvesWithAncestorValue; transforming", new Throwable("stack trace for legacy "+this));
+ readResolve();
+ }
+ return localDefaultResolvesWithAncestorValue;
+ }
+
+ public boolean getAncestorDefaultInheritable() {
+ if (ancestorDefaultInheritable==null) {
+ log.warn("Encountered legacy "+this+" with null ancestorDefaultInheritable; transforming", new Throwable("stack trace for legacy "+this));
+ readResolve();
+ }
+ return ancestorDefaultInheritable;
}
+ // standard deserialization method
+ private ConfigInheritance readResolve() {
+ try {
+ if (useLocalDefaultValue!=null) {
+ // move away from useLocalDefaultValue to localDefaultResolvesWithAncestorValue
+
+ Field fNew = getClass().getDeclaredField("localDefaultResolvesWithAncestorValue");
+ fNew.setAccessible(true);
+ Field fOld = getClass().getDeclaredField("useLocalDefaultValue");
+ fOld.setAccessible(true);
+
+ if (fNew.get(this)==null) {
+ fNew.set(this, useLocalDefaultValue);
+ } else {
+ if (!fNew.get(this).equals(useLocalDefaultValue)) {
+ throw new IllegalStateException("Incompatible values detected for "+fOld+" ("+fOld.get(this)+") and "+fNew+" ("+fNew.get(this)+")");
+ }
+ }
+ fOld.set(this, null);
+ }
+
+ if (ancestorDefaultInheritable==null) {
+ Field f = getClass().getDeclaredField("ancestorDefaultInheritable");
+ f.setAccessible(true);
+ f.set(this, true);
+ }
+
+ } catch (Exception e) {
+ throw Exceptions.propagate(e);
+ }
+
+ for (ConfigInheritance knownMode: Arrays.asList(
+ NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, NEVER_INHERITED, OVERWRITE, DEEP_MERGE)) {
+ if (equals(knownMode)) return knownMode;
--- End diff --
you're right, adding a `protected readResolve` on the `DelegatingConfigInheritance`
(i'm re-reviewing this after #440)
---
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 #462: Inherit config default values
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/462#discussion_r94985670
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java ---
@@ -170,12 +324,103 @@ public String getConflictResolutionStrategy() {
return conflictResolutionStrategy;
}
+ @Deprecated /** @deprecated since 0.10.0 when it was introduced, prefer {@link #getLocalDefaultResolvesWithAncestorValue()} */
public boolean getUseLocalDefaultValue() {
- return useLocalDefaultValue;
+ return getLocalDefaultResolvesWithAncestorValue();
+ }
+
+ /** see {@link #localDefaultResolvesWithAncestorValue} */
+ public boolean getLocalDefaultResolvesWithAncestorValue() {
+ if (localDefaultResolvesWithAncestorValue==null) {
+ // in case some legacy path is using an improperly deserialized object
+ log.warn("Encountered legacy "+this+" with null localDefaultResolvesWithAncestorValue; transforming", new Throwable("stack trace for legacy "+this));
+ readResolve();
+ }
+ return localDefaultResolvesWithAncestorValue;
+ }
+
+ public boolean getAncestorDefaultInheritable() {
+ if (ancestorDefaultInheritable==null) {
+ log.warn("Encountered legacy "+this+" with null ancestorDefaultInheritable; transforming", new Throwable("stack trace for legacy "+this));
+ readResolve();
+ }
+ return ancestorDefaultInheritable;
+ }
+
+ // standard deserialization method
+ private ConfigInheritance readResolve() {
+ try {
+ if (useLocalDefaultValue!=null) {
+ // move away from useLocalDefaultValue to localDefaultResolvesWithAncestorValue
+
+ Field fNew = getClass().getDeclaredField("localDefaultResolvesWithAncestorValue");
+ fNew.setAccessible(true);
+ Field fOld = getClass().getDeclaredField("useLocalDefaultValue");
+ fOld.setAccessible(true);
+
+ if (fNew.get(this)==null) {
+ fNew.set(this, useLocalDefaultValue);
+ } else {
+ if (!fNew.get(this).equals(useLocalDefaultValue)) {
+ throw new IllegalStateException("Incompatible values detected for "+fOld+" ("+fOld.get(this)+") and "+fNew+" ("+fNew.get(this)+")");
+ }
+ }
+ fOld.set(this, null);
+ }
+
+ if (ancestorDefaultInheritable==null) {
+ Field f = getClass().getDeclaredField("ancestorDefaultInheritable");
+ f.setAccessible(true);
+ f.set(this, true);
+ }
+
+ } catch (Exception e) {
+ throw Exceptions.propagate(e);
+ }
+
+ return returnEquivalentConstant(this);
}
@Override
+ public boolean equals(Object obj) {
+ if (obj==null) return false;
+ if (obj instanceof DelegatingConfigInheritance) return equals( ((DelegatingConfigInheritance)obj).getDelegate() );
+ if (obj.getClass().equals(BasicConfigInheritance.class)) {
--- End diff --
This seems wrong, why not `instanceof`?
You already had to work around it in the delegate.
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r101001789
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void addParameters(AbstractBrooklynObjectSpec<?, ?> spec, List<? e
spec.parametersAdd(BasicSpecParameter.fromSpec(loader.getManagementContext(), spec));
}
if (explicitParams.size() > 0) {
- spec.parametersAdd(explicitParams);
+ spec.parametersReplace(resolveParameters(explicitParams, spec.getParameters(), spec));
+ }
+ }
+
+ /** merge parameters against other parameters and known and type-inherited config keys */
+ static Collection<SpecParameter<?>> resolveParameters(Collection<? extends SpecParameter<?>> newParams, Collection<? extends SpecParameter<?>> existingReferenceParamsToKeep, AbstractBrooklynObjectSpec<?,?> spec) {
+ Map<String,SpecParameter<?>> existingToKeep = MutableMap.of();
+ if (existingReferenceParamsToKeep!=null) {
+ for (SpecParameter<?> p: existingReferenceParamsToKeep) {
+ existingToKeep.put(p.getConfigKey().getName(), p);
+ }
+ }
+
+ List<SpecParameter<?>> result = MutableList.<SpecParameter<?>>of();
+ for (SpecParameter<?> p: newParams) {
+ if (p instanceof SpecParameterForInheritance) {
+ SpecParameter<?> existingP = existingToKeep.remove(p.getConfigKey().getName());
+ if (existingP!=null) {
+ p = ((SpecParameterForInheritance<?>)p).resolveWithAncestor(existingP);
+ } else {
+ // TODO find config keys on the type (not available as parameters)
+ /* we don't currently do this due to low priority; all it means if there is a config key in java,
+ * and a user wishes to expose it as a parameter, they have to redeclare everything;
+ * nothing from the config key in java will be inherited */
+ p = ((SpecParameterForInheritance<?>)p).resolveWithAncestor((ConfigKey<?>)null);
+ }
+ result.add(p);
+ } else {
+ existingToKeep.remove(p.getConfigKey().getName());
+ result.add(p);
+ }
+ }
+ result.addAll(existingToKeep.values());
+ return result;
+ }
+
+ /** instance of {@link SpecParameter} which includes information about what fields are explicitly set,
+ * for use with a subsequent merge */
+ @SuppressWarnings("serial")
+ @Beta
+ static class SpecParameterForInheritance<T> extends BasicSpecParameter<T> {
+
+ private final boolean hasType, hasLabelSet, hasPinnedSet, hasDefaultValue, hasConstraints, hasRuntimeInheritance, hasTypeInheritance;
+
+ private <SensorType> SpecParameterForInheritance(String label, Boolean pinned, ConfigKey<T> config, AttributeSensor<SensorType> sensor,
+ boolean hasType, boolean hasDefaultValue, boolean hasConstraints, boolean hasRuntimeInheritance, boolean hasTypeInheritance) {
+ super(Preconditions.checkNotNull(label!=null ? label : config.getName(), "label or config name must be set"),
+ pinned==null ? true : pinned, config, sensor);
+ this.hasType = hasType;
+ this.hasLabelSet = label!=null;
+ this.hasPinnedSet = pinned!=null;
+ this.hasDefaultValue = hasDefaultValue;
+ this.hasConstraints = hasConstraints;
+ this.hasRuntimeInheritance = hasRuntimeInheritance;
+ this.hasTypeInheritance = hasTypeInheritance;
+ }
+
+ /** used if yaml specifies a parameter, but possibly incomplete, and a spec supertype has a parameter */
+ SpecParameter<?> resolveWithAncestor(SpecParameter<?> ancestor) {
+ if (ancestor==null) return new BasicSpecParameter<>(getLabel(), isPinned(), getConfigKey(), getSensor());
+
+ return new BasicSpecParameter<>(
+ hasLabelSet ? getLabel() : ancestor.getLabel(),
+ hasPinnedSet ? isPinned() : ancestor.isPinned(),
+ resolveWithAncestorConfigKey(ancestor.getConfigKey()),
+ hasType ? getSensor() : ancestor.getSensor());
+ }
+
+ /** as {@link #resolveWithAncestor(SpecParameter)} but where the param redefines/extends a config key coming from a java supertype, rather than a parameter */
+ // TODO not used yet; see calls to the other resolveWithAncestor method,
+ // and see BrooklynComponentTemplateResolver.findAllConfigKeys;
+ // also note whilst it is easiest to do this here, logically it is messy,
+ // and arguably it should be done when converting the spec to an instance
+ SpecParameter<?> resolveWithAncestor(ConfigKey<?> ancestor) {
+ if (ancestor==null) return new BasicSpecParameter<>(getLabel(), isPinned(), getConfigKey(), getSensor());
+
+ // TODO probably want to do this (but it could get expensive!)
+ // Set<Class<?>> types = MutableSet.<Class<?>>builder()
+ // .add(spec.getImplementation())
+ // .add(spec.getType())
+ // .addAll(spec.getAdditionalInterfaces())
+ // .remove(null)
+ // .build();
+ // // order above is important, respected below to take the first one defined
+ // MutableMap<String, ConfigKey<?>> result = MutableMap.copyOf(FlagUtils.findAllConfigKeys(null, types));
+
+ return new BasicSpecParameter<>(
+ getLabel(),
+ isPinned(),
+ resolveWithAncestorConfigKey(ancestor),
+ // TODO port sensor will be lost (see messy code above which sets the port sensor)
--- End diff --
no, we've never done proper combining of sensors when combinining a config definition with an ancestor. and btw this method isn't really used yet, at least this code path isn't, it's in the TODO code path at https://github.com/apache/brooklyn-server/pull/462/files/a777b36e16688ccf9ef53ad6c1d392c8ef0f85bc#diff-94206c636ff5bbab4c4251c3316c129e
---
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 #462: Inherit config default values
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/462#discussion_r94962954
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void addParameters(AbstractBrooklynObjectSpec<?, ?> spec, List<? e
spec.parametersAdd(BasicSpecParameter.fromSpec(loader.getManagementContext(), spec));
}
if (explicitParams.size() > 0) {
- spec.parametersAdd(explicitParams);
+ spec.parametersReplace(resolveParameters(explicitParams, spec.getParameters(), spec));
+ }
+ }
+
+ /** merge parameters against other parameters and known and type-inherited config keys */
+ static Collection<SpecParameter<?>> resolveParameters(Collection<? extends SpecParameter<?>> newParams, Collection<? extends SpecParameter<?>> existingReferenceParamsToKeep, AbstractBrooklynObjectSpec<?,?> spec) {
+ Map<String,SpecParameter<?>> existingToKeep = MutableMap.of();
+ if (existingReferenceParamsToKeep!=null) {
+ for (SpecParameter<?> p: existingReferenceParamsToKeep) {
+ existingToKeep.put(p.getConfigKey().getName(), p);
+ }
+ }
+
+ List<SpecParameter<?>> result = MutableList.<SpecParameter<?>>of();
+ for (SpecParameter<?> p: newParams) {
+ if (p instanceof SpecParameterForInheritance) {
+ SpecParameter<?> existingP = existingToKeep.remove(p.getConfigKey().getName());
+ if (existingP!=null) {
+ p = ((SpecParameterForInheritance<?>)p).resolveWithAncestor(existingP);
+ } else {
+ // TODO find config keys on the type (not available as parameters)
+ /* we don't currently do this due to low priority; all it means if there is a config key in java,
+ * and a user wishes to expose it as a parameter, they have to redeclare everything;
+ * nothing from the config key in java will be inherited */
+ p = ((SpecParameterForInheritance<?>)p).resolveWithAncestor((ConfigKey<?>)null);
+ }
+ result.add(p);
+ } else {
+ existingToKeep.remove(p.getConfigKey().getName());
+ result.add(p);
+ }
+ }
+ result.addAll(existingToKeep.values());
+ return result;
+ }
+
+ /** instance of {@link SpecParameter} which includes information about what fields are explicitly set,
+ * for use with a subsequent merge */
+ @SuppressWarnings("serial")
+ @Beta
+ static class SpecParameterForInheritance<T> extends BasicSpecParameter<T> {
+
+ private final boolean hasType, hasLabelSet, hasPinnedSet, hasDefaultValue, hasConstraints, hasRuntimeInheritance, hasTypeInheritance;
+
+ private <SensorType> SpecParameterForInheritance(String label, Boolean pinned, ConfigKey<T> config, AttributeSensor<SensorType> sensor,
+ boolean hasType, boolean hasDefaultValue, boolean hasConstraints, boolean hasRuntimeInheritance, boolean hasTypeInheritance) {
+ super(Preconditions.checkNotNull(label!=null ? label : config.getName(), "label or config name must be set"),
+ pinned==null ? true : pinned, config, sensor);
+ this.hasType = hasType;
+ this.hasLabelSet = label!=null;
+ this.hasPinnedSet = pinned!=null;
+ this.hasDefaultValue = hasDefaultValue;
+ this.hasConstraints = hasConstraints;
+ this.hasRuntimeInheritance = hasRuntimeInheritance;
+ this.hasTypeInheritance = hasTypeInheritance;
+ }
+
+ /** used if yaml specifies a parameter, but possibly incomplete, and a spec supertype has a parameter */
+ SpecParameter<?> resolveWithAncestor(SpecParameter<?> ancestor) {
+ if (ancestor==null) return new BasicSpecParameter<>(getLabel(), isPinned(), getConfigKey(), getSensor());
+
+ return new BasicSpecParameter<>(
+ hasLabelSet ? getLabel() : ancestor.getLabel(),
+ hasPinnedSet ? isPinned() : ancestor.isPinned(),
+ resolveWithAncestorConfigKey(ancestor.getConfigKey()),
+ hasType ? getSensor() : ancestor.getSensor());
+ }
+
+ /** as {@link #resolveWithAncestor(SpecParameter)} but where the param redefines/extends a config key coming from a java supertype, rather than a parameter */
+ // TODO not used yet; see calls to the other resolveWithAncestor method,
+ // and see BrooklynComponentTemplateResolver.findAllConfigKeys;
+ // also note whilst it is easiest to do this here, logically it is messy,
+ // and arguably it should be done when converting the spec to an instance
+ SpecParameter<?> resolveWithAncestor(ConfigKey<?> ancestor) {
+ if (ancestor==null) return new BasicSpecParameter<>(getLabel(), isPinned(), getConfigKey(), getSensor());
+
+ // TODO probably want to do this (but it could get expensive!)
+ // Set<Class<?>> types = MutableSet.<Class<?>>builder()
+ // .add(spec.getImplementation())
+ // .add(spec.getType())
+ // .addAll(spec.getAdditionalInterfaces())
+ // .remove(null)
+ // .build();
+ // // order above is important, respected below to take the first one defined
+ // MutableMap<String, ConfigKey<?>> result = MutableMap.copyOf(FlagUtils.findAllConfigKeys(null, types));
+
+ return new BasicSpecParameter<>(
+ getLabel(),
+ isPinned(),
+ resolveWithAncestorConfigKey(ancestor),
+ // TODO port sensor will be lost (see messy code above which sets the port sensor)
--- End diff --
Does this break existing blueprints?
---
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 #462: Inherit config default values
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/462#discussion_r94953612
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -227,13 +234,30 @@ public String toString() {
String label = (String)inputDef.get("label");
String description = (String)inputDef.get("description");
String type = (String)inputDef.get("type");
- Object defaultValue = inputDef.get("default");
Boolean pinned = (Boolean)inputDef.get("pinned");
+ boolean hasDefaultValue = inputDef.containsKey("default");
+ Object defaultValue = inputDef.get("default");
if (specialFlagTransformer != null) {
defaultValue = specialFlagTransformer.apply(defaultValue);
}
+ boolean hasConstraints = inputDef.containsKey("constraints");
Predicate<?> constraint = parseConstraints(inputDef.get("constraints"), loader);
- ConfigInheritance parentInheritance = parseInheritance(inputDef.get("inheritance.parent"), loader);
+
+ ConfigInheritance runtimeInheritance;
+ boolean hasRuntimeInheritance;
+ if (inputDef.containsKey("inheritance.runtime")) {
+ hasRuntimeInheritance = true;
+ runtimeInheritance = parseInheritance(inputDef.get("inheritance.runtime"), loader);
+ } else if (inputDef.containsKey("inheritance.parent")) {
+ // this alias will be deprecated
+ hasRuntimeInheritance = true;
+ runtimeInheritance = parseInheritance(inputDef.get("inheritance.parent"), loader);
+ } else {
+ hasRuntimeInheritance = false;
+ runtimeInheritance = parseInheritance(null, loader);
--- End diff --
`runtimeInheritance = null` is more clear
---
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 #462: Inherit config default values
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/462#discussion_r94961073
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void addParameters(AbstractBrooklynObjectSpec<?, ?> spec, List<? e
spec.parametersAdd(BasicSpecParameter.fromSpec(loader.getManagementContext(), spec));
}
if (explicitParams.size() > 0) {
- spec.parametersAdd(explicitParams);
+ spec.parametersReplace(resolveParameters(explicitParams, spec.getParameters(), spec));
+ }
+ }
+
+ /** merge parameters against other parameters and known and type-inherited config keys */
+ static Collection<SpecParameter<?>> resolveParameters(Collection<? extends SpecParameter<?>> newParams, Collection<? extends SpecParameter<?>> existingReferenceParamsToKeep, AbstractBrooklynObjectSpec<?,?> spec) {
+ Map<String,SpecParameter<?>> existingToKeep = MutableMap.of();
+ if (existingReferenceParamsToKeep!=null) {
+ for (SpecParameter<?> p: existingReferenceParamsToKeep) {
+ existingToKeep.put(p.getConfigKey().getName(), p);
+ }
+ }
+
+ List<SpecParameter<?>> result = MutableList.<SpecParameter<?>>of();
+ for (SpecParameter<?> p: newParams) {
+ if (p instanceof SpecParameterForInheritance) {
+ SpecParameter<?> existingP = existingToKeep.remove(p.getConfigKey().getName());
+ if (existingP!=null) {
+ p = ((SpecParameterForInheritance<?>)p).resolveWithAncestor(existingP);
+ } else {
+ // TODO find config keys on the type (not available as parameters)
+ /* we don't currently do this due to low priority; all it means if there is a config key in java,
+ * and a user wishes to expose it as a parameter, they have to redeclare everything;
+ * nothing from the config key in java will be inherited */
+ p = ((SpecParameterForInheritance<?>)p).resolveWithAncestor((ConfigKey<?>)null);
+ }
+ result.add(p);
+ } else {
+ existingToKeep.remove(p.getConfigKey().getName());
+ result.add(p);
+ }
+ }
+ result.addAll(existingToKeep.values());
+ return result;
+ }
+
+ /** instance of {@link SpecParameter} which includes information about what fields are explicitly set,
+ * for use with a subsequent merge */
+ @SuppressWarnings("serial")
+ @Beta
+ static class SpecParameterForInheritance<T> extends BasicSpecParameter<T> {
--- End diff --
What's the case of having this as a separate class vs updating `BasicSpecParameter`?
Personal preference to sticking with just `BasicSpecParameter` ( + `Maybe` for the fields) to avoid `instanceof` checks. For persistence compat - could use the `readObject` trick.
---
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 #462: Inherit config default values
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100509692
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java ---
@@ -28,51 +32,188 @@
import org.apache.brooklyn.config.ConfigKey;
import org.apache.brooklyn.config.ConfigValueAtContainer;
import org.apache.brooklyn.util.collections.CollectionMerger;
+import org.apache.brooklyn.util.exceptions.Exceptions;
import org.apache.brooklyn.util.exceptions.ReferenceWithError;
import org.apache.brooklyn.util.guava.Maybe;
+import org.apache.brooklyn.util.text.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+@SuppressWarnings("serial")
public class BasicConfigInheritance implements ConfigInheritance {
+ private static final Logger log = LoggerFactory.getLogger(BasicConfigInheritance.class);
+
private static final long serialVersionUID = -5916548049057961051L;
public static final String CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE = "deep_merge";
public static final String CONFLICT_RESOLUTION_STRATEGY_OVERWRITE = "overwrite";
+ public static abstract class DelegatingConfigInheritance implements ConfigInheritance {
+ protected abstract ConfigInheritance getDelegate();
+
+ @Override @Deprecated public InheritanceMode isInherited(ConfigKey<?> key, Object from, Object to) {
+ return getDelegate().isInherited(key, from, to);
+ }
+ @Override public <TContainer, TValue> boolean isReinheritable(ConfigValueAtContainer<TContainer, TValue> parent, ConfigInheritanceContext context) {
+ return getDelegate().isReinheritable(parent, context);
+ }
+ @Override public <TContainer, TValue> boolean considerParent(ConfigValueAtContainer<TContainer, TValue> local, ConfigValueAtContainer<TContainer, TValue> parent, ConfigInheritanceContext context) {
+ return getDelegate().considerParent(local, parent, context);
+ }
+ @Override
+ public <TContainer, TValue> ReferenceWithError<ConfigValueAtContainer<TContainer, TValue>> resolveWithParent(ConfigValueAtContainer<TContainer, TValue> local, ConfigValueAtContainer<TContainer, TValue> resolvedParent, ConfigInheritanceContext context) {
+ return getDelegate().resolveWithParent(local, resolvedParent, context);
+ }
+ @Override
+ public boolean equals(Object obj) {
+ return super.equals(obj) || getDelegate().equals(obj);
+ }
+
+ // standard deserialization method
+ protected ConfigInheritance readResolve() {
+ return returnEquivalentConstant(this);
+ }
+ }
+
+ private static ConfigInheritance returnEquivalentConstant(ConfigInheritance candidate) {
+ for (ConfigInheritance knownMode: Arrays.asList(
+ NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, NEVER_INHERITED, OVERWRITE, BasicConfigInheritance.DEEP_MERGE)) {
+ if (candidate.equals(knownMode)) return knownMode;
+ }
+ if (candidate.equals(new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, true))) {
+ // ignore the ancestor flag for this mode
+ return NEVER_INHERITED;
+ }
+ return candidate;
+ }
+
+ /*
+ * use of delegate is so that stateless classes can be defined to make the serialization nice,
+ * both the name and hiding the implementation detail (also making it easier for that detail to change);
+ * with aliased type names the field names here could even be aliases for the type names.
+ * (we could alternatively have an "alias-for-final-instance" mode in serialization,
+ * to optimize where we know that a java instance is final.)
+ */
+
+ private static class NotReinherited extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** Indicates that a config key value should not be passed down from a container where it is defined.
* Unlike {@link #NEVER_INHERITED} these values can be passed down if set as anonymous keys at a container
* (ie the container does not expect it) to a container which does expect it, but it will not be passed down further.
* If the inheritor also defines a value the parent's value is ignored irrespective
* (as in {@link #OVERWRITE}; see {@link #NOT_REINHERITED_ELSE_DEEP_MERGE} if merging is desired). */
- public static BasicConfigInheritance NOT_REINHERITED = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_OVERWRITE,false);
+ public static ConfigInheritance NOT_REINHERITED = new NotReinherited();
+
+ private static class NotReinheritedElseDeepMerge extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** As {@link #NOT_REINHERITED} but in cases where a value is inherited because a parent did not recognize it,
* if the inheritor also defines a value the two values should be merged. */
- public static BasicConfigInheritance NOT_REINHERITED_ELSE_DEEP_MERGE = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE,false);
- /** Indicates that a key's value should never be inherited, even if defined on a container that does not know the key.
- * Most usages will prefer {@link #NOT_REINHERITED}. */
- public static BasicConfigInheritance NEVER_INHERITED = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_OVERWRITE,true);
+ public static ConfigInheritance NOT_REINHERITED_ELSE_DEEP_MERGE = new NotReinheritedElseDeepMerge();
+
+ private static class NeverInherited extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, false);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
+ /** Indicates that a key's value should never be inherited, even if inherited from a value set on a container that does not know the key.
+ * (Most usages will prefer {@link #NOT_REINHERITED}.) */
+ public static ConfigInheritance NEVER_INHERITED = new NeverInherited();
+
+ private static class Overwrite extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(true, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** Indicates that if a key has a value at both an ancestor and a descendant, the descendant and his descendants
* will prefer the value at the descendant. */
- public static BasicConfigInheritance OVERWRITE = new BasicConfigInheritance(true,CONFLICT_RESOLUTION_STRATEGY_OVERWRITE,false);
+ public static ConfigInheritance OVERWRITE = new Overwrite();
+
+ private static class DeepMerge extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(true, CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** Indicates that if a key has a value at both an ancestor and a descendant, the descendant and his descendants
- * should attempt to merge the values. If the values are not mergable behaviour is undefined
+ * should attempt to merge the values. If the values are not mergeable behaviour is undefined
--- End diff --
@geomacy I think this PR is big enough already, and taking too long to get merged. I agree we should look at places we declare that "behaviour is undefined" - some of those should be forbidden (i.e. fail-fast, until we decide to actually support it), others should have clearly defined semantics, and only rarely is "undefined" a reasonable behaviour.
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100795903
--- Diff: camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java ---
@@ -403,8 +408,10 @@ protected ConfigInheritance getDefaultConfigInheritance() {
// name "B" the "val2" then applies to both!
//
// I plan to propose a change on dev@brooklyn, to replace `@SetFromFlag`!
-
- Set<FlagConfigKeyAndValueRecord> allKeys = MutableSet.of();
+
+ // need to de-dupe? (can't use Set bc FCKVR doesn't impl equals/hashcode)
--- End diff --
good job we didn't call it `FlagUnifiedConfigKeyValueRecord` :)
---
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 #462: Inherit config default values
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/462#discussion_r94962175
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void addParameters(AbstractBrooklynObjectSpec<?, ?> spec, List<? e
spec.parametersAdd(BasicSpecParameter.fromSpec(loader.getManagementContext(), spec));
}
if (explicitParams.size() > 0) {
- spec.parametersAdd(explicitParams);
+ spec.parametersReplace(resolveParameters(explicitParams, spec.getParameters(), spec));
+ }
+ }
+
+ /** merge parameters against other parameters and known and type-inherited config keys */
+ static Collection<SpecParameter<?>> resolveParameters(Collection<? extends SpecParameter<?>> newParams, Collection<? extends SpecParameter<?>> existingReferenceParamsToKeep, AbstractBrooklynObjectSpec<?,?> spec) {
+ Map<String,SpecParameter<?>> existingToKeep = MutableMap.of();
+ if (existingReferenceParamsToKeep!=null) {
+ for (SpecParameter<?> p: existingReferenceParamsToKeep) {
+ existingToKeep.put(p.getConfigKey().getName(), p);
+ }
+ }
+
+ List<SpecParameter<?>> result = MutableList.<SpecParameter<?>>of();
+ for (SpecParameter<?> p: newParams) {
+ if (p instanceof SpecParameterForInheritance) {
+ SpecParameter<?> existingP = existingToKeep.remove(p.getConfigKey().getName());
+ if (existingP!=null) {
+ p = ((SpecParameterForInheritance<?>)p).resolveWithAncestor(existingP);
+ } else {
+ // TODO find config keys on the type (not available as parameters)
+ /* we don't currently do this due to low priority; all it means if there is a config key in java,
+ * and a user wishes to expose it as a parameter, they have to redeclare everything;
+ * nothing from the config key in java will be inherited */
+ p = ((SpecParameterForInheritance<?>)p).resolveWithAncestor((ConfigKey<?>)null);
+ }
+ result.add(p);
+ } else {
+ existingToKeep.remove(p.getConfigKey().getName());
+ result.add(p);
+ }
+ }
+ result.addAll(existingToKeep.values());
+ return result;
+ }
+
+ /** instance of {@link SpecParameter} which includes information about what fields are explicitly set,
+ * for use with a subsequent merge */
+ @SuppressWarnings("serial")
+ @Beta
+ static class SpecParameterForInheritance<T> extends BasicSpecParameter<T> {
+
+ private final boolean hasType, hasLabelSet, hasPinnedSet, hasDefaultValue, hasConstraints, hasRuntimeInheritance, hasTypeInheritance;
+
+ private <SensorType> SpecParameterForInheritance(String label, Boolean pinned, ConfigKey<T> config, AttributeSensor<SensorType> sensor,
+ boolean hasType, boolean hasDefaultValue, boolean hasConstraints, boolean hasRuntimeInheritance, boolean hasTypeInheritance) {
+ super(Preconditions.checkNotNull(label!=null ? label : config.getName(), "label or config name must be set"),
+ pinned==null ? true : pinned, config, sensor);
+ this.hasType = hasType;
+ this.hasLabelSet = label!=null;
+ this.hasPinnedSet = pinned!=null;
+ this.hasDefaultValue = hasDefaultValue;
+ this.hasConstraints = hasConstraints;
+ this.hasRuntimeInheritance = hasRuntimeInheritance;
+ this.hasTypeInheritance = hasTypeInheritance;
+ }
+
+ /** used if yaml specifies a parameter, but possibly incomplete, and a spec supertype has a parameter */
+ SpecParameter<?> resolveWithAncestor(SpecParameter<?> ancestor) {
+ if (ancestor==null) return new BasicSpecParameter<>(getLabel(), isPinned(), getConfigKey(), getSensor());
+
+ return new BasicSpecParameter<>(
--- End diff --
Can you guarantee that resolving is always top-down? What I mean, is it possible to call `resolveWithAncestor` with another `SpecParameterForInheritance`?
If so then should update `hasXXX` fields according to the `ancestor` state.
---
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 #462: Inherit config default values
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/462#discussion_r94785585
--- Diff: camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/SpecParameterUnwrappingTest.java ---
@@ -169,19 +175,23 @@ public void testDependantCatalogsInheritParameters(Class<? extends BrooklynObjec
CatalogItem<?, ?> item = catalog.getCatalogItem(SYMBOLIC_NAME, TEST_VERSION);
AbstractBrooklynObjectSpec<?,?> spec = catalog.peekSpec(item);
List<SpecParameter<?>> params = spec.getParameters();
- switch (type.getSimpleName()) {
- case "ConfigEntityForTest":
- assertEquals(params.size(), 3);
- break;
- case "ConfigPolicyForTest":
- assertEquals(params.size(), 2);
- break;
- case "ConfigLocationForTest":
- assertEquals(params.size(), 7);
- break;
- }
+ // should have simple in parent yaml type and sample from parent java type
+ Asserts.assertSize(params, getDefaultsFor(type.getSimpleName()) + 2);
assertTrue(Iterables.tryFind(params, nameEqualTo("simple")).isPresent());
assertTrue(Iterables.tryFind(params, labelEqualTo("simple")).isPresent());
+ assertTrue(Iterables.tryFind(params, nameEqualTo(SHARED_CONFIG.getName())).isPresent());
+ }
+
+ private int getDefaultsFor(String simpleName) {
--- End diff --
Good cleanup, could include it as a second parameter in the provider, this works as well.
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100844827
--- Diff: api/src/main/java/org/apache/brooklyn/api/internal/AbstractBrooklynObjectSpec.java ---
@@ -147,27 +147,28 @@ public SpecT tagsReplace(Iterable<Object> tagsToReplace) {
// it is a CatalogConfig or merely a config key, maybe introducing displayable, or even priority
// (but note part of the reason for CatalogConfig.priority is that java reflection doesn't preserve field order) .
// see also comments on the camp SpecParameterResolver.
+
+ // probably the thing to do is deprecate the ambiguous method in favour of an explicit
@Beta
public SpecT parameters(List<? extends SpecParameter<?>> parameters) {
return parametersReplace(parameters);
}
- /** adds the given parameters */
+ /** adds the given parameters, new ones first so they dominate subsequent ones */
@Beta
public SpecT parametersAdd(List<? extends SpecParameter<?>> parameters) {
// parameters follows immutable pattern, unlike the other fields
Set<SpecParameter<?>> params = MutableSet.<SpecParameter<?>>copyOf(parameters);
Set<SpecParameter<?>> current = MutableSet.<SpecParameter<?>>copyOf(this.parameters);
current.removeAll(params);
- this.parameters = ImmutableList.<SpecParameter<?>>builder()
+ return parametersReplace(ImmutableList.<SpecParameter<?>>builder()
.addAll(params)
.addAll(current)
- .build();
- return self();
+ .build());
}
/** replaces parameters with the given */
@Beta
- public SpecT parametersReplace(List<? extends SpecParameter<?>> parameters) {
+ public SpecT parametersReplace(Collection<? extends SpecParameter<?>> parameters) {
--- End diff --
going with `Iterable`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100846344
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void addParameters(AbstractBrooklynObjectSpec<?, ?> spec, List<? e
spec.parametersAdd(BasicSpecParameter.fromSpec(loader.getManagementContext(), spec));
}
if (explicitParams.size() > 0) {
- spec.parametersAdd(explicitParams);
+ spec.parametersReplace(resolveParameters(explicitParams, spec.getParameters(), spec));
+ }
+ }
+
+ /** merge parameters against other parameters and known and type-inherited config keys */
+ static Collection<SpecParameter<?>> resolveParameters(Collection<? extends SpecParameter<?>> newParams, Collection<? extends SpecParameter<?>> existingReferenceParamsToKeep, AbstractBrooklynObjectSpec<?,?> spec) {
+ Map<String,SpecParameter<?>> existingToKeep = MutableMap.of();
+ if (existingReferenceParamsToKeep!=null) {
+ for (SpecParameter<?> p: existingReferenceParamsToKeep) {
+ existingToKeep.put(p.getConfigKey().getName(), p);
+ }
+ }
+
+ List<SpecParameter<?>> result = MutableList.<SpecParameter<?>>of();
+ for (SpecParameter<?> p: newParams) {
+ if (p instanceof SpecParameterForInheritance) {
+ SpecParameter<?> existingP = existingToKeep.remove(p.getConfigKey().getName());
+ if (existingP!=null) {
+ p = ((SpecParameterForInheritance<?>)p).resolveWithAncestor(existingP);
+ } else {
+ // TODO find config keys on the type (not available as parameters)
+ /* we don't currently do this due to low priority; all it means if there is a config key in java,
+ * and a user wishes to expose it as a parameter, they have to redeclare everything;
+ * nothing from the config key in java will be inherited */
+ p = ((SpecParameterForInheritance<?>)p).resolveWithAncestor((ConfigKey<?>)null);
+ }
+ result.add(p);
+ } else {
+ existingToKeep.remove(p.getConfigKey().getName());
+ result.add(p);
+ }
+ }
+ result.addAll(existingToKeep.values());
+ return result;
+ }
+
+ /** instance of {@link SpecParameter} which includes information about what fields are explicitly set,
+ * for use with a subsequent merge */
+ @SuppressWarnings("serial")
+ @Beta
+ static class SpecParameterForInheritance<T> extends BasicSpecParameter<T> {
--- End diff --
the separate class is to know when something still needs to be inherited and when inheritance has been completed. would welcome a second opinion here @neykov because it is ugly, but i think it should never leak ie the ugliness is entirely hidden.
---
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 #462: Inherit config default values
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100523726
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BrooklynObjectInternal.java ---
@@ -152,8 +152,8 @@
/** This is currently the only way to get some rolled up collections and raw,
* and also to test for the presence of a value (without any default).
* As more accessors are added callers may be asked to migrate.
- * Callers may also consider using {@link #findKeys(com.google.common.base.Predicate)}
- * if that isn't too inefficient. */
+ * Callers may also consider using {@link #findKeysDeprecated(com.google.common.base.Predicate)}
--- End diff --
Do you mean `findKeysDeclared`, rather than `findKeysDeprecated`?
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100796950
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -227,13 +234,30 @@ public String toString() {
String label = (String)inputDef.get("label");
String description = (String)inputDef.get("description");
String type = (String)inputDef.get("type");
- Object defaultValue = inputDef.get("default");
Boolean pinned = (Boolean)inputDef.get("pinned");
+ boolean hasDefaultValue = inputDef.containsKey("default");
+ Object defaultValue = inputDef.get("default");
if (specialFlagTransformer != null) {
defaultValue = specialFlagTransformer.apply(defaultValue);
}
+ boolean hasConstraints = inputDef.containsKey("constraints");
Predicate<?> constraint = parseConstraints(inputDef.get("constraints"), loader);
- ConfigInheritance parentInheritance = parseInheritance(inputDef.get("inheritance.parent"), loader);
+
+ ConfigInheritance runtimeInheritance;
+ boolean hasRuntimeInheritance;
+ if (inputDef.containsKey("inheritance.runtime")) {
+ hasRuntimeInheritance = true;
+ runtimeInheritance = parseInheritance(inputDef.get("inheritance.runtime"), loader);
+ } else if (inputDef.containsKey("inheritance.parent")) {
+ // this alias will be deprecated
--- End diff --
done
---
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 #462: Inherit config default values
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/462#discussion_r94968550
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void addParameters(AbstractBrooklynObjectSpec<?, ?> spec, List<? e
spec.parametersAdd(BasicSpecParameter.fromSpec(loader.getManagementContext(), spec));
}
if (explicitParams.size() > 0) {
- spec.parametersAdd(explicitParams);
+ spec.parametersReplace(resolveParameters(explicitParams, spec.getParameters(), spec));
+ }
+ }
+
+ /** merge parameters against other parameters and known and type-inherited config keys */
+ static Collection<SpecParameter<?>> resolveParameters(Collection<? extends SpecParameter<?>> newParams, Collection<? extends SpecParameter<?>> existingReferenceParamsToKeep, AbstractBrooklynObjectSpec<?,?> spec) {
+ Map<String,SpecParameter<?>> existingToKeep = MutableMap.of();
+ if (existingReferenceParamsToKeep!=null) {
+ for (SpecParameter<?> p: existingReferenceParamsToKeep) {
+ existingToKeep.put(p.getConfigKey().getName(), p);
+ }
+ }
+
+ List<SpecParameter<?>> result = MutableList.<SpecParameter<?>>of();
+ for (SpecParameter<?> p: newParams) {
+ if (p instanceof SpecParameterForInheritance) {
+ SpecParameter<?> existingP = existingToKeep.remove(p.getConfigKey().getName());
+ if (existingP!=null) {
+ p = ((SpecParameterForInheritance<?>)p).resolveWithAncestor(existingP);
+ } else {
+ // TODO find config keys on the type (not available as parameters)
+ /* we don't currently do this due to low priority; all it means if there is a config key in java,
+ * and a user wishes to expose it as a parameter, they have to redeclare everything;
+ * nothing from the config key in java will be inherited */
+ p = ((SpecParameterForInheritance<?>)p).resolveWithAncestor((ConfigKey<?>)null);
+ }
+ result.add(p);
+ } else {
+ existingToKeep.remove(p.getConfigKey().getName());
+ result.add(p);
+ }
+ }
+ result.addAll(existingToKeep.values());
+ return result;
+ }
+
+ /** instance of {@link SpecParameter} which includes information about what fields are explicitly set,
+ * for use with a subsequent merge */
+ @SuppressWarnings("serial")
+ @Beta
+ static class SpecParameterForInheritance<T> extends BasicSpecParameter<T> {
+
+ private final boolean hasType, hasLabelSet, hasPinnedSet, hasDefaultValue, hasConstraints, hasRuntimeInheritance, hasTypeInheritance;
+
+ private <SensorType> SpecParameterForInheritance(String label, Boolean pinned, ConfigKey<T> config, AttributeSensor<SensorType> sensor,
+ boolean hasType, boolean hasDefaultValue, boolean hasConstraints, boolean hasRuntimeInheritance, boolean hasTypeInheritance) {
+ super(Preconditions.checkNotNull(label!=null ? label : config.getName(), "label or config name must be set"),
+ pinned==null ? true : pinned, config, sensor);
+ this.hasType = hasType;
+ this.hasLabelSet = label!=null;
+ this.hasPinnedSet = pinned!=null;
+ this.hasDefaultValue = hasDefaultValue;
+ this.hasConstraints = hasConstraints;
+ this.hasRuntimeInheritance = hasRuntimeInheritance;
+ this.hasTypeInheritance = hasTypeInheritance;
+ }
+
+ /** used if yaml specifies a parameter, but possibly incomplete, and a spec supertype has a parameter */
+ SpecParameter<?> resolveWithAncestor(SpecParameter<?> ancestor) {
+ if (ancestor==null) return new BasicSpecParameter<>(getLabel(), isPinned(), getConfigKey(), getSensor());
+
+ return new BasicSpecParameter<>(
+ hasLabelSet ? getLabel() : ancestor.getLabel(),
+ hasPinnedSet ? isPinned() : ancestor.isPinned(),
+ resolveWithAncestorConfigKey(ancestor.getConfigKey()),
+ hasType ? getSensor() : ancestor.getSensor());
+ }
+
+ /** as {@link #resolveWithAncestor(SpecParameter)} but where the param redefines/extends a config key coming from a java supertype, rather than a parameter */
+ // TODO not used yet; see calls to the other resolveWithAncestor method,
+ // and see BrooklynComponentTemplateResolver.findAllConfigKeys;
+ // also note whilst it is easiest to do this here, logically it is messy,
+ // and arguably it should be done when converting the spec to an instance
+ SpecParameter<?> resolveWithAncestor(ConfigKey<?> ancestor) {
+ if (ancestor==null) return new BasicSpecParameter<>(getLabel(), isPinned(), getConfigKey(), getSensor());
+
+ // TODO probably want to do this (but it could get expensive!)
+ // Set<Class<?>> types = MutableSet.<Class<?>>builder()
+ // .add(spec.getImplementation())
+ // .add(spec.getType())
+ // .addAll(spec.getAdditionalInterfaces())
+ // .remove(null)
+ // .build();
+ // // order above is important, respected below to take the first one defined
+ // MutableMap<String, ConfigKey<?>> result = MutableMap.copyOf(FlagUtils.findAllConfigKeys(null, types));
+
+ return new BasicSpecParameter<>(
+ getLabel(),
+ isPinned(),
+ resolveWithAncestorConfigKey(ancestor),
+ // TODO port sensor will be lost (see messy code above which sets the port sensor)
+ getSensor());
+ }
+
+ @SuppressWarnings({ "unchecked", "rawtypes" })
+ private ConfigKey<?> resolveWithAncestorConfigKey(ConfigKey<?> ancestor) {
+ ConfigKey<?> dominant = getConfigKey();
+ return ConfigKeys.<Object>builder((TypeToken)(hasType ? dominant : ancestor).getTypeToken())
+ .name(dominant.getName())
+ .description((dominant.getDescription()!=null ? dominant : ancestor).getDescription())
+ .defaultValue((hasDefaultValue ? dominant : ancestor).getDefaultValue())
+ .constraint((Predicate<Object>) (hasConstraints ? dominant : ancestor).getConstraint())
+ .runtimeInheritance( (hasRuntimeInheritance ? dominant : ancestor).getInheritanceByContext(InheritanceContext.RUNTIME_MANAGEMENT))
+ .typeInheritance( (hasTypeInheritance ? dominant : ancestor).getInheritanceByContext(InheritanceContext.TYPE_DEFINITION))
+ // not yet configurable from yaml
+ //.reconfigurable()
+ .build();
+ }
+
+ public boolean isHasDefaultValue() {
--- End diff --
This just sounds wrong :). But then need to stick to convention.
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/462
more updates; now just need to:
* fix the bug geomacy found
* rewview comments from neykov re SpecParameterUnwrappingTest.java
---
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 #462: Inherit config default values
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/462#discussion_r100802788
--- Diff: utils/common/src/main/java/org/apache/brooklyn/util/guava/Maybe.java ---
@@ -457,6 +457,8 @@ public boolean equals(Object obj) {
Maybe<?> other = (Maybe<?>)obj;
if (!isPresent())
return !other.isPresent();
+ if (!other.isPresent())
+ return false;
--- End diff --
I suppose it's useful when you put it inside containers (i.e. `remove(Object)` works for sets)
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100788414
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void addParameters(AbstractBrooklynObjectSpec<?, ?> spec, List<? e
spec.parametersAdd(BasicSpecParameter.fromSpec(loader.getManagementContext(), spec));
}
if (explicitParams.size() > 0) {
- spec.parametersAdd(explicitParams);
+ spec.parametersReplace(resolveParameters(explicitParams, spec.getParameters(), spec));
+ }
+ }
+
+ /** merge parameters against other parameters and known and type-inherited config keys */
+ static Collection<SpecParameter<?>> resolveParameters(Collection<? extends SpecParameter<?>> newParams, Collection<? extends SpecParameter<?>> existingReferenceParamsToKeep, AbstractBrooklynObjectSpec<?,?> spec) {
--- End diff --
it would be needed for the `TODO` 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 #462: Inherit config default values
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100507371
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java ---
@@ -170,12 +324,103 @@ public String getConflictResolutionStrategy() {
return conflictResolutionStrategy;
}
+ @Deprecated /** @deprecated since 0.10.0 when it was introduced, prefer {@link #getLocalDefaultResolvesWithAncestorValue()} */
public boolean getUseLocalDefaultValue() {
- return useLocalDefaultValue;
+ return getLocalDefaultResolvesWithAncestorValue();
+ }
+
+ /** see {@link #localDefaultResolvesWithAncestorValue} */
+ public boolean getLocalDefaultResolvesWithAncestorValue() {
+ if (localDefaultResolvesWithAncestorValue==null) {
+ // in case some legacy path is using an improperly deserialized object
+ log.warn("Encountered legacy "+this+" with null localDefaultResolvesWithAncestorValue; transforming", new Throwable("stack trace for legacy "+this));
+ readResolve();
+ }
+ return localDefaultResolvesWithAncestorValue;
+ }
+
+ public boolean getAncestorDefaultInheritable() {
+ if (ancestorDefaultInheritable==null) {
+ log.warn("Encountered legacy "+this+" with null ancestorDefaultInheritable; transforming", new Throwable("stack trace for legacy "+this));
+ readResolve();
+ }
+ return ancestorDefaultInheritable;
+ }
+
+ // standard deserialization method
+ private ConfigInheritance readResolve() {
+ try {
+ if (useLocalDefaultValue!=null) {
+ // move away from useLocalDefaultValue to localDefaultResolvesWithAncestorValue
+
+ Field fNew = getClass().getDeclaredField("localDefaultResolvesWithAncestorValue");
+ fNew.setAccessible(true);
+ Field fOld = getClass().getDeclaredField("useLocalDefaultValue");
+ fOld.setAccessible(true);
+
+ if (fNew.get(this)==null) {
+ fNew.set(this, useLocalDefaultValue);
+ } else {
+ if (!fNew.get(this).equals(useLocalDefaultValue)) {
+ throw new IllegalStateException("Incompatible values detected for "+fOld+" ("+fOld.get(this)+") and "+fNew+" ("+fNew.get(this)+")");
+ }
+ }
+ fOld.set(this, null);
+ }
+
+ if (ancestorDefaultInheritable==null) {
+ Field f = getClass().getDeclaredField("ancestorDefaultInheritable");
+ f.setAccessible(true);
+ f.set(this, true);
+ }
+
+ } catch (Exception e) {
+ throw Exceptions.propagate(e);
+ }
+
+ return returnEquivalentConstant(this);
}
@Override
+ public boolean equals(Object obj) {
--- End diff --
Implements `equals` but not `hashCode`, which is wrong.
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r90568746
--- Diff: core/src/main/java/org/apache/brooklyn/util/core/xstream/ClassRenamingMapper.java ---
@@ -42,7 +42,7 @@ public ClassRenamingMapper(Mapper wrapped, Map<String, String> nameToType) {
@Override
public Class<?> realClass(String elementName) {
- Optional<String> elementNameOpt = Reflections.tryFindMappedName(nameToType, elementName);
+ Maybe<String> elementNameOpt = Reflections.findMappedNameMaybe(nameToType, elementName);
--- End diff --
the Maybe.absent() contains a useful error message. the Optional.absent() was completely unhelpful.
re null i don't know whether null is valid or not ... and nor do i need to, i don't think -- it's not our responsibility here (delegating the rename to the super) to check whether the rename method gave us the right thing.
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/462
this improves fix for BROOKLYN-328 and now also fixes many of the issues described at BROOKLYN-329, specifically
*( causes default values to be inheritable
and
* 5) the ConfigMap.findKeys method never looked at the keys declared on the container; I'm tidying that in order to test the above
* 6) if a yaml subtype reclares a supertype's parameter (from yaml) or config key (from java) only partially, the results are not merged as one would expect, but instead all parts of the super's declaration are lost
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100807828
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java ---
@@ -82,9 +223,17 @@ public InheritanceMode isInherited(ConfigKey<?> key, Object from, Object to) {
protected <TContainer, TValue> void checkInheritanceContext(ConfigValueAtContainer<TContainer, TValue> local, ConfigInheritanceContext context) {
ConfigInheritance rightInheritance = ConfigInheritances.findInheritance(local, context, this);
- if (!equals(rightInheritance))
+ if (!isSameRootInstanceAs(rightInheritance)) {
throw new IllegalStateException("Low level inheritance computation error: caller should invoke on "+rightInheritance+" "
+ "(the inheritance at "+local+"), not "+this);
+ }
+ }
+
+ private boolean isSameRootInstanceAs(ConfigInheritance other) {
+ if (other==null) return false;
+ if (this==other) return true;
+ if (other instanceof DelegatingConfigInheritance) return isSameRootInstanceAs( ((DelegatingConfigInheritance)other).getDelegate() );
--- End diff --
`equals` would work but it wouldn't catch quite as many errors; it should be invoked on the same _instance_; have added comment to explain. you're right dropping the delegate pattern would allow this line to be removed but i think losing the internal fields in persisted state is worth 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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100796338
--- Diff: core/src/main/java/org/apache/brooklyn/core/mgmt/EntityManagementUtils.java ---
@@ -255,8 +255,9 @@ private static void mergeWrapperParentSpecToChildEntity(EntitySpec<? extends App
wrappedChild.locationSpecs(wrapperParent.getLocationSpecs());
wrappedChild.locations(wrapperParent.getLocations());
- if (!wrapperParent.getParameters().isEmpty())
- wrappedChild.parametersReplace(wrapperParent.getParameters());
+ if (!wrapperParent.getParameters().isEmpty()) {
+ wrappedChild.parametersAdd(wrapperParent.getParameters());
--- End diff --
good spot
---
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 #462: Inherit config default values
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/462#discussion_r94985326
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java ---
@@ -170,12 +324,103 @@ public String getConflictResolutionStrategy() {
return conflictResolutionStrategy;
}
+ @Deprecated /** @deprecated since 0.10.0 when it was introduced, prefer {@link #getLocalDefaultResolvesWithAncestorValue()} */
public boolean getUseLocalDefaultValue() {
- return useLocalDefaultValue;
+ return getLocalDefaultResolvesWithAncestorValue();
+ }
+
+ /** see {@link #localDefaultResolvesWithAncestorValue} */
+ public boolean getLocalDefaultResolvesWithAncestorValue() {
+ if (localDefaultResolvesWithAncestorValue==null) {
+ // in case some legacy path is using an improperly deserialized object
+ log.warn("Encountered legacy "+this+" with null localDefaultResolvesWithAncestorValue; transforming", new Throwable("stack trace for legacy "+this));
+ readResolve();
--- End diff --
Are you suspecting the object could be coming from a non-xstream based deserialization? If not, then this is too much on the defensive 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #462: Inherit config default values
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r93231733
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java ---
@@ -397,48 +397,83 @@ protected ConfigInheritance getDefaultRuntimeInheritance() {
final InheritanceContext context = InheritanceContext.RUNTIME_MANAGEMENT;
ConfigInheritance currentInheritance = ConfigInheritances.findInheritance(queryKey, context, getDefaultRuntimeInheritance());
-
+
BasicConfigValueAtContainer<TContainer, Object> last = null;
-
+
while (c!=null) {
Maybe<Object> v = getRawValueAtContainer(c, queryKey);
BasicConfigValueAtContainer<TContainer, Object> next = new BasicConfigValueAtContainer<TContainer, Object>(c, getKeyAtContainer(c, queryKey), v);
-
+
if (last!=null && !currentInheritance.considerParent(last, next, context)) break;
-
+
ConfigInheritance currentInheritanceExplicit = ConfigInheritances.findInheritance(next.getKey(), InheritanceContext.RUNTIME_MANAGEMENT, null);
if (currentInheritanceExplicit!=null) {
if (count>0 && !currentInheritanceExplicit.isReinheritable(next, context)) break;
currentInheritance = currentInheritanceExplicit;
}
-
+
if (next.isValueExplicitlySet()) result.add(0, next);
last = next;
c = getParentOfContainer(c);
count++;
}
-
+
return result;
}
-
- @Override
+
+ @Override @Deprecated
public Set<ConfigKey<?>> findKeys(Predicate<? super ConfigKey<?>> filter) {
+ return findKeys(filter, KeyFindingMode.PRESENT_NOT_RESOLVED);
+ }
+
+ @Override
+ public Set<ConfigKey<?>> findKeysDeclared(Predicate<? super ConfigKey<?>> filter) {
+ return findKeys(filter, KeyFindingMode.DECLARED_OR_PRESENT);
+ }
+
+ @Override
+ public Set<ConfigKey<?>> findKeysPresent(Predicate<? super ConfigKey<?>> filter) {
+ return findKeys(filter, KeyFindingMode.PRESENT_BUT_RESOLVED);
+ }
+
+ protected enum KeyFindingMode { DECLARED_OR_PRESENT, PRESENT_BUT_RESOLVED, PRESENT_NOT_RESOLVED }
--- End diff --
Maybe `PRESENT_BUT_RESOLVED` could be called instead `PRESENT_AND_RESOLVED`? Does that convey the sense better?
---
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 #462: Inherit config default values
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r94440837
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java ---
@@ -355,40 +355,40 @@ protected ConfigInheritance getDefaultRuntimeInheritance() {
// prefer default and type of ownKey
Maybe<T> defaultValue = raw ? Maybe.<T>absent() :
ownKey.hasDefaultValue() ? coerceFn.apply(Maybe.of((Object)ownKey.getDefaultValue())) :
- queryKey.hasDefaultValue() ? coerceFn.apply(Maybe.of((Object)queryKey.getDefaultValue())) :
- Maybe.<T>absent();
-
- if (ownKey instanceof ConfigKeySelfExtracting) {
-
- Function<TContainer, Maybe<Object>> lookupFn = new Function<TContainer, Maybe<Object>>() {
- @Override public Maybe<Object> apply(TContainer input) {
- // lookup against ownKey as it may do extra resolution (eg grab *.* subkeys if a map)
- Maybe<Object> result = getRawValueAtContainer(input, ownKey);
- if (!raw) result = resolveRawValueFromContainer(input, ownKey, result);
- return result;
- }
- };
- Function<TContainer, TContainer> parentFn = new Function<TContainer, TContainer>() {
- @Override public TContainer apply(TContainer input) {
- return getParentOfContainer(input);
+ queryKey.hasDefaultValue() ? coerceFn.apply(Maybe.of((Object)queryKey.getDefaultValue())) :
+ Maybe.<T>absent();
+
+ if (ownKey instanceof ConfigKeySelfExtracting) {
--- End diff --
Indentation is wrong
---
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 #462: Inherit config default values
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r93292616
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void addParameters(AbstractBrooklynObjectSpec<?, ?> spec, List<? e
spec.parametersAdd(BasicSpecParameter.fromSpec(loader.getManagementContext(), spec));
}
if (explicitParams.size() > 0) {
- spec.parametersAdd(explicitParams);
+ spec.parametersReplace(resolveParameters(explicitParams, spec.getParameters(), spec));
+ }
+ }
+
+ /** merge parameters against other parameters and known and type-inherited config keys */
+ static Collection<SpecParameter<?>> resolveParameters(Collection<? extends SpecParameter<?>> newParams, Collection<? extends SpecParameter<?>> existingReferenceParamsToKeep, AbstractBrooklynObjectSpec<?,?> spec) {
+ Map<String,SpecParameter<?>> existingToKeep = MutableMap.of();
+ if (existingReferenceParamsToKeep!=null) {
+ for (SpecParameter<?> p: existingReferenceParamsToKeep) {
+ existingToKeep.put(p.getConfigKey().getName(), p);
+ }
+ }
+
+ List<SpecParameter<?>> result = MutableList.<SpecParameter<?>>of();
+ for (SpecParameter<?> p: newParams) {
+ if (p instanceof SpecParameterForInheritance) {
+ SpecParameter<?> existingP = existingToKeep.remove(p.getConfigKey().getName());
+ if (existingP!=null) {
+ p = ((SpecParameterForInheritance<?>)p).resolveWithAncestor(existingP);
+ } else {
+ // TODO find config keys on the type (not available as parameters)
+ /* we don't currently do this due to low priority; all it means if there is a config key in java,
+ * and a user wishes to expose it as a parameter, they have to redeclare everything;
+ * nothing from the config key in java will be inherited */
+ p = ((SpecParameterForInheritance<?>)p).resolveWithAncestor((ConfigKey<?>)null);
+ }
+ result.add(p);
+ } else {
+ existingToKeep.remove(p.getConfigKey().getName());
+ result.add(p);
+ }
+ }
+ result.addAll(existingToKeep.values());
+ return result;
+ }
+
+ /** instance of {@link SpecParameter} which includes information about what fields are explicitly set,
+ * for use with a subsequent merge */
+ @SuppressWarnings("serial")
+ @Beta
+ static class SpecParameterForInheritance<T> extends BasicSpecParameter<T> {
+
+ private final boolean hasType, hasLabelSet, hasPinnedSet, hasDefaultValue, hasConstraints, hasRuntimeInheritance, hasTypeInheritance;
+
+ private <SensorType> SpecParameterForInheritance(String label, Boolean pinned, ConfigKey<T> config, AttributeSensor<SensorType> sensor,
+ boolean hasType, boolean hasDefaultValue, boolean hasConstraints, boolean hasRuntimeInheritance, boolean hasTypeInheritance) {
+ super(Preconditions.checkNotNull(label!=null ? label : config.getName(), "label or config name must be set"),
+ pinned==null ? true : pinned, config, sensor);
+ this.hasType = hasType;
+ this.hasLabelSet = label!=null;
+ this.hasPinnedSet = pinned!=null;
+ this.hasDefaultValue = hasDefaultValue;
+ this.hasConstraints = hasConstraints;
+ this.hasRuntimeInheritance = hasRuntimeInheritance;
+ this.hasTypeInheritance = hasTypeInheritance;
+ }
+
+ /** used if yaml specifies a parameter, but possibly incomplete, and a spec supertype has a parameter */
+ SpecParameter<?> resolveWithAncestor(SpecParameter<?> ancestor) {
+ if (ancestor==null) return new BasicSpecParameter<>(getLabel(), isPinned(), getConfigKey(), getSensor());
+
+ return new BasicSpecParameter<>(
+ hasLabelSet ? getLabel() : ancestor.getLabel(),
+ hasPinnedSet ? isPinned() : ancestor.isPinned(),
+ resolveWithAncestorConfigKey(ancestor.getConfigKey()),
+ hasType ? getSensor() : ancestor.getSensor());
--- End diff --
why is the decision between `getSensor()` and `ancestor.getSensor()` predicated upon `hasType` particularly? I don't see how they are related.
---
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 #462: Inherit config default values
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r101298270
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BrooklynDynamicType.java ---
@@ -175,20 +179,29 @@ protected static void buildConfigKeys(Class<? extends BrooklynObject> clazz, Abs
LOG.warn("cannot access config key (skipping): "+f);
}
}
+ Collection<Class<?>> interfaces = MutableSet.copyOf(Arrays.asList(clazz.getInterfaces()));
LinkedHashSet<String> keys = new LinkedHashSet<String>(configKeysAll.keys());
for (String kn: keys) {
List<FieldAndValue<ConfigKey<?>>> kk = Lists.newArrayList(configKeysAll.get(kn));
- if (kk.size()>1) {
- // remove anything which extends another value in the list
- for (FieldAndValue<ConfigKey<?>> k: kk) {
- ConfigKey<?> key = value(k);
- if (key instanceof BasicConfigKeyOverwriting) {
- ConfigKey<?> parent = ((BasicConfigKeyOverwriting<?>)key).getParentKey();
- // find and remove the parent from consideration
- for (FieldAndValue<ConfigKey<?>> k2: kk) {
- if (value(k2) == parent)
- configKeysAll.remove(kn, k2);
- }
+ // remove anything which isn't reinheritable, or which is overridden
+ for (FieldAndValue<ConfigKey<?>> k: kk) {
+ ConfigKey<?> key = value(k);
+ if (!ConfigInheritances.isKeyReinheritable(key, InheritanceContext.TYPE_DEFINITION)) {
+ if (k.field.getDeclaringClass().equals(clazz)) {
+ // proceed if key is declared on this class
+ } else if (interfaces.contains(k.field.getDeclaringClass())) {
+ // proceed if key is declared on an exlicitly declared interface
+ } else {
+ // key not re-inheritable from parent so not visible here
+ configKeysAll.remove(k.value.getName(), k);
+ }
+ }
+ if (key instanceof BasicConfigKeyOverwriting) {
+ ConfigKey<?> parent = ((BasicConfigKeyOverwriting<?>)key).getParentKey();
+ // find and remove the parent from consideration
+ for (FieldAndValue<ConfigKey<?>> k2: kk) {
+ if (value(k2) == parent)
+ configKeysAll.remove(kn, k2);
}
}
kk = Lists.newArrayList(configKeysAll.get(kn));
--- End diff --
You've moved this update of `kk` inside the `for( ... k : kk)` loop, which doesn't look right. Updating the list variable each time round an iterator initialised from that variable?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100808041
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java ---
@@ -170,12 +324,103 @@ public String getConflictResolutionStrategy() {
return conflictResolutionStrategy;
}
+ @Deprecated /** @deprecated since 0.10.0 when it was introduced, prefer {@link #getLocalDefaultResolvesWithAncestorValue()} */
public boolean getUseLocalDefaultValue() {
- return useLocalDefaultValue;
+ return getLocalDefaultResolvesWithAncestorValue();
+ }
+
+ /** see {@link #localDefaultResolvesWithAncestorValue} */
+ public boolean getLocalDefaultResolvesWithAncestorValue() {
+ if (localDefaultResolvesWithAncestorValue==null) {
+ // in case some legacy path is using an improperly deserialized object
+ log.warn("Encountered legacy "+this+" with null localDefaultResolvesWithAncestorValue; transforming", new Throwable("stack trace for legacy "+this));
+ readResolve();
--- End diff --
i think this should never happen; logging is safety first in case it is
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100798508
--- Diff: utils/common/src/main/java/org/apache/brooklyn/util/guava/Maybe.java ---
@@ -457,6 +457,8 @@ public boolean equals(Object obj) {
Maybe<?> other = (Maybe<?>)obj;
if (!isPresent())
return !other.isPresent();
+ if (!other.isPresent())
+ return false;
--- End diff --
that seems harsh to me; could cause problems if we serialized a `Maybe`?
could compare the exceptions but for now I will just add javadoc
---
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 #462: Inherit config default values
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r94390638
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void addParameters(AbstractBrooklynObjectSpec<?, ?> spec, List<? e
spec.parametersAdd(BasicSpecParameter.fromSpec(loader.getManagementContext(), spec));
}
if (explicitParams.size() > 0) {
- spec.parametersAdd(explicitParams);
+ spec.parametersReplace(resolveParameters(explicitParams, spec.getParameters(), spec));
+ }
+ }
+
+ /** merge parameters against other parameters and known and type-inherited config keys */
+ static Collection<SpecParameter<?>> resolveParameters(Collection<? extends SpecParameter<?>> newParams, Collection<? extends SpecParameter<?>> existingReferenceParamsToKeep, AbstractBrooklynObjectSpec<?,?> spec) {
+ Map<String,SpecParameter<?>> existingToKeep = MutableMap.of();
+ if (existingReferenceParamsToKeep!=null) {
+ for (SpecParameter<?> p: existingReferenceParamsToKeep) {
+ existingToKeep.put(p.getConfigKey().getName(), p);
+ }
+ }
+
+ List<SpecParameter<?>> result = MutableList.<SpecParameter<?>>of();
+ for (SpecParameter<?> p: newParams) {
+ if (p instanceof SpecParameterForInheritance) {
+ SpecParameter<?> existingP = existingToKeep.remove(p.getConfigKey().getName());
+ if (existingP!=null) {
+ p = ((SpecParameterForInheritance<?>)p).resolveWithAncestor(existingP);
+ } else {
+ // TODO find config keys on the type (not available as parameters)
+ /* we don't currently do this due to low priority; all it means if there is a config key in java,
+ * and a user wishes to expose it as a parameter, they have to redeclare everything;
+ * nothing from the config key in java will be inherited */
+ p = ((SpecParameterForInheritance<?>)p).resolveWithAncestor((ConfigKey<?>)null);
+ }
+ result.add(p);
+ } else {
+ existingToKeep.remove(p.getConfigKey().getName());
+ result.add(p);
+ }
+ }
+ result.addAll(existingToKeep.values());
+ return result;
+ }
+
+ /** instance of {@link SpecParameter} which includes information about what fields are explicitly set,
+ * for use with a subsequent merge */
+ @SuppressWarnings("serial")
+ @Beta
+ static class SpecParameterForInheritance<T> extends BasicSpecParameter<T> {
+
+ private final boolean hasType, hasLabelSet, hasPinnedSet, hasDefaultValue, hasConstraints, hasRuntimeInheritance, hasTypeInheritance;
+
+ private <SensorType> SpecParameterForInheritance(String label, Boolean pinned, ConfigKey<T> config, AttributeSensor<SensorType> sensor,
+ boolean hasType, boolean hasDefaultValue, boolean hasConstraints, boolean hasRuntimeInheritance, boolean hasTypeInheritance) {
+ super(Preconditions.checkNotNull(label!=null ? label : config.getName(), "label or config name must be set"),
+ pinned==null ? true : pinned, config, sensor);
+ this.hasType = hasType;
+ this.hasLabelSet = label!=null;
+ this.hasPinnedSet = pinned!=null;
+ this.hasDefaultValue = hasDefaultValue;
+ this.hasConstraints = hasConstraints;
+ this.hasRuntimeInheritance = hasRuntimeInheritance;
+ this.hasTypeInheritance = hasTypeInheritance;
+ }
+
+ /** used if yaml specifies a parameter, but possibly incomplete, and a spec supertype has a parameter */
+ SpecParameter<?> resolveWithAncestor(SpecParameter<?> ancestor) {
+ if (ancestor==null) return new BasicSpecParameter<>(getLabel(), isPinned(), getConfigKey(), getSensor());
+
+ return new BasicSpecParameter<>(
+ hasLabelSet ? getLabel() : ancestor.getLabel(),
+ hasPinnedSet ? isPinned() : ancestor.isPinned(),
+ resolveWithAncestorConfigKey(ancestor.getConfigKey()),
+ hasType ? getSensor() : ancestor.getSensor());
--- End diff --
ignore that; have looked at it with a fresh eye, the `type` _is_ the sensor type of the parameter.
---
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 #462: Inherit config default values
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/462
---
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 #462: Inherit config default values
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/462#discussion_r94924294
--- Diff: api/src/main/java/org/apache/brooklyn/api/objs/Configurable.java ---
@@ -98,6 +99,14 @@
*/
<T> T set(HasConfigKey<T> key, Task<T> val);
- Set<ConfigKey<?>> findKeys(Predicate<? super ConfigKey<?>> predicate);
+ /** @deprecated since 0.11.0 see {@link ConfigMap#findKeys(Predicate)} */
+ @Deprecated
+ Set<ConfigKey<?>> findKeys(Predicate<? super ConfigKey<?>> filter);
+
+ /** see {@link ConfigMap#findKeysDelcared(Predicate)} */
--- End diff --
Typo `Delcared`
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/462
@geomacy good find in your failing test [above](https://github.com/apache/brooklyn-server/pull/462#issuecomment-270408039) -- we do not filter out hidden ancestor config keys whenever we populate the list of declared config keys, neither in:
* `BasicSpecParameter.fromSpec(..)`, where we extend a spec from a spec; nor
* `InternalEntityFactory.loadUnitializedEntity(...)`, where we create a spec for a java class; nor
* `BrooklynDynamicType.buildConfigKeys(..)`, where we record the entity signature for a java type
i think it is worth fixing 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 #462: Inherit config default values
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/462#discussion_r94973110
--- Diff: utils/common/src/main/java/org/apache/brooklyn/util/guava/Maybe.java ---
@@ -457,6 +457,8 @@ public boolean equals(Object obj) {
Maybe<?> other = (Maybe<?>)obj;
if (!isPresent())
return !other.isPresent();
+ if (!other.isPresent())
+ return false;
--- End diff --
`Optional.absent()` returns `true` if `other==this`. Could be useful to keep the same convention.
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/462
failure seems unrelated / intermittent ?
```
Failed tests:
org.apache.brooklyn.core.entity.ApplicationLifecycleStateTest.(org.apache.brooklyn.core.entity.ApplicationLifecycleStateTest)
Run 1: PASS
Run 2: PASS
Run 3: PASS
Run 4: PASS
Run 5: PASS
Run 6: PASS
Run 7: PASS
Run 8: ApplicationLifecycleStateTest.testStartsThenChildFailsButWithQuorumCausesAppToSucceed:170->assertHealthContinually:196 entity=Application[csqdskg2]; attribute=Sensor: service.state (org.apache.brooklyn.core.entity.lifecycle.Lifecycle) expected [running] but found [on-fire]
Run 9: PASS
Run 10: PASS
```
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/462
rebased on master; still need to address comments above
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100795410
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java ---
@@ -28,51 +32,188 @@
import org.apache.brooklyn.config.ConfigKey;
import org.apache.brooklyn.config.ConfigValueAtContainer;
import org.apache.brooklyn.util.collections.CollectionMerger;
+import org.apache.brooklyn.util.exceptions.Exceptions;
import org.apache.brooklyn.util.exceptions.ReferenceWithError;
import org.apache.brooklyn.util.guava.Maybe;
+import org.apache.brooklyn.util.text.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+@SuppressWarnings("serial")
public class BasicConfigInheritance implements ConfigInheritance {
+ private static final Logger log = LoggerFactory.getLogger(BasicConfigInheritance.class);
+
private static final long serialVersionUID = -5916548049057961051L;
public static final String CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE = "deep_merge";
public static final String CONFLICT_RESOLUTION_STRATEGY_OVERWRITE = "overwrite";
+ public static abstract class DelegatingConfigInheritance implements ConfigInheritance {
+ protected abstract ConfigInheritance getDelegate();
+
+ @Override @Deprecated public InheritanceMode isInherited(ConfigKey<?> key, Object from, Object to) {
+ return getDelegate().isInherited(key, from, to);
+ }
+ @Override public <TContainer, TValue> boolean isReinheritable(ConfigValueAtContainer<TContainer, TValue> parent, ConfigInheritanceContext context) {
+ return getDelegate().isReinheritable(parent, context);
+ }
+ @Override public <TContainer, TValue> boolean considerParent(ConfigValueAtContainer<TContainer, TValue> local, ConfigValueAtContainer<TContainer, TValue> parent, ConfigInheritanceContext context) {
+ return getDelegate().considerParent(local, parent, context);
+ }
+ @Override
+ public <TContainer, TValue> ReferenceWithError<ConfigValueAtContainer<TContainer, TValue>> resolveWithParent(ConfigValueAtContainer<TContainer, TValue> local, ConfigValueAtContainer<TContainer, TValue> resolvedParent, ConfigInheritanceContext context) {
+ return getDelegate().resolveWithParent(local, resolvedParent, context);
+ }
+ @Override
+ public boolean equals(Object obj) {
+ return super.equals(obj) || getDelegate().equals(obj);
+ }
+
+ // standard deserialization method
+ protected ConfigInheritance readResolve() {
+ return returnEquivalentConstant(this);
+ }
+ }
+
+ private static ConfigInheritance returnEquivalentConstant(ConfigInheritance candidate) {
+ for (ConfigInheritance knownMode: Arrays.asList(
+ NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, NEVER_INHERITED, OVERWRITE, BasicConfigInheritance.DEEP_MERGE)) {
+ if (candidate.equals(knownMode)) return knownMode;
+ }
+ if (candidate.equals(new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, true))) {
+ // ignore the ancestor flag for this mode
+ return NEVER_INHERITED;
+ }
+ return candidate;
+ }
+
+ /*
+ * use of delegate is so that stateless classes can be defined to make the serialization nice,
+ * both the name and hiding the implementation detail (also making it easier for that detail to change);
+ * with aliased type names the field names here could even be aliases for the type names.
+ * (we could alternatively have an "alias-for-final-instance" mode in serialization,
+ * to optimize where we know that a java instance is final.)
+ */
+
+ private static class NotReinherited extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** Indicates that a config key value should not be passed down from a container where it is defined.
* Unlike {@link #NEVER_INHERITED} these values can be passed down if set as anonymous keys at a container
* (ie the container does not expect it) to a container which does expect it, but it will not be passed down further.
* If the inheritor also defines a value the parent's value is ignored irrespective
* (as in {@link #OVERWRITE}; see {@link #NOT_REINHERITED_ELSE_DEEP_MERGE} if merging is desired). */
- public static BasicConfigInheritance NOT_REINHERITED = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_OVERWRITE,false);
+ public static ConfigInheritance NOT_REINHERITED = new NotReinherited();
+
+ private static class NotReinheritedElseDeepMerge extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** As {@link #NOT_REINHERITED} but in cases where a value is inherited because a parent did not recognize it,
* if the inheritor also defines a value the two values should be merged. */
- public static BasicConfigInheritance NOT_REINHERITED_ELSE_DEEP_MERGE = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE,false);
- /** Indicates that a key's value should never be inherited, even if defined on a container that does not know the key.
- * Most usages will prefer {@link #NOT_REINHERITED}. */
- public static BasicConfigInheritance NEVER_INHERITED = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_OVERWRITE,true);
+ public static ConfigInheritance NOT_REINHERITED_ELSE_DEEP_MERGE = new NotReinheritedElseDeepMerge();
+
+ private static class NeverInherited extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, false);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
+ /** Indicates that a key's value should never be inherited, even if inherited from a value set on a container that does not know the key.
+ * (Most usages will prefer {@link #NOT_REINHERITED}.) */
+ public static ConfigInheritance NEVER_INHERITED = new NeverInherited();
+
+ private static class Overwrite extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(true, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** Indicates that if a key has a value at both an ancestor and a descendant, the descendant and his descendants
* will prefer the value at the descendant. */
- public static BasicConfigInheritance OVERWRITE = new BasicConfigInheritance(true,CONFLICT_RESOLUTION_STRATEGY_OVERWRITE,false);
+ public static ConfigInheritance OVERWRITE = new Overwrite();
+
+ private static class DeepMerge extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(true, CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** Indicates that if a key has a value at both an ancestor and a descendant, the descendant and his descendants
- * should attempt to merge the values. If the values are not mergable behaviour is undefined
+ * should attempt to merge the values. If the values are not mergeable behaviour is undefined
* (and often the descendant's value will simply overwrite). */
- public static BasicConfigInheritance DEEP_MERGE = new BasicConfigInheritance(true,CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE,false);
+ public static ConfigInheritance DEEP_MERGE = new DeepMerge();
+
+ // support conversion from these legacy fields
+ @SuppressWarnings("deprecation")
+ private static void registerReplacements() {
+ ConfigInheritance.Legacy.registerReplacement(ConfigInheritance.DEEP_MERGE, DEEP_MERGE);
+ ConfigInheritance.Legacy.registerReplacement(ConfigInheritance.ALWAYS, OVERWRITE);
+ ConfigInheritance.Legacy.registerReplacement(ConfigInheritance.NONE, NOT_REINHERITED);
+ }
+ static { registerReplacements(); }
- /** reinheritable? true/false; if false, children/descendants/inheritors will never see it; default true */
+ /** whether a value on a key defined locally should be inheritable by descendants.
+ * if false at a point where a key is defined,
+ * children/descendants/inheritors will not be able to see its value, whether explicit or default.
+ * default true: things are normally reinherited.
+ * <p>
+ * note that this only takes effect where a key is defined locally.
+ * if a key is not defined at an ancestor, a descendant setting this value false will not prevent it
+ * from inheriting values from ancestors.
+ * <p>
+ * typical use case for setting this is false is where a key is consumed and descendants should not
+ * "reconsume" it. for example setting files to install on a VM need only be applied once,
+ * and if it has <b>runtime management</b> hierarchy descendants which also understand that field they
+ * should not install the same files.
+ * (there is normally no reason to set this false in the context of <b>type hierarchy</b> inheritance because
+ * an ancestor type cannot "consume" a value.) */
protected final boolean isReinherited;
- /** conflict-resolution-strategy? in {@link BasicConfigInheritance} supported values are
+
+ /** a symbol indicating a conflict-resolution-strategy understood by the implementation.
+ * in {@link BasicConfigInheritance} supported values are
* {@link #CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE} and {@link #CONFLICT_RESOLUTION_STRATEGY_OVERWRITE}.
- * subclasses may pass null if they provide a custom implementaton of {@link #resolveWithParentCustomStrategy(ConfigValueAtContainer, ConfigValueAtContainer, org.apache.brooklyn.config.ConfigInheritance.ConfigInheritanceContext)} */
+ * subclasses may pass null or a different string if they provide a custom implementaton
+ * of {@link #resolveWithParentCustomStrategy(ConfigValueAtContainer, ConfigValueAtContainer, org.apache.brooklyn.config.ConfigInheritance.ConfigInheritanceContext)} */
@Nullable protected final String conflictResolutionStrategy;
- /** use-local-default-value? true/false; if true, overwrite above means "always ignore (even if null)"; default false
- * whereas merge means "use local default (if non-null)" */
- protected final boolean useLocalDefaultValue;
+
+ /** @deprecated since 0.10.0 when this was introduced, now renamed {@link #localDefaultResolvesWithAncestorValue} */
+ @Deprecated protected final Boolean useLocalDefaultValue;
+ /** whether a local default value should be considered for resolution in the presence of an ancestor value.
+ * can use true with overwrite to mean don't inherit, or true with merge to mean local default merged on top of inherited
+ * (but be careful here, if local default is null in a merge it will delete ancestor values).
+ * <p>
+ * in most cases this is false, meaning a default value is ignored if the parent has a value.
+ * <p>
+ * null should not be used. a boxed object is taken (as opposed to a primitive boolean) only in order to support migration.
+ */
+ @Nonnull
+ protected final Boolean localDefaultResolvesWithAncestorValue;
- protected BasicConfigInheritance(boolean isReinherited, @Nullable String conflictResolutionStrategy, boolean useLocalDefaultValue) {
+ /** whether a default set in an ancestor container's key definition will be considered as the
+ * local default value at descendants who don't define any other value (nothing set locally and local default is null);
+ * <p>
+ * if true (now the usual behaviour), if an ancestor defines a default and a descendant doesn't, the ancestor's value will be taken as a default.
+ * if it is also the case that localDefaultResolvesWithAncestorValue is true at the <i>ancestor</i> then a descendant who
+ * defines a local default value (with this field true) will have its conflict resolution strategy
+ * applied with the ancestor's default value.
+ * <p>
+ * if this is false, ancestor defaults are completely ignored; prior to 0.10.0 this was the normal behaviour,
+ * but it caused surprises where default values in parameters did not take effect.
+ * <p>
+ * null should not be used. a boxed object is taken (as opposed to a primitive boolean) only in order to support migration.
+ */
+ @Nonnull
+ protected final Boolean ancestorDefaultInheritable;
--- End diff --
have thought about this: the two conditions are independent dimensions, and it would matter if we used `localDefaultResolvesWithAncestorValue` for any purpose other than blocking inheritance from ancestors. have added more comments to explain, including your words about the "independent dimensions" which is a good point: including the control dimensions lets the logic be correct, even if we only expose a subset of possibilities to the end user (which simplifies a little bit the complexity of config inheritance).
---
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 #462: Inherit config default values
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/462#discussion_r94961965
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void addParameters(AbstractBrooklynObjectSpec<?, ?> spec, List<? e
spec.parametersAdd(BasicSpecParameter.fromSpec(loader.getManagementContext(), spec));
}
if (explicitParams.size() > 0) {
- spec.parametersAdd(explicitParams);
+ spec.parametersReplace(resolveParameters(explicitParams, spec.getParameters(), spec));
+ }
+ }
+
+ /** merge parameters against other parameters and known and type-inherited config keys */
+ static Collection<SpecParameter<?>> resolveParameters(Collection<? extends SpecParameter<?>> newParams, Collection<? extends SpecParameter<?>> existingReferenceParamsToKeep, AbstractBrooklynObjectSpec<?,?> spec) {
+ Map<String,SpecParameter<?>> existingToKeep = MutableMap.of();
+ if (existingReferenceParamsToKeep!=null) {
+ for (SpecParameter<?> p: existingReferenceParamsToKeep) {
+ existingToKeep.put(p.getConfigKey().getName(), p);
+ }
+ }
+
+ List<SpecParameter<?>> result = MutableList.<SpecParameter<?>>of();
+ for (SpecParameter<?> p: newParams) {
+ if (p instanceof SpecParameterForInheritance) {
+ SpecParameter<?> existingP = existingToKeep.remove(p.getConfigKey().getName());
+ if (existingP!=null) {
+ p = ((SpecParameterForInheritance<?>)p).resolveWithAncestor(existingP);
+ } else {
+ // TODO find config keys on the type (not available as parameters)
+ /* we don't currently do this due to low priority; all it means if there is a config key in java,
+ * and a user wishes to expose it as a parameter, they have to redeclare everything;
+ * nothing from the config key in java will be inherited */
+ p = ((SpecParameterForInheritance<?>)p).resolveWithAncestor((ConfigKey<?>)null);
+ }
+ result.add(p);
+ } else {
+ existingToKeep.remove(p.getConfigKey().getName());
+ result.add(p);
+ }
+ }
+ result.addAll(existingToKeep.values());
+ return result;
+ }
+
+ /** instance of {@link SpecParameter} which includes information about what fields are explicitly set,
+ * for use with a subsequent merge */
+ @SuppressWarnings("serial")
+ @Beta
+ static class SpecParameterForInheritance<T> extends BasicSpecParameter<T> {
+
+ private final boolean hasType, hasLabelSet, hasPinnedSet, hasDefaultValue, hasConstraints, hasRuntimeInheritance, hasTypeInheritance;
+
+ private <SensorType> SpecParameterForInheritance(String label, Boolean pinned, ConfigKey<T> config, AttributeSensor<SensorType> sensor,
+ boolean hasType, boolean hasDefaultValue, boolean hasConstraints, boolean hasRuntimeInheritance, boolean hasTypeInheritance) {
+ super(Preconditions.checkNotNull(label!=null ? label : config.getName(), "label or config name must be set"),
+ pinned==null ? true : pinned, config, sensor);
+ this.hasType = hasType;
+ this.hasLabelSet = label!=null;
+ this.hasPinnedSet = pinned!=null;
+ this.hasDefaultValue = hasDefaultValue;
+ this.hasConstraints = hasConstraints;
+ this.hasRuntimeInheritance = hasRuntimeInheritance;
+ this.hasTypeInheritance = hasTypeInheritance;
+ }
+
+ /** used if yaml specifies a parameter, but possibly incomplete, and a spec supertype has a parameter */
+ SpecParameter<?> resolveWithAncestor(SpecParameter<?> ancestor) {
+ if (ancestor==null) return new BasicSpecParameter<>(getLabel(), isPinned(), getConfigKey(), getSensor());
+
+ return new BasicSpecParameter<>(
+ hasLabelSet ? getLabel() : ancestor.getLabel(),
+ hasPinnedSet ? isPinned() : ancestor.isPinned(),
+ resolveWithAncestorConfigKey(ancestor.getConfigKey()),
+ hasType ? getSensor() : ancestor.getSensor());
--- End diff --
Wrong indentation - all should start at the same column.
---
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 #462: Inherit config default values
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/462#discussion_r94960067
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void addParameters(AbstractBrooklynObjectSpec<?, ?> spec, List<? e
spec.parametersAdd(BasicSpecParameter.fromSpec(loader.getManagementContext(), spec));
}
if (explicitParams.size() > 0) {
- spec.parametersAdd(explicitParams);
+ spec.parametersReplace(resolveParameters(explicitParams, spec.getParameters(), spec));
+ }
+ }
+
+ /** merge parameters against other parameters and known and type-inherited config keys */
+ static Collection<SpecParameter<?>> resolveParameters(Collection<? extends SpecParameter<?>> newParams, Collection<? extends SpecParameter<?>> existingReferenceParamsToKeep, AbstractBrooklynObjectSpec<?,?> spec) {
+ Map<String,SpecParameter<?>> existingToKeep = MutableMap.of();
+ if (existingReferenceParamsToKeep!=null) {
+ for (SpecParameter<?> p: existingReferenceParamsToKeep) {
+ existingToKeep.put(p.getConfigKey().getName(), p);
+ }
+ }
+
+ List<SpecParameter<?>> result = MutableList.<SpecParameter<?>>of();
+ for (SpecParameter<?> p: newParams) {
+ if (p instanceof SpecParameterForInheritance) {
+ SpecParameter<?> existingP = existingToKeep.remove(p.getConfigKey().getName());
+ if (existingP!=null) {
+ p = ((SpecParameterForInheritance<?>)p).resolveWithAncestor(existingP);
+ } else {
+ // TODO find config keys on the type (not available as parameters)
--- End diff --
Suggest we introduce support for both cases at the same time to avoid confusion of when the inheritance works and when it doesn't.
---
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 #462: Inherit config default values
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r94418145
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java ---
@@ -28,51 +32,188 @@
import org.apache.brooklyn.config.ConfigKey;
import org.apache.brooklyn.config.ConfigValueAtContainer;
import org.apache.brooklyn.util.collections.CollectionMerger;
+import org.apache.brooklyn.util.exceptions.Exceptions;
import org.apache.brooklyn.util.exceptions.ReferenceWithError;
import org.apache.brooklyn.util.guava.Maybe;
+import org.apache.brooklyn.util.text.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+@SuppressWarnings("serial")
public class BasicConfigInheritance implements ConfigInheritance {
+ private static final Logger log = LoggerFactory.getLogger(BasicConfigInheritance.class);
+
private static final long serialVersionUID = -5916548049057961051L;
public static final String CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE = "deep_merge";
public static final String CONFLICT_RESOLUTION_STRATEGY_OVERWRITE = "overwrite";
+ public static abstract class DelegatingConfigInheritance implements ConfigInheritance {
+ protected abstract ConfigInheritance getDelegate();
+
+ @Override @Deprecated public InheritanceMode isInherited(ConfigKey<?> key, Object from, Object to) {
+ return getDelegate().isInherited(key, from, to);
+ }
+ @Override public <TContainer, TValue> boolean isReinheritable(ConfigValueAtContainer<TContainer, TValue> parent, ConfigInheritanceContext context) {
+ return getDelegate().isReinheritable(parent, context);
+ }
+ @Override public <TContainer, TValue> boolean considerParent(ConfigValueAtContainer<TContainer, TValue> local, ConfigValueAtContainer<TContainer, TValue> parent, ConfigInheritanceContext context) {
+ return getDelegate().considerParent(local, parent, context);
+ }
+ @Override
+ public <TContainer, TValue> ReferenceWithError<ConfigValueAtContainer<TContainer, TValue>> resolveWithParent(ConfigValueAtContainer<TContainer, TValue> local, ConfigValueAtContainer<TContainer, TValue> resolvedParent, ConfigInheritanceContext context) {
+ return getDelegate().resolveWithParent(local, resolvedParent, context);
+ }
+ @Override
+ public boolean equals(Object obj) {
+ return super.equals(obj) || getDelegate().equals(obj);
+ }
+
+ // standard deserialization method
+ protected ConfigInheritance readResolve() {
+ return returnEquivalentConstant(this);
+ }
+ }
+
+ private static ConfigInheritance returnEquivalentConstant(ConfigInheritance candidate) {
+ for (ConfigInheritance knownMode: Arrays.asList(
+ NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, NEVER_INHERITED, OVERWRITE, BasicConfigInheritance.DEEP_MERGE)) {
+ if (candidate.equals(knownMode)) return knownMode;
+ }
+ if (candidate.equals(new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, true))) {
+ // ignore the ancestor flag for this mode
+ return NEVER_INHERITED;
+ }
+ return candidate;
+ }
+
+ /*
+ * use of delegate is so that stateless classes can be defined to make the serialization nice,
+ * both the name and hiding the implementation detail (also making it easier for that detail to change);
+ * with aliased type names the field names here could even be aliases for the type names.
+ * (we could alternatively have an "alias-for-final-instance" mode in serialization,
+ * to optimize where we know that a java instance is final.)
+ */
+
+ private static class NotReinherited extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** Indicates that a config key value should not be passed down from a container where it is defined.
* Unlike {@link #NEVER_INHERITED} these values can be passed down if set as anonymous keys at a container
* (ie the container does not expect it) to a container which does expect it, but it will not be passed down further.
* If the inheritor also defines a value the parent's value is ignored irrespective
* (as in {@link #OVERWRITE}; see {@link #NOT_REINHERITED_ELSE_DEEP_MERGE} if merging is desired). */
- public static BasicConfigInheritance NOT_REINHERITED = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_OVERWRITE,false);
+ public static ConfigInheritance NOT_REINHERITED = new NotReinherited();
+
+ private static class NotReinheritedElseDeepMerge extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** As {@link #NOT_REINHERITED} but in cases where a value is inherited because a parent did not recognize it,
* if the inheritor also defines a value the two values should be merged. */
- public static BasicConfigInheritance NOT_REINHERITED_ELSE_DEEP_MERGE = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE,false);
- /** Indicates that a key's value should never be inherited, even if defined on a container that does not know the key.
- * Most usages will prefer {@link #NOT_REINHERITED}. */
- public static BasicConfigInheritance NEVER_INHERITED = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_OVERWRITE,true);
+ public static ConfigInheritance NOT_REINHERITED_ELSE_DEEP_MERGE = new NotReinheritedElseDeepMerge();
+
+ private static class NeverInherited extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, false);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
+ /** Indicates that a key's value should never be inherited, even if inherited from a value set on a container that does not know the key.
+ * (Most usages will prefer {@link #NOT_REINHERITED}.) */
+ public static ConfigInheritance NEVER_INHERITED = new NeverInherited();
+
+ private static class Overwrite extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(true, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** Indicates that if a key has a value at both an ancestor and a descendant, the descendant and his descendants
* will prefer the value at the descendant. */
- public static BasicConfigInheritance OVERWRITE = new BasicConfigInheritance(true,CONFLICT_RESOLUTION_STRATEGY_OVERWRITE,false);
+ public static ConfigInheritance OVERWRITE = new Overwrite();
+
+ private static class DeepMerge extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(true, CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** Indicates that if a key has a value at both an ancestor and a descendant, the descendant and his descendants
- * should attempt to merge the values. If the values are not mergable behaviour is undefined
+ * should attempt to merge the values. If the values are not mergeable behaviour is undefined
* (and often the descendant's value will simply overwrite). */
- public static BasicConfigInheritance DEEP_MERGE = new BasicConfigInheritance(true,CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE,false);
+ public static ConfigInheritance DEEP_MERGE = new DeepMerge();
+
+ // support conversion from these legacy fields
+ @SuppressWarnings("deprecation")
+ private static void registerReplacements() {
+ ConfigInheritance.Legacy.registerReplacement(ConfigInheritance.DEEP_MERGE, DEEP_MERGE);
+ ConfigInheritance.Legacy.registerReplacement(ConfigInheritance.ALWAYS, OVERWRITE);
+ ConfigInheritance.Legacy.registerReplacement(ConfigInheritance.NONE, NOT_REINHERITED);
+ }
+ static { registerReplacements(); }
- /** reinheritable? true/false; if false, children/descendants/inheritors will never see it; default true */
+ /** whether a value on a key defined locally should be inheritable by descendants.
+ * if false at a point where a key is defined,
+ * children/descendants/inheritors will not be able to see its value, whether explicit or default.
+ * default true: things are normally reinherited.
+ * <p>
+ * note that this only takes effect where a key is defined locally.
+ * if a key is not defined at an ancestor, a descendant setting this value false will not prevent it
+ * from inheriting values from ancestors.
+ * <p>
+ * typical use case for setting this is false is where a key is consumed and descendants should not
+ * "reconsume" it. for example setting files to install on a VM need only be applied once,
+ * and if it has <b>runtime management</b> hierarchy descendants which also understand that field they
+ * should not install the same files.
+ * (there is normally no reason to set this false in the context of <b>type hierarchy</b> inheritance because
+ * an ancestor type cannot "consume" a value.) */
protected final boolean isReinherited;
- /** conflict-resolution-strategy? in {@link BasicConfigInheritance} supported values are
+
+ /** a symbol indicating a conflict-resolution-strategy understood by the implementation.
+ * in {@link BasicConfigInheritance} supported values are
* {@link #CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE} and {@link #CONFLICT_RESOLUTION_STRATEGY_OVERWRITE}.
- * subclasses may pass null if they provide a custom implementaton of {@link #resolveWithParentCustomStrategy(ConfigValueAtContainer, ConfigValueAtContainer, org.apache.brooklyn.config.ConfigInheritance.ConfigInheritanceContext)} */
+ * subclasses may pass null or a different string if they provide a custom implementaton
+ * of {@link #resolveWithParentCustomStrategy(ConfigValueAtContainer, ConfigValueAtContainer, org.apache.brooklyn.config.ConfigInheritance.ConfigInheritanceContext)} */
@Nullable protected final String conflictResolutionStrategy;
- /** use-local-default-value? true/false; if true, overwrite above means "always ignore (even if null)"; default false
- * whereas merge means "use local default (if non-null)" */
- protected final boolean useLocalDefaultValue;
+
+ /** @deprecated since 0.10.0 when this was introduced, now renamed {@link #localDefaultResolvesWithAncestorValue} */
+ @Deprecated protected final Boolean useLocalDefaultValue;
+ /** whether a local default value should be considered for resolution in the presence of an ancestor value.
+ * can use true with overwrite to mean don't inherit, or true with merge to mean local default merged on top of inherited
--- End diff --
add a link for clarity - "can use true with {@link CONFLICT_RESOLUTION_STRATEGY_OVERWRITE} to mean..." etc. and same for merge strategy.
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/462
Could cause an issue if absents are used as keys with the intention that
they all be different but that would be weird. Have checked our hashCode
and it's a constant for absents already.
---
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 #462: Inherit config default values
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on the issue:
https://github.com/apache/brooklyn-server/pull/462
In general I think this looks good to me and does what it says on the tin; I just had
the few observations and questions above.
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100805561
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java ---
@@ -28,51 +32,188 @@
import org.apache.brooklyn.config.ConfigKey;
import org.apache.brooklyn.config.ConfigValueAtContainer;
import org.apache.brooklyn.util.collections.CollectionMerger;
+import org.apache.brooklyn.util.exceptions.Exceptions;
import org.apache.brooklyn.util.exceptions.ReferenceWithError;
import org.apache.brooklyn.util.guava.Maybe;
+import org.apache.brooklyn.util.text.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+@SuppressWarnings("serial")
public class BasicConfigInheritance implements ConfigInheritance {
+ private static final Logger log = LoggerFactory.getLogger(BasicConfigInheritance.class);
+
private static final long serialVersionUID = -5916548049057961051L;
public static final String CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE = "deep_merge";
public static final String CONFLICT_RESOLUTION_STRATEGY_OVERWRITE = "overwrite";
+ public static abstract class DelegatingConfigInheritance implements ConfigInheritance {
+ protected abstract ConfigInheritance getDelegate();
+
+ @Override @Deprecated public InheritanceMode isInherited(ConfigKey<?> key, Object from, Object to) {
+ return getDelegate().isInherited(key, from, to);
+ }
+ @Override public <TContainer, TValue> boolean isReinheritable(ConfigValueAtContainer<TContainer, TValue> parent, ConfigInheritanceContext context) {
+ return getDelegate().isReinheritable(parent, context);
+ }
+ @Override public <TContainer, TValue> boolean considerParent(ConfigValueAtContainer<TContainer, TValue> local, ConfigValueAtContainer<TContainer, TValue> parent, ConfigInheritanceContext context) {
+ return getDelegate().considerParent(local, parent, context);
+ }
+ @Override
+ public <TContainer, TValue> ReferenceWithError<ConfigValueAtContainer<TContainer, TValue>> resolveWithParent(ConfigValueAtContainer<TContainer, TValue> local, ConfigValueAtContainer<TContainer, TValue> resolvedParent, ConfigInheritanceContext context) {
+ return getDelegate().resolveWithParent(local, resolvedParent, context);
+ }
+ @Override
+ public boolean equals(Object obj) {
+ return super.equals(obj) || getDelegate().equals(obj);
+ }
+
+ // standard deserialization method
+ protected ConfigInheritance readResolve() {
+ return returnEquivalentConstant(this);
+ }
+ }
+
+ private static ConfigInheritance returnEquivalentConstant(ConfigInheritance candidate) {
+ for (ConfigInheritance knownMode: Arrays.asList(
+ NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, NEVER_INHERITED, OVERWRITE, BasicConfigInheritance.DEEP_MERGE)) {
+ if (candidate.equals(knownMode)) return knownMode;
+ }
+ if (candidate.equals(new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, true))) {
--- End diff --
agree
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100808284
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java ---
@@ -170,12 +324,103 @@ public String getConflictResolutionStrategy() {
return conflictResolutionStrategy;
}
+ @Deprecated /** @deprecated since 0.10.0 when it was introduced, prefer {@link #getLocalDefaultResolvesWithAncestorValue()} */
public boolean getUseLocalDefaultValue() {
- return useLocalDefaultValue;
+ return getLocalDefaultResolvesWithAncestorValue();
+ }
+
+ /** see {@link #localDefaultResolvesWithAncestorValue} */
+ public boolean getLocalDefaultResolvesWithAncestorValue() {
+ if (localDefaultResolvesWithAncestorValue==null) {
+ // in case some legacy path is using an improperly deserialized object
+ log.warn("Encountered legacy "+this+" with null localDefaultResolvesWithAncestorValue; transforming", new Throwable("stack trace for legacy "+this));
+ readResolve();
+ }
+ return localDefaultResolvesWithAncestorValue;
+ }
+
+ public boolean getAncestorDefaultInheritable() {
+ if (ancestorDefaultInheritable==null) {
+ log.warn("Encountered legacy "+this+" with null ancestorDefaultInheritable; transforming", new Throwable("stack trace for legacy "+this));
+ readResolve();
+ }
+ return ancestorDefaultInheritable;
+ }
+
+ // standard deserialization method
+ private ConfigInheritance readResolve() {
+ try {
+ if (useLocalDefaultValue!=null) {
+ // move away from useLocalDefaultValue to localDefaultResolvesWithAncestorValue
+
+ Field fNew = getClass().getDeclaredField("localDefaultResolvesWithAncestorValue");
+ fNew.setAccessible(true);
+ Field fOld = getClass().getDeclaredField("useLocalDefaultValue");
+ fOld.setAccessible(true);
+
+ if (fNew.get(this)==null) {
+ fNew.set(this, useLocalDefaultValue);
+ } else {
+ if (!fNew.get(this).equals(useLocalDefaultValue)) {
+ throw new IllegalStateException("Incompatible values detected for "+fOld+" ("+fOld.get(this)+") and "+fNew+" ("+fNew.get(this)+")");
+ }
+ }
+ fOld.set(this, null);
+ }
+
+ if (ancestorDefaultInheritable==null) {
+ Field f = getClass().getDeclaredField("ancestorDefaultInheritable");
+ f.setAccessible(true);
+ f.set(this, true);
+ }
--- End diff --
reflection because fields are final; added comment
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #462: Inherit config default values
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/462#discussion_r94984001
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java ---
@@ -82,9 +223,17 @@ public InheritanceMode isInherited(ConfigKey<?> key, Object from, Object to) {
protected <TContainer, TValue> void checkInheritanceContext(ConfigValueAtContainer<TContainer, TValue> local, ConfigInheritanceContext context) {
ConfigInheritance rightInheritance = ConfigInheritances.findInheritance(local, context, this);
- if (!equals(rightInheritance))
+ if (!isSameRootInstanceAs(rightInheritance)) {
throw new IllegalStateException("Low level inheritance computation error: caller should invoke on "+rightInheritance+" "
+ "(the inheritance at "+local+"), not "+this);
+ }
+ }
+
+ private boolean isSameRootInstanceAs(ConfigInheritance other) {
+ if (other==null) return false;
+ if (this==other) return true;
+ if (other instanceof DelegatingConfigInheritance) return isSameRootInstanceAs( ((DelegatingConfigInheritance)other).getDelegate() );
--- End diff --
Another reason to avoid the delegate. Why is `equals` a bad idea?
Note that you could still end up with an anonymous `BasicConfigInheritance` coming from persistence - they are not resolved to the singletons.
---
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 #462: Inherit config default values
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r93294912
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void addParameters(AbstractBrooklynObjectSpec<?, ?> spec, List<? e
spec.parametersAdd(BasicSpecParameter.fromSpec(loader.getManagementContext(), spec));
}
if (explicitParams.size() > 0) {
- spec.parametersAdd(explicitParams);
+ spec.parametersReplace(resolveParameters(explicitParams, spec.getParameters(), spec));
+ }
+ }
+
+ /** merge parameters against other parameters and known and type-inherited config keys */
+ static Collection<SpecParameter<?>> resolveParameters(Collection<? extends SpecParameter<?>> newParams, Collection<? extends SpecParameter<?>> existingReferenceParamsToKeep, AbstractBrooklynObjectSpec<?,?> spec) {
+ Map<String,SpecParameter<?>> existingToKeep = MutableMap.of();
+ if (existingReferenceParamsToKeep!=null) {
+ for (SpecParameter<?> p: existingReferenceParamsToKeep) {
+ existingToKeep.put(p.getConfigKey().getName(), p);
+ }
+ }
+
+ List<SpecParameter<?>> result = MutableList.<SpecParameter<?>>of();
+ for (SpecParameter<?> p: newParams) {
+ if (p instanceof SpecParameterForInheritance) {
--- End diff --
It's a bit hard to parse this mentally - the `remove` and the `result.add` are both done in both limbs of the `if`. Also the cast `(ConfigKey<?>)` is strange, to force the use of the overload for `resolveWithAncestor(ConfigKey<?> ancestor)`, especially when that has a comment on it that says "// TODO not used yet;"
Would it be clearer to leave the code for now just as
```java
for (SpecParameter<?> p: newParams) {
final SpecParameter<?> existingP = existingToKeep.remove(p.getConfigKey().getName());
if (p instanceof SpecParameterForInheritance) {
// TODO find config keys on the type (not available as parameters)
/* we don't currently do this due to low priority; all it means if there is a config key in java,
* and a user wishes to expose it as a parameter, they have to redeclare everything;
* nothing from the config key in java will be inherited */
p = ((SpecParameterForInheritance<?>)p).resolveWithAncestor(existingP);
}
result.add(p);
}
```
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/462
Seems there may be a pathway by which REST does not populate an entity's `EntityType.configKeys` in this PR, reported by @sjcorbett . Looking in to this now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #462: Inherit config default values
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/462#discussion_r94777251
--- Diff: api/src/main/java/org/apache/brooklyn/api/internal/AbstractBrooklynObjectSpec.java ---
@@ -147,27 +147,28 @@ public SpecT tagsReplace(Iterable<Object> tagsToReplace) {
// it is a CatalogConfig or merely a config key, maybe introducing displayable, or even priority
// (but note part of the reason for CatalogConfig.priority is that java reflection doesn't preserve field order) .
// see also comments on the camp SpecParameterResolver.
+
+ // probably the thing to do is deprecate the ambiguous method in favour of an explicit
@Beta
public SpecT parameters(List<? extends SpecParameter<?>> parameters) {
return parametersReplace(parameters);
}
- /** adds the given parameters */
+ /** adds the given parameters, new ones first so they dominate subsequent ones */
@Beta
public SpecT parametersAdd(List<? extends SpecParameter<?>> parameters) {
// parameters follows immutable pattern, unlike the other fields
Set<SpecParameter<?>> params = MutableSet.<SpecParameter<?>>copyOf(parameters);
Set<SpecParameter<?>> current = MutableSet.<SpecParameter<?>>copyOf(this.parameters);
current.removeAll(params);
- this.parameters = ImmutableList.<SpecParameter<?>>builder()
+ return parametersReplace(ImmutableList.<SpecParameter<?>>builder()
.addAll(params)
.addAll(current)
- .build();
- return self();
+ .build());
}
/** replaces parameters with the given */
@Beta
- public SpecT parametersReplace(List<? extends SpecParameter<?>> parameters) {
+ public SpecT parametersReplace(Collection<? extends SpecParameter<?>> parameters) {
--- End diff --
`parameters` are ordered - that's why the signature requires a `List`. It's also consistent with the other two parameters related methods.
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100810871
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java ---
@@ -170,12 +324,103 @@ public String getConflictResolutionStrategy() {
return conflictResolutionStrategy;
}
+ @Deprecated /** @deprecated since 0.10.0 when it was introduced, prefer {@link #getLocalDefaultResolvesWithAncestorValue()} */
public boolean getUseLocalDefaultValue() {
- return useLocalDefaultValue;
+ return getLocalDefaultResolvesWithAncestorValue();
+ }
+
+ /** see {@link #localDefaultResolvesWithAncestorValue} */
+ public boolean getLocalDefaultResolvesWithAncestorValue() {
+ if (localDefaultResolvesWithAncestorValue==null) {
+ // in case some legacy path is using an improperly deserialized object
+ log.warn("Encountered legacy "+this+" with null localDefaultResolvesWithAncestorValue; transforming", new Throwable("stack trace for legacy "+this));
+ readResolve();
+ }
+ return localDefaultResolvesWithAncestorValue;
+ }
+
+ public boolean getAncestorDefaultInheritable() {
+ if (ancestorDefaultInheritable==null) {
+ log.warn("Encountered legacy "+this+" with null ancestorDefaultInheritable; transforming", new Throwable("stack trace for legacy "+this));
+ readResolve();
+ }
+ return ancestorDefaultInheritable;
+ }
+
+ // standard deserialization method
+ private ConfigInheritance readResolve() {
+ try {
+ if (useLocalDefaultValue!=null) {
+ // move away from useLocalDefaultValue to localDefaultResolvesWithAncestorValue
+
+ Field fNew = getClass().getDeclaredField("localDefaultResolvesWithAncestorValue");
+ fNew.setAccessible(true);
+ Field fOld = getClass().getDeclaredField("useLocalDefaultValue");
+ fOld.setAccessible(true);
+
+ if (fNew.get(this)==null) {
+ fNew.set(this, useLocalDefaultValue);
+ } else {
+ if (!fNew.get(this).equals(useLocalDefaultValue)) {
+ throw new IllegalStateException("Incompatible values detected for "+fOld+" ("+fOld.get(this)+") and "+fNew+" ("+fNew.get(this)+")");
+ }
+ }
+ fOld.set(this, null);
+ }
+
+ if (ancestorDefaultInheritable==null) {
+ Field f = getClass().getDeclaredField("ancestorDefaultInheritable");
+ f.setAccessible(true);
+ f.set(this, true);
+ }
+
+ } catch (Exception e) {
+ throw Exceptions.propagate(e);
+ }
+
+ return returnEquivalentConstant(this);
}
@Override
+ public boolean equals(Object obj) {
--- End diff --
good spot, also added for `DelegatingCI`
---
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 #462: Inherit config default values
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100508376
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java ---
@@ -28,51 +32,188 @@
import org.apache.brooklyn.config.ConfigKey;
import org.apache.brooklyn.config.ConfigValueAtContainer;
import org.apache.brooklyn.util.collections.CollectionMerger;
+import org.apache.brooklyn.util.exceptions.Exceptions;
import org.apache.brooklyn.util.exceptions.ReferenceWithError;
import org.apache.brooklyn.util.guava.Maybe;
+import org.apache.brooklyn.util.text.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+@SuppressWarnings("serial")
public class BasicConfigInheritance implements ConfigInheritance {
+ private static final Logger log = LoggerFactory.getLogger(BasicConfigInheritance.class);
+
private static final long serialVersionUID = -5916548049057961051L;
public static final String CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE = "deep_merge";
public static final String CONFLICT_RESOLUTION_STRATEGY_OVERWRITE = "overwrite";
+ public static abstract class DelegatingConfigInheritance implements ConfigInheritance {
+ protected abstract ConfigInheritance getDelegate();
+
+ @Override @Deprecated public InheritanceMode isInherited(ConfigKey<?> key, Object from, Object to) {
+ return getDelegate().isInherited(key, from, to);
+ }
+ @Override public <TContainer, TValue> boolean isReinheritable(ConfigValueAtContainer<TContainer, TValue> parent, ConfigInheritanceContext context) {
+ return getDelegate().isReinheritable(parent, context);
+ }
+ @Override public <TContainer, TValue> boolean considerParent(ConfigValueAtContainer<TContainer, TValue> local, ConfigValueAtContainer<TContainer, TValue> parent, ConfigInheritanceContext context) {
+ return getDelegate().considerParent(local, parent, context);
+ }
+ @Override
+ public <TContainer, TValue> ReferenceWithError<ConfigValueAtContainer<TContainer, TValue>> resolveWithParent(ConfigValueAtContainer<TContainer, TValue> local, ConfigValueAtContainer<TContainer, TValue> resolvedParent, ConfigInheritanceContext context) {
+ return getDelegate().resolveWithParent(local, resolvedParent, context);
+ }
+ @Override
+ public boolean equals(Object obj) {
+ return super.equals(obj) || getDelegate().equals(obj);
+ }
+
+ // standard deserialization method
+ protected ConfigInheritance readResolve() {
+ return returnEquivalentConstant(this);
+ }
+ }
+
+ private static ConfigInheritance returnEquivalentConstant(ConfigInheritance candidate) {
+ for (ConfigInheritance knownMode: Arrays.asList(
+ NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, NEVER_INHERITED, OVERWRITE, BasicConfigInheritance.DEEP_MERGE)) {
+ if (candidate.equals(knownMode)) return knownMode;
+ }
+ if (candidate.equals(new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, true))) {
+ // ignore the ancestor flag for this mode
+ return NEVER_INHERITED;
+ }
+ return candidate;
+ }
+
+ /*
+ * use of delegate is so that stateless classes can be defined to make the serialization nice,
+ * both the name and hiding the implementation detail (also making it easier for that detail to change);
+ * with aliased type names the field names here could even be aliases for the type names.
+ * (we could alternatively have an "alias-for-final-instance" mode in serialization,
+ * to optimize where we know that a java instance is final.)
+ */
+
+ private static class NotReinherited extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** Indicates that a config key value should not be passed down from a container where it is defined.
* Unlike {@link #NEVER_INHERITED} these values can be passed down if set as anonymous keys at a container
* (ie the container does not expect it) to a container which does expect it, but it will not be passed down further.
* If the inheritor also defines a value the parent's value is ignored irrespective
* (as in {@link #OVERWRITE}; see {@link #NOT_REINHERITED_ELSE_DEEP_MERGE} if merging is desired). */
- public static BasicConfigInheritance NOT_REINHERITED = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_OVERWRITE,false);
+ public static ConfigInheritance NOT_REINHERITED = new NotReinherited();
+
+ private static class NotReinheritedElseDeepMerge extends DelegatingConfigInheritance {
+ final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE, false, true);
+ @Override protected ConfigInheritance getDelegate() { return DELEGATE; }
+ }
/** As {@link #NOT_REINHERITED} but in cases where a value is inherited because a parent did not recognize it,
* if the inheritor also defines a value the two values should be merged. */
- public static BasicConfigInheritance NOT_REINHERITED_ELSE_DEEP_MERGE = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE,false);
- /** Indicates that a key's value should never be inherited, even if defined on a container that does not know the key.
- * Most usages will prefer {@link #NOT_REINHERITED}. */
- public static BasicConfigInheritance NEVER_INHERITED = new BasicConfigInheritance(false,CONFLICT_RESOLUTION_STRATEGY_OVERWRITE,true);
+ public static ConfigInheritance NOT_REINHERITED_ELSE_DEEP_MERGE = new NotReinheritedElseDeepMerge();
--- End diff --
Why not `final` for all these fields? Seems crazy to allow any external code to assign a new value for what each of these "constants" means!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #462: Inherit config default values
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/462#discussion_r94985394
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java ---
@@ -170,12 +324,103 @@ public String getConflictResolutionStrategy() {
return conflictResolutionStrategy;
}
+ @Deprecated /** @deprecated since 0.10.0 when it was introduced, prefer {@link #getLocalDefaultResolvesWithAncestorValue()} */
public boolean getUseLocalDefaultValue() {
- return useLocalDefaultValue;
+ return getLocalDefaultResolvesWithAncestorValue();
+ }
+
+ /** see {@link #localDefaultResolvesWithAncestorValue} */
+ public boolean getLocalDefaultResolvesWithAncestorValue() {
+ if (localDefaultResolvesWithAncestorValue==null) {
+ // in case some legacy path is using an improperly deserialized object
+ log.warn("Encountered legacy "+this+" with null localDefaultResolvesWithAncestorValue; transforming", new Throwable("stack trace for legacy "+this));
+ readResolve();
+ }
+ return localDefaultResolvesWithAncestorValue;
+ }
+
+ public boolean getAncestorDefaultInheritable() {
+ if (ancestorDefaultInheritable==null) {
+ log.warn("Encountered legacy "+this+" with null ancestorDefaultInheritable; transforming", new Throwable("stack trace for legacy "+this));
+ readResolve();
+ }
+ return ancestorDefaultInheritable;
+ }
+
+ // standard deserialization method
+ private ConfigInheritance readResolve() {
+ try {
+ if (useLocalDefaultValue!=null) {
+ // move away from useLocalDefaultValue to localDefaultResolvesWithAncestorValue
+
+ Field fNew = getClass().getDeclaredField("localDefaultResolvesWithAncestorValue");
+ fNew.setAccessible(true);
+ Field fOld = getClass().getDeclaredField("useLocalDefaultValue");
+ fOld.setAccessible(true);
+
+ if (fNew.get(this)==null) {
+ fNew.set(this, useLocalDefaultValue);
+ } else {
+ if (!fNew.get(this).equals(useLocalDefaultValue)) {
+ throw new IllegalStateException("Incompatible values detected for "+fOld+" ("+fOld.get(this)+") and "+fNew+" ("+fNew.get(this)+")");
+ }
+ }
+ fOld.set(this, null);
+ }
+
+ if (ancestorDefaultInheritable==null) {
+ Field f = getClass().getDeclaredField("ancestorDefaultInheritable");
+ f.setAccessible(true);
+ f.set(this, true);
+ }
--- End diff --
Could you add a comment describing why the use of reflection - can't figure it out.
---
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 #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100937270
--- Diff: camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/SpecParameterUnwrappingTest.java ---
@@ -309,7 +298,7 @@ public void testCatalogConfigOverridesParameters() {
CatalogItem<?, ?> item = catalog.getCatalogItem(SYMBOLIC_NAME, TEST_VERSION);
AbstractBrooklynObjectSpec<?,?> spec = catalog.peekSpec(item);
List<SpecParameter<?>> params = spec.getParameters();
- assertEquals(params.size(), 3);
+ assertEquals(params.size(), NUM_ENTITY_DEFAULT_CONFIG_KEYS + 2);
--- End diff --
if we deployed i think it would be; here we're just doing `peekSpec` so it doesn't have the app config keys, it just has `NUM_ENTITY_DEFAULT_CONFIG_KEYS + ConfigEntityForTest.NUM_CONFIG_KEYS_DEFINED_HERE + NUM_CONFIG_KEYS_FROM_TEST_BLUEPRINT` = `1+1+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 #462: Inherit config default values
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100370235
--- Diff: api/src/main/java/org/apache/brooklyn/api/internal/AbstractBrooklynObjectSpec.java ---
@@ -147,27 +147,28 @@ public SpecT tagsReplace(Iterable<Object> tagsToReplace) {
// it is a CatalogConfig or merely a config key, maybe introducing displayable, or even priority
// (but note part of the reason for CatalogConfig.priority is that java reflection doesn't preserve field order) .
// see also comments on the camp SpecParameterResolver.
+
+ // probably the thing to do is deprecate the ambiguous method in favour of an explicit
@Beta
public SpecT parameters(List<? extends SpecParameter<?>> parameters) {
return parametersReplace(parameters);
}
- /** adds the given parameters */
+ /** adds the given parameters, new ones first so they dominate subsequent ones */
@Beta
public SpecT parametersAdd(List<? extends SpecParameter<?>> parameters) {
// parameters follows immutable pattern, unlike the other fields
Set<SpecParameter<?>> params = MutableSet.<SpecParameter<?>>copyOf(parameters);
Set<SpecParameter<?>> current = MutableSet.<SpecParameter<?>>copyOf(this.parameters);
current.removeAll(params);
- this.parameters = ImmutableList.<SpecParameter<?>>builder()
+ return parametersReplace(ImmutableList.<SpecParameter<?>>builder()
.addAll(params)
.addAll(current)
- .build();
- return self();
+ .build());
}
/** replaces parameters with the given */
@Beta
- public SpecT parametersReplace(List<? extends SpecParameter<?>> parameters) {
+ public SpecT parametersReplace(Collection<? extends SpecParameter<?>> parameters) {
--- End diff --
I'd be fine with this being the even more general `parametersReplace(Iterable<? extends SpecParameter<?>> parameters)` (which implies ordered, even though it's a super-type of collection!)
Agree with the comment about consistency - all three methods should take the same type for their parameter.
---
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 #462: Inherit config default values
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/462#discussion_r94962666
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void addParameters(AbstractBrooklynObjectSpec<?, ?> spec, List<? e
spec.parametersAdd(BasicSpecParameter.fromSpec(loader.getManagementContext(), spec));
}
if (explicitParams.size() > 0) {
- spec.parametersAdd(explicitParams);
+ spec.parametersReplace(resolveParameters(explicitParams, spec.getParameters(), spec));
+ }
+ }
+
+ /** merge parameters against other parameters and known and type-inherited config keys */
+ static Collection<SpecParameter<?>> resolveParameters(Collection<? extends SpecParameter<?>> newParams, Collection<? extends SpecParameter<?>> existingReferenceParamsToKeep, AbstractBrooklynObjectSpec<?,?> spec) {
+ Map<String,SpecParameter<?>> existingToKeep = MutableMap.of();
+ if (existingReferenceParamsToKeep!=null) {
+ for (SpecParameter<?> p: existingReferenceParamsToKeep) {
+ existingToKeep.put(p.getConfigKey().getName(), p);
+ }
+ }
+
+ List<SpecParameter<?>> result = MutableList.<SpecParameter<?>>of();
+ for (SpecParameter<?> p: newParams) {
+ if (p instanceof SpecParameterForInheritance) {
+ SpecParameter<?> existingP = existingToKeep.remove(p.getConfigKey().getName());
+ if (existingP!=null) {
+ p = ((SpecParameterForInheritance<?>)p).resolveWithAncestor(existingP);
+ } else {
+ // TODO find config keys on the type (not available as parameters)
+ /* we don't currently do this due to low priority; all it means if there is a config key in java,
+ * and a user wishes to expose it as a parameter, they have to redeclare everything;
+ * nothing from the config key in java will be inherited */
+ p = ((SpecParameterForInheritance<?>)p).resolveWithAncestor((ConfigKey<?>)null);
+ }
+ result.add(p);
+ } else {
+ existingToKeep.remove(p.getConfigKey().getName());
+ result.add(p);
+ }
+ }
+ result.addAll(existingToKeep.values());
+ return result;
+ }
+
+ /** instance of {@link SpecParameter} which includes information about what fields are explicitly set,
+ * for use with a subsequent merge */
+ @SuppressWarnings("serial")
+ @Beta
+ static class SpecParameterForInheritance<T> extends BasicSpecParameter<T> {
+
+ private final boolean hasType, hasLabelSet, hasPinnedSet, hasDefaultValue, hasConstraints, hasRuntimeInheritance, hasTypeInheritance;
+
+ private <SensorType> SpecParameterForInheritance(String label, Boolean pinned, ConfigKey<T> config, AttributeSensor<SensorType> sensor,
+ boolean hasType, boolean hasDefaultValue, boolean hasConstraints, boolean hasRuntimeInheritance, boolean hasTypeInheritance) {
+ super(Preconditions.checkNotNull(label!=null ? label : config.getName(), "label or config name must be set"),
+ pinned==null ? true : pinned, config, sensor);
+ this.hasType = hasType;
+ this.hasLabelSet = label!=null;
+ this.hasPinnedSet = pinned!=null;
+ this.hasDefaultValue = hasDefaultValue;
+ this.hasConstraints = hasConstraints;
+ this.hasRuntimeInheritance = hasRuntimeInheritance;
+ this.hasTypeInheritance = hasTypeInheritance;
+ }
+
+ /** used if yaml specifies a parameter, but possibly incomplete, and a spec supertype has a parameter */
+ SpecParameter<?> resolveWithAncestor(SpecParameter<?> ancestor) {
+ if (ancestor==null) return new BasicSpecParameter<>(getLabel(), isPinned(), getConfigKey(), getSensor());
+
+ return new BasicSpecParameter<>(
+ hasLabelSet ? getLabel() : ancestor.getLabel(),
+ hasPinnedSet ? isPinned() : ancestor.isPinned(),
+ resolveWithAncestorConfigKey(ancestor.getConfigKey()),
+ hasType ? getSensor() : ancestor.getSensor());
+ }
+
+ /** as {@link #resolveWithAncestor(SpecParameter)} but where the param redefines/extends a config key coming from a java supertype, rather than a parameter */
+ // TODO not used yet; see calls to the other resolveWithAncestor method,
+ // and see BrooklynComponentTemplateResolver.findAllConfigKeys;
+ // also note whilst it is easiest to do this here, logically it is messy,
+ // and arguably it should be done when converting the spec to an instance
+ SpecParameter<?> resolveWithAncestor(ConfigKey<?> ancestor) {
+ if (ancestor==null) return new BasicSpecParameter<>(getLabel(), isPinned(), getConfigKey(), getSensor());
+
+ // TODO probably want to do this (but it could get expensive!)
+ // Set<Class<?>> types = MutableSet.<Class<?>>builder()
+ // .add(spec.getImplementation())
+ // .add(spec.getType())
+ // .addAll(spec.getAdditionalInterfaces())
+ // .remove(null)
+ // .build();
+ // // order above is important, respected below to take the first one defined
+ // MutableMap<String, ConfigKey<?>> result = MutableMap.copyOf(FlagUtils.findAllConfigKeys(null, types));
--- End diff --
+1, (limited) caching will help here
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100810977
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BrooklynObjectInternal.java ---
@@ -152,8 +152,8 @@
/** This is currently the only way to get some rolled up collections and raw,
* and also to test for the presence of a value (without any default).
* As more accessors are added callers may be asked to migrate.
- * Callers may also consider using {@link #findKeys(com.google.common.base.Predicate)}
- * if that isn't too inefficient. */
+ * Callers may also consider using {@link #findKeysDeprecated(com.google.common.base.Predicate)}
--- End diff --
yes - TY
---
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 #462: Inherit config default values
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r94416378
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java ---
@@ -170,12 +324,103 @@ public String getConflictResolutionStrategy() {
return conflictResolutionStrategy;
}
+ @Deprecated /** @deprecated since 0.10.0 when it was introduced, prefer {@link #getLocalDefaultResolvesWithAncestorValue()} */
--- End diff --
since 0.11.0 now, and in a number of other places in the PR
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #462: Inherit config default values
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r100796966
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -227,13 +234,30 @@ public String toString() {
String label = (String)inputDef.get("label");
String description = (String)inputDef.get("description");
String type = (String)inputDef.get("type");
- Object defaultValue = inputDef.get("default");
Boolean pinned = (Boolean)inputDef.get("pinned");
+ boolean hasDefaultValue = inputDef.containsKey("default");
+ Object defaultValue = inputDef.get("default");
if (specialFlagTransformer != null) {
defaultValue = specialFlagTransformer.apply(defaultValue);
}
+ boolean hasConstraints = inputDef.containsKey("constraints");
Predicate<?> constraint = parseConstraints(inputDef.get("constraints"), loader);
- ConfigInheritance parentInheritance = parseInheritance(inputDef.get("inheritance.parent"), loader);
+
+ ConfigInheritance runtimeInheritance;
+ boolean hasRuntimeInheritance;
+ if (inputDef.containsKey("inheritance.runtime")) {
+ hasRuntimeInheritance = true;
+ runtimeInheritance = parseInheritance(inputDef.get("inheritance.runtime"), loader);
+ } else if (inputDef.containsKey("inheritance.parent")) {
+ // this alias will be deprecated
+ hasRuntimeInheritance = true;
+ runtimeInheritance = parseInheritance(inputDef.get("inheritance.parent"), loader);
+ } else {
+ hasRuntimeInheritance = false;
+ runtimeInheritance = parseInheritance(null, loader);
--- End diff --
done, thx
---
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.
---