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 #565: Be truly immediate/non-blocking more ofte...
Date Mon, 06 Mar 2017 12:38:59 GMT
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/565#discussion_r104391702
  
    --- Diff: core/src/main/java/org/apache/brooklyn/core/objs/AbstractConfigurationSupportInternal.java
---
    @@ -139,17 +132,19 @@ public T call() {
             // or Absent if the config key was unset.
             Object unresolved = getRaw(key).or(key.getDefaultValue());
             final Object marker = new Object();
    -        // Give tasks a short grace period to resolve.
    -        Object resolved = Tasks.resolving(unresolved)
    +        Maybe<Object> resolved = Tasks.resolving(unresolved)
                     .as(Object.class)
                     .defaultValue(marker)
                     .immediately(true)
                     .deep(true)
                     .context(getContext())
    -                .get();
    -        return (resolved != marker)
    -                ? TypeCoercions.tryCoerce(resolved, key.getTypeToken())
    -                        : Maybe.<T>absent();
    +                .getMaybe();
    +        if (resolved.isAbsent()) return Maybe.Absent.<T>castAbsent(resolved);
    +        if (resolved.get()==marker) {
    +            // TODO changed Feb 2017, previously returned absent, in contrast to what
the javadoc says
    --- End diff --
    
    I don't think this code will ever be called. The `.defaultValue(marker)` only affects
the behaviour of `.get()`. It doesn't affect the result of `.getMaybe()`, so we'll never have
the marker object returned (now that you've changed it to use `getMaybe()`).
    
    I'm fine with us leaving this in for your PR, but I'd be interested what situation you're
trying to cover with this. I tried writing a few more tests but could never get into this
code path.
    
    My understanding of what it was doing previously... By setting the `defaultValue(marker)`
and calling `get()`, it was trying to avoid the exception being thrown when `getMaybe()` would
have return an absent (so instead it returned us the marker). That would seem to agree with
the javadoc (which javadoc are you referring to?).
    
    However, I agree that your change to call `getMaybe()` instead is much nicer - we get
to keep the original `Maybe.absent()` value has the better explanation of why it's absent.


---
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