brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From aledsage <...@git.apache.org>
Subject [GitHub] brooklyn-server pull request #173: BROOKLYN-286: merge config keys map value...
Date Thu, 02 Jun 2016 13:14:41 GMT
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/173#discussion_r65536289
  
    --- Diff: core/src/main/java/org/apache/brooklyn/core/entity/internal/EntityConfigMap.java
---
    @@ -111,47 +110,107 @@ public EntityConfigMap(AbstractEntity entity, Map<ConfigKey<?>,
Object> storage)
             // Don't use groovy truth: if the set value is e.g. 0, then would ignore set
value and return default!
             if (ownKey instanceof ConfigKeySelfExtracting) {
                 Object rawval = ownConfig.get(key);
    -            T result = null;
    -            boolean complete = false;
    -            if (((ConfigKeySelfExtracting<T>)ownKey).isSet(ownConfig)) {
    -                ExecutionContext exec = entity.getExecutionContext();
    -                result = ((ConfigKeySelfExtracting<T>)ownKey).extractValue(ownConfig,
exec);
    -                complete = true;
    -            } else if (isInherited(ownKey, inheritance) && 
    -                    ((ConfigKeySelfExtracting<T>)ownKey).isSet(inheritedConfig))
{
    -                ExecutionContext exec = entity.getExecutionContext();
    -                result = ((ConfigKeySelfExtracting<T>)ownKey).extractValue(inheritedConfig,
exec);
    -                complete = true;
    -            } else if (localConfigBag.containsKey(ownKey)) {
    -                // TODO configBag.get doesn't handle tasks/attributeWhenReady - it only
uses TypeCoercions
    -                result = localConfigBag.get(ownKey);
    -                complete = true;
    -            } else if (isInherited(ownKey, inheritance) && 
    -                    inheritedConfigBag.containsKey(ownKey)) {
    -                result = inheritedConfigBag.get(ownKey);
    -                complete = true;
    -            }
    +            Maybe<T> result = getConfigImpl((ConfigKeySelfExtracting<T>)ownKey,
parentInheritanceMode);
     
                 if (rawval instanceof Task) {
                     entity.getManagementSupport().getEntityChangeListener().onConfigChanged(key);
                 }
    -            if (complete) {
    -                return result;
    +            if (result.isPresent()) {
    +                return result.get();
                 }
             } else {
                 LOG.warn("Config key {} of {} is not a ConfigKeySelfExtracting; cannot retrieve
value; returning default", ownKey, this);
             }
             return TypeCoercions.coerce((defaultValue != null) ? defaultValue : ownKey.getDefaultValue(),
key.getTypeToken());
         }
    +    
    +    @SuppressWarnings("unchecked")
    +    private <T> Maybe<T> getConfigImpl(ConfigKeySelfExtracting<T> key,
InheritanceMode parentInheritance) {
    +        ExecutionContext exec = entity.getExecutionContext();
    +        Maybe<T> ownValue;
    +        Maybe<T> parentValue;
    +
    +        // Get own value
    +        if (((ConfigKeySelfExtracting<T>)key).isSet(ownConfig)) {
    +            ownValue = Maybe.of(((ConfigKeySelfExtracting<T>)key).extractValue(ownConfig,
exec));
    +        } else if (localConfigBag.containsKey(key)) {
    +            // TODO configBag.get doesn't handle tasks/attributeWhenReady - it only uses
TypeCoercions
    +            // Precedence ordering has changed; previously we'd prefer an explicit isSet(inheritedConfig)
    +            // over the localConfigBag.get(key).
    +            ownValue = Maybe.of(localConfigBag.get(key));
    +        } else {
    +            ownValue = Maybe.<T>absent();
    +        }
    +        
    +        // Get the parent-inheritance value (but only if we'll need it)
    +        switch (parentInheritance) {
    +        case IF_NO_EXPLICIT_VALUE:
    +            if (ownValue.isAbsent()) {
    +                if (((ConfigKeySelfExtracting<T>)key).isSet(inheritedConfig)) {
    +                    parentValue = Maybe.of(((ConfigKeySelfExtracting<T>)key).extractValue(inheritedConfig,
exec));
    +                } else if (inheritedConfigBag.containsKey(key)) {
    +                    parentValue = Maybe.of(inheritedConfigBag.get(key));
    +                } else {
    +                    parentValue = Maybe.absent();
    +                }
    +            } else {
    +                parentValue = Maybe.absent();
    +            }
    +            break;
    +        case MERGE:
    +            if (((ConfigKeySelfExtracting<T>)key).isSet(inheritedConfig)) {
    +                parentValue = Maybe.of(((ConfigKeySelfExtracting<T>)key).extractValue(inheritedConfig,
exec));
    +            } else if (inheritedConfigBag.containsKey(key)) {
    +                parentValue = Maybe.of(inheritedConfigBag.get(key));
    +            } else {
    +                parentValue = Maybe.absent();
    +            }
    +            break;
    +        case NONE:
    +            parentValue = Maybe.absent();
    +            break;
    +        default:
    +            throw new IllegalStateException("Unsupported parent-inheritance mode for
"+key.getName()+": "+parentInheritance);
    +        }
    +
    +        // Merge or override, as appropriate
    +        switch (parentInheritance) {
    +        case IF_NO_EXPLICIT_VALUE:
    +            return ownValue.isPresent() ? ownValue : parentValue;
    +        case MERGE:
    +            return (Maybe<T>) deepMerge(ownValue, parentValue, key);
    +        case NONE:
    +            return ownValue;
    +        default:
    +            throw new IllegalStateException("Unsupported parent-inheritance mode for
"+key.getName()+": "+parentInheritance);
    +        }
    +    }
    +    
    +    private <T> Maybe<?> deepMerge(Maybe<? extends T> val1, Maybe<?
extends T> val2, ConfigKey<?> keyForLogging) {
    +        if (val2.isAbsent() || val2.isNull()) {
    +            return val1;
    +        } else if (val1.isAbsent()) {
    +            return val2;
    +        } else if (val1.isNull()) {
    +            return val1; // an explicit null means an override; don't merge
    +        } else if (val1.get() instanceof Map && val2.get() instanceof Map) {
    +            return Maybe.of(CollectionMerger.builder().build().merge((Map<?,?>)val1.get(),
(Map<?,?>)val2.get()));
    --- End diff --
    
    @neykov I kind of agree, but am not sure. The examples I can think of are all shallow
except for templateOptions. So doing a deep merge will have no impact (because the values
of `shell.env` etc will be shallow). Therefore keeping the default as deep feels like we have
a consistent rule that makes it work for templateOptions, and hopefully doesn't impact the
other uses.
    
    I worry slightly about deep for `templateOptions` (versus an explicit depth=2). If a value
within templateOptions is a map, then it will be deep-merged rather than overriding. Most
of the values are not, but there are some - e.g. jclouds `TemplateOptions.userMetadata(Map<String,
String> userMetadata)`. What would someone expect? Would they expect to merge additional
userMetadata or to override the value?
    
    There is also potentially surprising behaviour for lists + sets within templateOptions.
These values are merged to concatenate the sets/lists. For example, `TemplateOptions.networks(Iterable<String>
networks)` will be merged.
    
    I think for now we just want a sensible default that can be easily explained. And we can
worry later about the edge cases where the default doesn't match what someone wants, and it's
tricky (or even impossible) for them to avoid that without changing their super-type location-definition.
    
    Another possibility is that we have a parameterised `MERGE` that says the depth. But that
seems overly elaborate. Same for saying whether lists/sets should be merged or not.
    
    So in conclusion:
    * should we just rename it to DEEP_MERGE?
    * should we change the defaults so that it doesn't merge (i.e. concatenate) lists/sets
deep within a map when doing a 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.
---

Mime
View raw message