address most PR comments still to do: * version refs in deprecation notes * geomacy's test case and parameter order * aled's tests * several comments from neykov re SpecParameterUnwrappingTest.java
Project: http://git-wip-us.apache.org/repos/asf/brooklyn-server/repo Commit: http://git-wip-us.apache.org/repos/asf/brooklyn-server/commit/bb954698 Tree: http://git-wip-us.apache.org/repos/asf/brooklyn-server/tree/bb954698 Diff: http://git-wip-us.apache.org/repos/asf/brooklyn-server/diff/bb954698 Branch: refs/heads/master Commit: bb95469820daf6bcb22b6fe80e454ebd9506a1f0 Parents: 9fc308c Author: Alex Heneveld <alex.henev...@cloudsoftcorp.com> Authored: Mon Feb 13 14:59:49 2017 +0000 Committer: Alex Heneveld <alex.henev...@cloudsoftcorp.com> Committed: Mon Feb 13 14:59:49 2017 +0000 ---------------------------------------------------------------------- .../apache/brooklyn/api/objs/Configurable.java | 2 +- .../BrooklynComponentTemplateResolver.java | 2 +- .../core/config/BasicConfigInheritance.java | 89 +++++++++++++++----- .../config/internal/AbstractConfigMapImpl.java | 62 +++++++------- .../core/mgmt/EntityManagementUtils.java | 3 +- .../brooklyn/core/objs/BasicSpecParameter.java | 39 +++++---- .../core/objs/BrooklynObjectInternal.java | 2 +- .../mgmt/persist/XmlMementoSerializerTest.java | 2 +- .../org/apache/brooklyn/util/guava/Maybe.java | 19 ++++- 9 files changed, 143 insertions(+), 77 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/bb954698/api/src/main/java/org/apache/brooklyn/api/objs/Configurable.java ---------------------------------------------------------------------- diff --git a/api/src/main/java/org/apache/brooklyn/api/objs/Configurable.java b/api/src/main/java/org/apache/brooklyn/api/objs/Configurable.java index 4d1d2f4..e0226b0 100644 --- a/api/src/main/java/org/apache/brooklyn/api/objs/Configurable.java +++ b/api/src/main/java/org/apache/brooklyn/api/objs/Configurable.java @@ -103,7 +103,7 @@ public interface Configurable { @Deprecated Set<ConfigKey<?>> findKeys(Predicate<? super ConfigKey<?>> filter); - /** see {@link ConfigMap#findKeysDelcared(Predicate)} */ + /** see {@link ConfigMap#findKeysDeclared(Predicate)} */ public Set<ConfigKey<?>> findKeysDeclared(Predicate<? super ConfigKey<?>> filter); /** see {@link ConfigMap#findKeysPresent(Predicate)} */ http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/bb954698/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java ---------------------------------------------------------------------- diff --git a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java index 4a0e1ff..adeb7a7 100644 --- a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java +++ b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java @@ -409,7 +409,7 @@ public class BrooklynComponentTemplateResolver { // // I plan to propose a change on dev@brooklyn, to replace `@SetFromFlag`! - // need to de-dupe? (can't use Set bc FCKVR doesn't impl equals/hashcode) + // need to de-dupe? (can't use Set bc FCKAVR doesn't impl equals/hashcode) // TODO merge *bagFlags* with existing spec params, merge yaml w yaml parent params elsewhere List<FlagConfigKeyAndValueRecord> allKeys = MutableList.of(); allKeys.addAll(FlagUtils.findAllParameterConfigKeys(spec.getParameters(), bagFlags)); http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/bb954698/core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java b/core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java index d63d330..4d66293 100644 --- a/core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java +++ b/core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java @@ -49,6 +49,9 @@ public class BasicConfigInheritance implements ConfigInheritance { public static final String CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE = "deep_merge"; public static final String CONFLICT_RESOLUTION_STRATEGY_OVERWRITE = "overwrite"; + /** This class allows us to have constant instances of {@link ConfigInheritance} + * which have no internal fields (ie they don't extend {@link BasicConfigInheritance}) + * so are pretty on serialization, but otherwise act identically. */ public static abstract class DelegatingConfigInheritance implements ConfigInheritance { protected abstract ConfigInheritance getDelegate(); @@ -69,20 +72,24 @@ public class BasicConfigInheritance implements ConfigInheritance { public boolean equals(Object obj) { return super.equals(obj) || getDelegate().equals(obj); } - + + @Override + public int hashCode() { + return getDelegate().hashCode(); + } + // 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 + if (candidate.equals(NEVER_INHERITED_OLD)) { return NEVER_INHERITED; } return candidate; @@ -105,7 +112,7 @@ public class BasicConfigInheritance implements ConfigInheritance { * (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 ConfigInheritance NOT_REINHERITED = new NotReinherited(); + public static final 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); @@ -113,7 +120,7 @@ public class BasicConfigInheritance implements ConfigInheritance { } /** 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 ConfigInheritance NOT_REINHERITED_ELSE_DEEP_MERGE = new NotReinheritedElseDeepMerge(); + public static final 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); @@ -121,7 +128,11 @@ public class BasicConfigInheritance implements ConfigInheritance { } /** 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(); + public static final ConfigInheritance NEVER_INHERITED = new NeverInherited(); + + /** In case we deserialize an old copy; the last arg (ancestor inherit default) is irrelevant + * so accept this as a synonym for {@link #NEVER_INHERITED}. */ + private static final ConfigInheritance NEVER_INHERITED_OLD = new BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, true); private static class Overwrite extends DelegatingConfigInheritance { final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(true, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, true); @@ -129,7 +140,7 @@ public class BasicConfigInheritance implements ConfigInheritance { } /** 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 ConfigInheritance OVERWRITE = new Overwrite(); + public static final ConfigInheritance OVERWRITE = new Overwrite(); private static class DeepMerge extends DelegatingConfigInheritance { final static BasicConfigInheritance DELEGATE = new BasicConfigInheritance(true, CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE, false, true); @@ -138,7 +149,7 @@ public class BasicConfigInheritance implements ConfigInheritance { /** 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 mergeable behaviour is undefined * (and often the descendant's value will simply overwrite). */ - public static ConfigInheritance DEEP_MERGE = new DeepMerge(); + public static final ConfigInheritance DEEP_MERGE = new DeepMerge(); // support conversion from these legacy fields @SuppressWarnings("deprecation") @@ -169,17 +180,27 @@ public class BasicConfigInheritance implements ConfigInheritance { /** 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 or a different string if they provide a custom implementaton + * subclasses may pass null or a different string if they provide a custom implementation * of {@link #resolveWithParentCustomStrategy(ConfigValueAtContainer, ConfigValueAtContainer, org.apache.brooklyn.config.ConfigInheritance.ConfigInheritanceContext)} */ @Nullable protected final String conflictResolutionStrategy; /** @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 + * can use true with {@link #CONFLICT_RESOLUTION_STRATEGY_OVERWRITE} to mean don't inherit, + * or true with {@link #CONFLICT_RESOLUTION_STRATEGY_DEEP_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. + * in most cases this is false, meaning a default value is ignored if the parent has a value, + * but it can be used when a key supplies a default which should conflict-resolve with an ancestor value: + * a trivial example is when not reinheriting, a default should conflict-resolve (overwriting) an explicit ancestor value. + * more interesting potentially, this could indicate + * that a default value is being introduced which should be merged/combined with ancestors; + * we don't use this (config inheritance is complex enough and we don't have a compelling use case + * to expose more complexity to users) but having this as a concept, and the related + * {@link #ancestorDefaultInheritable} specifying (in this case) whether a local default should + * resolve/merge/combine with ancestor defaults in addition to ancestor explicit values, + * means the logic in this refers to the right control dimensions rather than taking shortcuts. * <p> * null should not be used. a boxed object is taken (as opposed to a primitive boolean) only in order to support migration. */ @@ -187,7 +208,7 @@ public class BasicConfigInheritance implements ConfigInheritance { protected final Boolean localDefaultResolvesWithAncestorValue; /** 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); + * 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 @@ -197,6 +218,21 @@ public class BasicConfigInheritance implements ConfigInheritance { * 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> + * as currently we only really use {@link #localDefaultResolvesWithAncestorValue} true when we + * wish to block reinheritance, this is false in that case, + * and true in all other cases (where we wish to have ancestor inheritance, but only if there + * is no local value that should trump); + * however logically there are other possibilities, such as a local default expecting to + * merge/combine with an ancestor value, in which case we'd probably want this to be true + * to indicate the local default should combine with the ancestor default, + * but it could also make sense for this to be false, meaning the local default would + * merge with an explicit ancestor value but not with an ancestor default + * (see javadoc on {@link #localDefaultResolvesWithAncestorValue} for more info). + * <p> + * it gets complex and we've tried to simplify the config modes that we actually use, + * but have made efforts in the code to account for the different logical possibilities, + * hence the complexity of this. + * <p> * null should not be used. a boxed object is taken (as opposed to a primitive boolean) only in order to support migration. */ @Nonnull @@ -232,6 +268,8 @@ public class BasicConfigInheritance implements ConfigInheritance { private boolean isSameRootInstanceAs(ConfigInheritance other) { if (other==null) return false; if (this==other) return true; + // we should consider the argument the same if it is a delegate to us, + // e.g. for when this method is invoked by the delegate comparing against its definition if (other instanceof DelegatingConfigInheritance) return isSameRootInstanceAs( ((DelegatingConfigInheritance)other).getDelegate() ); return false; } @@ -350,6 +388,7 @@ public class BasicConfigInheritance implements ConfigInheritance { // standard deserialization method private ConfigInheritance readResolve() { try { + // uses reflection because fields are declared final if (useLocalDefaultValue!=null) { // move away from useLocalDefaultValue to localDefaultResolvesWithAncestorValue @@ -382,19 +421,29 @@ public class BasicConfigInheritance implements ConfigInheritance { } @Override + public int hashCode() { + return Objects.hash(conflictResolutionStrategy, isReinherited, localDefaultResolvesWithAncestorValue, ancestorDefaultInheritable); + } + + @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)) { - BasicConfigInheritance b = (BasicConfigInheritance)obj; - return Objects.equals(conflictResolutionStrategy, b.conflictResolutionStrategy) && - Objects.equals(isReinherited, b.isReinherited) && - Objects.equals(getLocalDefaultResolvesWithAncestorValue(), b.getLocalDefaultResolvesWithAncestorValue()) && - Objects.equals(getAncestorDefaultInheritable(), b.getAncestorDefaultInheritable()); + if (obj instanceof DelegatingConfigInheritance) { + return equals( ((DelegatingConfigInheritance)obj).getDelegate() ); + } + if (obj instanceof BasicConfigInheritance) { + return equalsAfterResolvingDelegate((BasicConfigInheritance)obj); } return false; } + protected boolean equalsAfterResolvingDelegate(BasicConfigInheritance b) { + return (Objects.equals(conflictResolutionStrategy, b.conflictResolutionStrategy) && + Objects.equals(isReinherited, b.isReinherited) && + Objects.equals(getLocalDefaultResolvesWithAncestorValue(), b.getLocalDefaultResolvesWithAncestorValue()) && + Objects.equals(getAncestorDefaultInheritable(), b.getAncestorDefaultInheritable())); + } + @Override public String toString() { return super.toString()+"[reinherit="+isReinherited()+"; strategy="+getConflictResolutionStrategy()+"; localDefaultResolvesWithAncestor="+localDefaultResolvesWithAncestorValue+"]"; http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/bb954698/core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java b/core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java index a7de989..010b91b 100644 --- a/core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java +++ b/core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java @@ -360,35 +360,35 @@ public abstract class AbstractConfigMapImpl<TContainer extends BrooklynObject> i 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); - } - }; - AncestorContainerAndKeyValueIterator<TContainer, T> ckvi = new AncestorContainerAndKeyValueIterator<TContainer,T>( - getContainer(), keyFn, lookupFn, coerceFn, parentFn); - - return ConfigInheritances.resolveInheriting( - getContainer(), ownKey, coerceFn.apply(lookupFn.apply(getContainer())), defaultValue, - ckvi, InheritanceContext.RUNTIME_MANAGEMENT, getDefaultRuntimeInheritance()); - - } else { - String message = "Config key "+ownKey+" of "+getBrooklynObject()+" is not a ConfigKeySelfExtracting; cannot retrieve value; returning default"; - LOG.warn(message); - return ReferenceWithError.newInstanceThrowingError(new BasicConfigValueAtContainer<TContainer,T>(getContainer(), ownKey, null, false, - defaultValue), - new IllegalStateException(message)); + 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); + } + }; + AncestorContainerAndKeyValueIterator<TContainer, T> ckvi = new AncestorContainerAndKeyValueIterator<TContainer,T>( + getContainer(), keyFn, lookupFn, coerceFn, parentFn); + + return ConfigInheritances.resolveInheriting( + getContainer(), ownKey, coerceFn.apply(lookupFn.apply(getContainer())), defaultValue, + ckvi, InheritanceContext.RUNTIME_MANAGEMENT, getDefaultRuntimeInheritance()); + + } else { + String message = "Config key "+ownKey+" of "+getBrooklynObject()+" is not a ConfigKeySelfExtracting; cannot retrieve value; returning default"; + LOG.warn(message); + return ReferenceWithError.newInstanceThrowingError(new BasicConfigValueAtContainer<TContainer,T>(getContainer(), ownKey, null, false, + defaultValue), + new IllegalStateException(message)); + } } @Override @@ -436,10 +436,10 @@ public abstract class AbstractConfigMapImpl<TContainer extends BrooklynObject> i @Override public Set<ConfigKey<?>> findKeysPresent(Predicate<? super ConfigKey<?>> filter) { - return findKeys(filter, KeyFindingMode.PRESENT_BUT_RESOLVED); + return findKeys(filter, KeyFindingMode.PRESENT_AND_RESOLVED); } - protected enum KeyFindingMode { DECLARED_OR_PRESENT, PRESENT_BUT_RESOLVED, PRESENT_NOT_RESOLVED } + protected enum KeyFindingMode { DECLARED_OR_PRESENT, PRESENT_AND_RESOLVED, PRESENT_NOT_RESOLVED } @SuppressWarnings("deprecation") protected Set<ConfigKey<?>> findKeys(Predicate<? super ConfigKey<?>> filter, KeyFindingMode mode) { @@ -461,7 +461,7 @@ public abstract class AbstractConfigMapImpl<TContainer extends BrooklynObject> i if (getParent()!=null) { switch (mode) { case DECLARED_OR_PRESENT: result.addAll( getParentInternal().config().getInternalConfigMap().findKeysDeclared(filter) ); break; - case PRESENT_BUT_RESOLVED: result.addAll( getParentInternal().config().getInternalConfigMap().findKeysPresent(filter) ); break; + case PRESENT_AND_RESOLVED: result.addAll( getParentInternal().config().getInternalConfigMap().findKeysPresent(filter) ); break; case PRESENT_NOT_RESOLVED: result.addAll( getParentInternal().config().getInternalConfigMap().findKeys(filter) ); break; default: throw new IllegalStateException("Unsupported key finding mode: "+mode); http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/bb954698/core/src/main/java/org/apache/brooklyn/core/mgmt/EntityManagementUtils.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/EntityManagementUtils.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/EntityManagementUtils.java index 0a8b580..cfee58f 100644 --- a/core/src/main/java/org/apache/brooklyn/core/mgmt/EntityManagementUtils.java +++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/EntityManagementUtils.java @@ -318,8 +318,7 @@ public class EntityManagementUtils { // prevent merge only if a location is defined at both levels ((spec.getLocations().isEmpty() && spec.getLocationSpecs().isEmpty()) || (Iterables.getOnlyElement(spec.getChildren()).getLocations().isEmpty()) && Iterables.getOnlyElement(spec.getChildren()).getLocationSpecs().isEmpty()) - // TODO what should we do with parameters? currently clobbers due to EntitySpec.parameters(...) behaviour. -// && (spec.getParameters().isEmpty() || Iterables.getOnlyElement(spec.getChildren()).getParameters().isEmpty()) + // parameters are collapsed on merge so don't need to be considered here ; } /** @deprecated since 0.9.0 use {@link #canUnwrapEntity(EntitySpec)} */ @Deprecated http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/bb954698/core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java b/core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java index 5344bc1..aa5150e 100644 --- a/core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java +++ b/core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java @@ -52,9 +52,12 @@ import org.apache.brooklyn.util.collections.MutableMap; import org.apache.brooklyn.util.guava.Maybe; import org.apache.brooklyn.util.text.StringPredicates; import org.apache.brooklyn.util.time.Duration; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import com.google.common.annotations.Beta; import com.google.common.base.Function; +import com.google.common.base.MoreObjects; import com.google.common.base.Objects; import com.google.common.base.Preconditions; import com.google.common.base.Predicate; @@ -67,6 +70,8 @@ import com.google.common.reflect.TypeToken; public class BasicSpecParameter<T> implements SpecParameter<T>{ private static final long serialVersionUID = -4728186276307619778L; + private static final Logger log = LoggerFactory.getLogger(BasicSpecParameter.class); + private final String label; /** pinning may become a priority or other more expansive indicator */ @@ -142,7 +147,7 @@ public class BasicSpecParameter<T> implements SpecParameter<T>{ @Override public String toString() { - return Objects.toStringHelper(this) + return MoreObjects.toStringHelper(this) .add("label", label) .add("pinned", pinned) .add("type", configKey) @@ -249,12 +254,12 @@ public class BasicSpecParameter<T> implements SpecParameter<T>{ hasRuntimeInheritance = true; runtimeInheritance = parseInheritance(inputDef.get("inheritance.runtime"), loader); } else if (inputDef.containsKey("inheritance.parent")) { - // this alias will be deprecated + log.warn("Using deprecated key 'inheritance.parent' for "+inputDef+"; replace with 'inheritance.runtime'"); hasRuntimeInheritance = true; runtimeInheritance = parseInheritance(inputDef.get("inheritance.parent"), loader); } else { hasRuntimeInheritance = false; - runtimeInheritance = parseInheritance(null, loader); + runtimeInheritance = null; } boolean hasTypeInheritance = inputDef.containsKey("inheritance.type"); @@ -421,8 +426,8 @@ public class BasicSpecParameter<T> implements SpecParameter<T>{ /** * Adds the given list of {@link SpecParameter parameters} to the provided - * {@link AbstractBrooklynObjectSpec spec} or generates a list from the - * spec if the provided list is empty. + * {@link AbstractBrooklynObjectSpec spec}, and if spec has no parameters it + * also generates a list from the spec * * @see EntitySpec#parameters(List) */ @@ -445,24 +450,24 @@ public class BasicSpecParameter<T> implements SpecParameter<T>{ } List<SpecParameter<?>> result = MutableList.<SpecParameter<?>>of(); + for (SpecParameter<?> p: newParams) { + final SpecParameter<?> existingP = existingToKeep.remove(p.getConfigKey().getName()); 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) + ConfigKey<?> configKeyExtendedByThisParameter = null; + // TODO find any matching config key declared on the type /* 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); + * none of the fields from the config key in java will be inherited */ + p = ((SpecParameterForInheritance<?>)p).resolveWithAncestor(configKeyExtendedByThisParameter); } - result.add(p); - } else { - existingToKeep.remove(p.getConfigKey().getName()); - result.add(p); } + result.add(p); } + result.addAll(existingToKeep.values()); return result; } @@ -494,9 +499,9 @@ public class BasicSpecParameter<T> implements SpecParameter<T>{ return new BasicSpecParameter<>( hasLabelSet ? getLabel() : ancestor.getLabel(), - hasPinnedSet ? isPinned() : ancestor.isPinned(), - resolveWithAncestorConfigKey(ancestor.getConfigKey()), - hasType ? getSensor() : ancestor.getSensor()); + 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 */ @@ -507,7 +512,7 @@ public class BasicSpecParameter<T> implements SpecParameter<T>{ 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!) + // TODO probably want to do this (but it could get expensive! - limited caching could help) // Set<Class<?>> types = MutableSet.<Class<?>>builder() // .add(spec.getImplementation()) // .add(spec.getType()) http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/bb954698/core/src/main/java/org/apache/brooklyn/core/objs/BrooklynObjectInternal.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/objs/BrooklynObjectInternal.java b/core/src/main/java/org/apache/brooklyn/core/objs/BrooklynObjectInternal.java index 49b637a..86daf1f 100644 --- a/core/src/main/java/org/apache/brooklyn/core/objs/BrooklynObjectInternal.java +++ b/core/src/main/java/org/apache/brooklyn/core/objs/BrooklynObjectInternal.java @@ -153,7 +153,7 @@ public interface BrooklynObjectInternal extends BrooklynObject, Rebindable { /** 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 #findKeysDeprecated(com.google.common.base.Predicate)} + * Callers may also consider using {@link #findKeysDeclared(com.google.common.base.Predicate)} * although efficiency should be considered (this gives direct access whereas that does lookups and copies). */ @Beta // TODO provide more accessors and deprecate this ConfigMapWithInheritance<? extends BrooklynObject> getInternalConfigMap(); http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/bb954698/core/src/test/java/org/apache/brooklyn/core/mgmt/persist/XmlMementoSerializerTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/brooklyn/core/mgmt/persist/XmlMementoSerializerTest.java b/core/src/test/java/org/apache/brooklyn/core/mgmt/persist/XmlMementoSerializerTest.java index 68aa938..a2ffb16 100644 --- a/core/src/test/java/org/apache/brooklyn/core/mgmt/persist/XmlMementoSerializerTest.java +++ b/core/src/test/java/org/apache/brooklyn/core/mgmt/persist/XmlMementoSerializerTest.java @@ -799,7 +799,7 @@ public class XmlMementoSerializerTest { public void testConfigInheritanceVals() throws Exception { ConfigInheritance val = BasicConfigInheritance.NEVER_INHERITED; - ConfigInheritance newVal = assertSerializeAndDeserialize(val); // TODO this line fails + ConfigInheritance newVal = assertSerializeAndDeserialize(val); Assert.assertSame(val, newVal); } http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/bb954698/utils/common/src/main/java/org/apache/brooklyn/util/guava/Maybe.java ---------------------------------------------------------------------- diff --git a/utils/common/src/main/java/org/apache/brooklyn/util/guava/Maybe.java b/utils/common/src/main/java/org/apache/brooklyn/util/guava/Maybe.java index 5766e6b..2dfe627 100644 --- a/utils/common/src/main/java/org/apache/brooklyn/util/guava/Maybe.java +++ b/utils/common/src/main/java/org/apache/brooklyn/util/guava/Maybe.java @@ -459,14 +459,27 @@ public abstract class Maybe<T> implements Serializable, Supplier<T> { return Objects.hashCode(31, get()); } + /** Two {@link Maybe} instances are equal if both present wrapping the same value, + * or if both are absent for any reason. + * <p> + * Specifically, in cases of absences, the reasons for absence are not compared. + * This could be revisited if there is compelling reason to do so, but in the main + * the cause of an absence is interesting for giving information to the user. + * Note this is different to the behaviour of {@link Optional} which says absences + * are only equal if they are the same instance. + */ @Override public boolean equals(Object obj) { if (!(obj instanceof Maybe)) return false; Maybe<?> other = (Maybe<?>)obj; - if (!isPresent()) - return !other.isPresent(); - if (!other.isPresent()) + if (!isPresent()) { + if (other.isPresent()) return false; + // could compare exceptions; see javadoc + return true; + } + if (!other.isPresent()) { return false; + } return Objects.equal(get(), other.get()); }