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 #835: Tasks subsystem improvements - adjuncts, ...
Date Thu, 05 Oct 2017 12:37:30 GMT
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/835#discussion_r142917944
  
    --- Diff: core/src/main/java/org/apache/brooklyn/core/mgmt/internal/NonDeploymentManagementContext.java
---
    @@ -254,7 +250,19 @@ public synchronized SubscriptionContext getSubscriptionContext(Entity
entity) {
             if (!this.entity.equals(entity)) throw new IllegalStateException("Non-deployment
context "+this+" can only use a single Entity: has "+this.entity+", but passed "+entity);
             if (mode==NonDeploymentManagementContextMode.MANAGEMENT_STOPPED)
                 throw new IllegalStateException("Entity "+entity+" is no longer managed;
subscription context not available");
    -        return subscriptionContext;
    +        // see also AbstractManagementContext.getSubscriptionContext - needed for callbacks,
to ensure the correct entity context is set
    +        Map<String, ?> subscriptionFlags = ImmutableMap.of("tags", ImmutableList.of(BrooklynTaskTags.tagForContextEntity(entity)));
    +        return new BasicSubscriptionContext(subscriptionFlags, qsm, entity);
    --- End diff --
    
    Why instantiate this in the `getSubscriptionContext` rather than the constructor?
    Do you really want a new instance every time it is called, rather than lazy-instantiation
and returning the same one?
    
    For the record, I don't think it should really matter (i.e. fine as-is) - at some point
we should be able to delete lots of this code. It was originally written when folk could call
entity constructors directly and then call methods on it (so the entity would not have a usable
management context).


---

Mime
View raw message