Github user sjcorbett commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/197#discussion_r18288709
  
    --- Diff: 
core/src/main/java/brooklyn/management/internal/LocalLocationManager.java ---
    @@ -158,50 +178,115 @@ public Location manage(Location loc) {
                 return loc;
             }
             
    -        AccessController.Response access = 
managementContext.getAccessManager().getAccessController().canManageLocation(loc);
    -        if (!access.isAllowed()) {
    -            throw new IllegalStateException("Access controller forbids 
management of "+loc+": "+access.getMsg());
    -        }
    -        
             Location parent = loc.getParent();
             if (parent != null && 
!managementContext.getLocationManager().isManaged(parent)) {
    -//            throw new IllegalStateException("Can't manage "+e+" because 
its parent is not yet managed ("+parent+")");
                 log.warn("Parent location "+parent+" of "+loc+" is not 
managed; attempting to manage it (in future this may be disallowed)");
    -            manage(parent);
    +            return manage(parent);
    +        } else {
    +            return manageRecursive(loc, ManagementTransitionMode.CREATING);
             }
    -        
    +    }
    +    
    +    @Override
    +    public void manageRebindedRoot(Location item) {
    +        ManagementTransitionMode mode = 
getLastManagementTransitionMode(item.getId());
    +        Preconditions.checkNotNull(mode, "Mode not set for rebinding %s", 
item);
    +        manageRecursive(item, mode);
    +    }
    +    
    +    protected Location manageRecursive(Location loc, final 
ManagementTransitionMode initialMode) {
    +        AccessController.Response access = 
managementContext.getAccessController().canManageLocation(loc);
    +        if (!access.isAllowed()) {
    +            throw new IllegalStateException("Access controller forbids 
management of "+loc+": "+access.getMsg());
    +        }
    +
             recursively(loc, new Predicate<AbstractLocation>() { public 
boolean apply(AbstractLocation it) {
    +            ManagementTransitionMode mode = 
getLastManagementTransitionMode(it.getId());
    +            if (mode==null) {
    +                setManagementTransitionMode(it, mode = initialMode);
    +            }
    +            
                 if (it.isManaged()) {
    -                return false;
    -            } else {
    -                boolean result = manageNonRecursive(it);
    -                if (result) {
    -                    it.setManagementContext(managementContext);
    +                if (mode==ManagementTransitionMode.CREATING) {
    +                    // silently bail out
    +                    return false;
    +                } else {
    +                    // on rebind, we just replace, fall through to below
    +                }
    +            }
    +            
    +            boolean result = manageNonRecursive(it, null);
    +            if (result) {
    +                it.setManagementContext(managementContext);
    +                if (!mode.isReadOnly()) {
                         it.onManagementStarted();
                         recordLocationEvent(it, Lifecycle.CREATED);
    -                    
managementContext.getRebindManager().getChangeListener().onManaged(it);
                     }
    -                return result;
    +                
managementContext.getRebindManager().getChangeListener().onManaged(it);
                 }
    +            return result;
             } });
             return loc;
         }
         
         @Override
    -    public void unmanage(Location loc) {
    +    public void unmanage(final Location loc) {
    +        unmanage(loc, ManagementTransitionMode.DESTROYING);
    +    }
    +    
    +    public void unmanage(final Location loc, final 
ManagementTransitionMode mode) {
    +        unmanage(loc, mode, false);
    +    }
    +    
    +    private void unmanage(final Location loc, ManagementTransitionMode 
mode, boolean hasBeenReplaced) {
             if (shouldSkipUnmanagement(loc)) return;
    -        
    -        recursively(loc, new Predicate<AbstractLocation>() { public 
boolean apply(AbstractLocation it) {
    -            if (shouldSkipUnmanagement(it)) return false;
    -            boolean result = unmanageNonRecursive(it);
    -            if (result) {
    -                it.onManagementStopped(); 
    -                
managementContext.getRebindManager().getChangeListener().onUnmanaged(it);
    -                recordLocationEvent(it, Lifecycle.DESTROYED);
    -                if (managementContext.gc != null) 
managementContext.gc.onUnmanaged(it);
    +
    +        if (hasBeenReplaced) {
    +            // we are unmanaging an old instance after having replaced it
    +            
    +            if 
(mode==ManagementTransitionMode.REBINDING_NO_LONGER_PRIMARY) {
    +                // when migrating away, these all need to be called
    +                
managementContext.getRebindManager().getChangeListener().onUnmanaged(loc);
    +                if (managementContext.gc != null) 
managementContext.gc.onUnmanaged(loc);
    +            } else {
    +                // should be coming *from* read only; nothing needed
    +                if (!mode.wasReadOnly())
    +                    log.warn("Should not be unmanaging "+loc+" in mode 
"+mode);
    --- End diff --
    
    Is this a programming error? Should it throw an exception? There is a 
similar case above.


---
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 [email protected] or file a JIRA ticket
with INFRA.
---

Reply via email to