brooklyn-dev mailing list archives

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

    https://github.com/apache/brooklyn-server/pull/173#discussion_r65531968
  
    --- 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 --
    
    In the general case a shallow merge would be the expected behaviour for me. Deep merge
makes sense only for `provisioning.properties` (where in practice it's of depth 2 and only
because of the special treatment of `templateOptions`). wdyt about having two inheritance
modes - DEEP_MERGE, SHALLOW_MERGE? Or at least rename MERGE to DEEP_MERGE for 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.
---

Mime
View raw message