[GitHub] brooklyn-server pull request #565: Be truly immediate/non-blocking more ofte...

[aledsage <gi...@git.apache.org>]

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