Policy/enricher rebind: use config - use âconfigâ instead of âflagsâ - use no-arg constructor where available - adds ManagementContextInternal.getPolicyFactory() - refactor InternalPolicyFactory to be more like InternalLocationFactory
Project: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/commit/80b05c15 Tree: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/tree/80b05c15 Diff: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/diff/80b05c15 Branch: refs/heads/master Commit: 80b05c15e23b3cb583f6625372f3353de804a289 Parents: 693a453 Author: Aled Sage <[email protected]> Authored: Tue May 27 10:37:07 2014 +0100 Committer: Aled Sage <[email protected]> Committed: Fri May 30 10:24:38 2014 +0100 ---------------------------------------------------------------------- .../entity/proxying/InternalPolicyFactory.java | 64 ++++++++++----- .../entity/rebind/RebindManagerImpl.java | 85 ++++++++++++++++---- .../entity/rebind/dto/BasicEnricherMemento.java | 2 +- .../entity/rebind/dto/MementosGenerators.java | 59 ++++++++++---- .../management/internal/LocalEntityManager.java | 5 ++ .../internal/LocalManagementContext.java | 6 ++ .../internal/ManagementContextInternal.java | 3 + .../NonDeploymentManagementContext.java | 7 ++ 8 files changed, 177 insertions(+), 54 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/80b05c15/core/src/main/java/brooklyn/entity/proxying/InternalPolicyFactory.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/entity/proxying/InternalPolicyFactory.java b/core/src/main/java/brooklyn/entity/proxying/InternalPolicyFactory.java index c51144d..1a5c4ed 100644 --- a/core/src/main/java/brooklyn/entity/proxying/InternalPolicyFactory.java +++ b/core/src/main/java/brooklyn/entity/proxying/InternalPolicyFactory.java @@ -113,14 +113,13 @@ public class InternalPolicyFactory { try { Class<? extends T> clazz = spec.getType(); - FactoryConstructionTracker.setConstructing(); T pol; - try { - pol = construct(clazz, spec); - } finally { - FactoryConstructionTracker.reset(); + if (isNewStylePolicy(clazz)) { + pol = constructPolicy(clazz); + } else { + pol = constructOldStyle(clazz, MutableMap.copyOf(spec.getFlags())); } - + if (spec.getDisplayName()!=null) ((AbstractPolicy)pol).setName(spec.getDisplayName()); @@ -154,12 +153,11 @@ public class InternalPolicyFactory { try { Class<? extends T> clazz = spec.getType(); - FactoryConstructionTracker.setConstructing(); T enricher; - try { - enricher = construct(clazz, spec); - } finally { - FactoryConstructionTracker.reset(); + if (isNewStyleEnricher(clazz)) { + enricher = constructEnricher(clazz); + } else { + enricher = constructOldStyle(clazz, MutableMap.copyOf(spec.getFlags())); } if (spec.getDisplayName()!=null) @@ -186,19 +184,43 @@ public class InternalPolicyFactory { } } - private <T extends Policy> T construct(Class<? extends T> clazz, PolicySpec<T> spec) throws InstantiationException, IllegalAccessException, InvocationTargetException { - if (isNewStylePolicy(clazz)) { - return clazz.newInstance(); - } else { - return constructOldStyle(clazz, MutableMap.copyOf(spec.getFlags())); + /** + * Constructs a new-style policy (fails if no no-arg constructor). + */ + public <T extends Policy> T constructPolicy(Class<T> clazz) { + try { + FactoryConstructionTracker.setConstructing(); + try { + if (isNewStylePolicy(clazz)) { + return clazz.newInstance(); + } else { + throw new IllegalStateException("Policy class "+clazz+" must have a no-arg constructor"); + } + } finally { + FactoryConstructionTracker.reset(); + } + } catch (Exception e) { + throw Exceptions.propagate(e); } } - private <T extends Enricher> T construct(Class<? extends T> clazz, EnricherSpec<T> spec) throws InstantiationException, IllegalAccessException, InvocationTargetException { - if (isNewStyleEnricher(clazz)) { - return clazz.newInstance(); - } else { - return constructOldStyle(clazz, MutableMap.copyOf(spec.getFlags())); + /** + * Constructs a new-style enricher (fails if no no-arg constructor). + */ + public <T extends Enricher> T constructEnricher(Class<T> clazz) { + try { + FactoryConstructionTracker.setConstructing(); + try { + if (isNewStyleEnricher(clazz)) { + return clazz.newInstance(); + } else { + throw new IllegalStateException("Enricher class "+clazz+" must have a no-arg constructor"); + } + } finally { + FactoryConstructionTracker.reset(); + } + } catch (Exception e) { + throw Exceptions.propagate(e); } } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/80b05c15/core/src/main/java/brooklyn/entity/rebind/RebindManagerImpl.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/entity/rebind/RebindManagerImpl.java b/core/src/main/java/brooklyn/entity/rebind/RebindManagerImpl.java index ab5a8eb..5cc6f6e 100644 --- a/core/src/main/java/brooklyn/entity/rebind/RebindManagerImpl.java +++ b/core/src/main/java/brooklyn/entity/rebind/RebindManagerImpl.java @@ -11,6 +11,7 @@ import java.util.concurrent.TimeoutException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import brooklyn.enricher.basic.AbstractEnricher; import brooklyn.entity.Application; import brooklyn.entity.Entity; import brooklyn.entity.basic.AbstractApplication; @@ -21,6 +22,7 @@ import brooklyn.entity.proxying.EntitySpec; import brooklyn.entity.proxying.InternalEntityFactory; import brooklyn.entity.proxying.InternalLocationFactory; import brooklyn.entity.rebind.RebindExceptionHandlerImpl.RebindFailureMode; +import brooklyn.entity.proxying.InternalPolicyFactory; import brooklyn.location.Location; import brooklyn.location.basic.AbstractLocation; import brooklyn.location.basic.LocationInternal; @@ -36,6 +38,7 @@ import brooklyn.mementos.PolicyMemento; import brooklyn.mementos.TreeNode; import brooklyn.policy.Enricher; import brooklyn.policy.Policy; +import brooklyn.policy.basic.AbstractPolicy; import brooklyn.util.collections.MutableMap; import brooklyn.util.exceptions.Exceptions; import brooklyn.util.flags.FlagUtils; @@ -513,35 +516,62 @@ public class RebindManagerImpl implements RebindManager { } /** - * Constructs a new policy, passing to its constructor the policy id and all of memento.getFlags(). + * Constructs a new policy, passing to its constructor the policy id and all of memento.getConfig(). */ private Policy newPolicy(PolicyMemento memento, Reflections reflections) { String id = memento.getId(); - String policyType = checkNotNull(memento.getType(), "policyType of "+id); - Class<?> policyClazz = reflections.loadClass(policyType); + String policyType = checkNotNull(memento.getType(), "policy type of %s must not be null in memento", id); + Class<? extends Policy> policyClazz = (Class<? extends Policy>) reflections.loadClass(policyType); + + if (InternalPolicyFactory.isNewStylePolicy(policyClazz)) { + InternalPolicyFactory policyFactory = managementContext.getPolicyFactory(); + Policy policy = policyFactory.constructPolicy(policyClazz); + FlagUtils.setFieldsFromFlags(ImmutableMap.of("id", id), policy); + ((AbstractPolicy)policy).setManagementContext(managementContext); + + return policy; - Map<String, Object> flags = MutableMap.<String, Object>builder() - .put("id", id) - .putAll(memento.getConfig()) - .build(); + } else { + LOG.warn("Deprecated rebind of policy without no-arg constructor; this may not be supported in future versions: id="+id+"; type="+policyType); + + // There are several possibilities for the constructor; find one that works. + // Prefer passing in the flags because required for Application to set the management context + // TODO Feels very hacky! + Map<String, Object> flags = MutableMap.<String, Object>of("id", id, "deferConstructionChecks", true); + flags.putAll(memento.getConfig()); - return (Policy) invokeConstructor(reflections, policyClazz, new Object[] {flags}); + return (Policy) invokeConstructor(reflections, policyClazz, new Object[] {flags}); + } } /** - * Constructs a new enricher, passing to its constructor the enricher id and all of memento.getFlags(). + * Constructs a new enricher, passing to its constructor the enricher id and all of memento.getConfig(). */ private Enricher newEnricher(EnricherMemento memento, Reflections reflections) { String id = memento.getId(); - String enricherType = checkNotNull(memento.getType(), "enricherType of "+id); - Class<?> enricherClazz = reflections.loadClass(enricherType); + String enricherType = checkNotNull(memento.getType(), "enricher type of %s must not be null in memento", id); + Class<? extends Enricher> enricherClazz = (Class<? extends Enricher>) reflections.loadClass(enricherType); + + if (InternalPolicyFactory.isNewStyleEnricher(enricherClazz)) { + InternalPolicyFactory policyFactory = managementContext.getPolicyFactory(); + Enricher enricher = policyFactory.constructEnricher(enricherClazz); + FlagUtils.setFieldsFromFlags(ImmutableMap.of("id", id), enricher); + ((AbstractEnricher)enricher).setManagementContext(managementContext); + + return enricher; - Map<String, Object> flags = MutableMap.<String, Object>builder() - .put("id", id) - .putAll(memento.getConfig()) - .build(); + } else { + LOG.warn("Deprecated rebind of enricher without no-arg constructor; this may not be supported in future versions: id="+id+"; type="+enricherType); + + // There are several possibilities for the constructor; find one that works. + // Prefer passing in the flags because required for Application to set the management context + // TODO Feels very hacky! + Map<String, Object> flags = MutableMap.<String, Object>of("id", id, "deferConstructionChecks", true); + flags.putAll(memento.getConfig()); + + return (Enricher) invokeConstructor(reflections, enricherClazz, new Object[] {flags}); + } - return (Enricher) invokeConstructor(reflections, enricherClazz, new Object[] {flags}); } private <T> T invokeConstructor(Reflections reflections, Class<T> clazz, Object[]... possibleArgs) { @@ -558,6 +588,29 @@ public class RebindManagerImpl implements RebindManager { throw new IllegalStateException("Cannot instantiate instance of type "+clazz+"; expected constructor signature not found"); } + private static boolean isNewStylePolicy(Class<?> clazz) { + if (!Policy.class.isAssignableFrom(clazz)) { + throw new IllegalArgumentException("Class "+clazz+" is not a policy"); + } + return hasNoArgConstructor(clazz); + } + + private static boolean isNewStyleEnricher(Class<?> clazz) { + if (!Enricher.class.isAssignableFrom(clazz)) { + throw new IllegalArgumentException("Class "+clazz+" is not an enricher"); + } + return hasNoArgConstructor(clazz); + } + + private static boolean hasNoArgConstructor(Class<?> clazz) { + try { + clazz.getConstructor(new Class[0]); + return true; + } catch (NoSuchMethodException e) { + return false; + } + } + /** * Wraps a ChangeListener, to log and never propagate any exceptions that it throws. * http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/80b05c15/core/src/main/java/brooklyn/entity/rebind/dto/BasicEnricherMemento.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/entity/rebind/dto/BasicEnricherMemento.java b/core/src/main/java/brooklyn/entity/rebind/dto/BasicEnricherMemento.java index e1c751a..3642101 100644 --- a/core/src/main/java/brooklyn/entity/rebind/dto/BasicEnricherMemento.java +++ b/core/src/main/java/brooklyn/entity/rebind/dto/BasicEnricherMemento.java @@ -16,7 +16,7 @@ import com.google.common.collect.Maps; */ public class BasicEnricherMemento extends AbstractMemento implements EnricherMemento, Serializable { - private static final long serialVersionUID = -1; // FIXME + private static final long serialVersionUID = 3922505388588186311L; public static Builder builder() { return new Builder(); http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/80b05c15/core/src/main/java/brooklyn/entity/rebind/dto/MementosGenerators.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/entity/rebind/dto/MementosGenerators.java b/core/src/main/java/brooklyn/entity/rebind/dto/MementosGenerators.java index f3214df..9f744ba 100644 --- a/core/src/main/java/brooklyn/entity/rebind/dto/MementosGenerators.java +++ b/core/src/main/java/brooklyn/entity/rebind/dto/MementosGenerators.java @@ -7,6 +7,7 @@ import java.util.Map; import java.util.Set; import brooklyn.config.ConfigKey; +import brooklyn.enricher.basic.AbstractEnricher; import brooklyn.entity.Application; import brooklyn.entity.Entity; import brooklyn.entity.Group; @@ -27,6 +28,7 @@ import brooklyn.mementos.LocationMemento; import brooklyn.mementos.PolicyMemento; import brooklyn.policy.Enricher; import brooklyn.policy.Policy; +import brooklyn.policy.basic.AbstractPolicy; import brooklyn.util.collections.MutableMap; import brooklyn.util.config.ConfigBag; import brooklyn.util.flags.FlagUtils; @@ -97,20 +99,7 @@ public class MementosGenerators { Map<ConfigKey<?>, Object> localConfig = ((EntityInternal)entity).getConfigMap().getLocalConfig(); for (Map.Entry<ConfigKey<?>, Object> entry : localConfig.entrySet()) { ConfigKey<?> key = checkNotNull(entry.getKey(), localConfig); - Object value = entry.getValue(); - - // TODO Swapping an attributeWhenReady task for the actual value, if completed. - // Long-term, want to just handle task-persistence properly. - if (value instanceof Task) { - Task<?> task = (Task<?>) value; - if (task.isDone() && !task.isError()) { - value = task.getUnchecked(); - } else { - // TODO how to record a completed but errored task? - value = null; - } - } - + Object value = configValueToPersistable(entry.getValue()); builder.config.put(key, value); } @@ -217,8 +206,19 @@ public class MementosGenerators { builder.id = policy.getId(); builder.displayName = policy.getName(); - builder.flags.putAll(FlagUtils.getFieldsWithFlagsExcludingModifiers(policy, Modifier.STATIC ^ Modifier.TRANSIENT)); + // TODO persist config keys as well? Or only support those defined on policy class; + // current code will lose the ConfigKey type on rebind for anything not defined on class. + // Whereas entities support that. + // TODO Do we need the "nonPersistableFlagNames" that locations use? + Map<ConfigKey<?>, Object> config = ((AbstractPolicy)policy).getConfigMap().getAllConfig(); + for (Map.Entry<ConfigKey<?>, Object> entry : config.entrySet()) { + ConfigKey<?> key = checkNotNull(entry.getKey(), "config=%s", config); + Object value = configValueToPersistable(entry.getValue()); + builder.config.put(key.getName(), value); + } + builder.config.putAll(FlagUtils.getFieldsWithFlagsExcludingModifiers(policy, Modifier.STATIC ^ Modifier.TRANSIENT)); + return builder; } @@ -237,8 +237,35 @@ public class MementosGenerators { builder.id = enricher.getId(); builder.displayName = enricher.getName(); - builder.flags.putAll(FlagUtils.getFieldsWithFlagsExcludingModifiers(enricher, Modifier.STATIC ^ Modifier.TRANSIENT)); + // TODO persist config keys as well? Or only support those defined on policy class; + // current code will lose the ConfigKey type on rebind for anything not defined on class. + // Whereas entities support that. + // TODO Do we need the "nonPersistableFlagNames" that locations use? + Map<ConfigKey<?>, Object> config = ((AbstractEnricher)enricher).getConfigMap().getAllConfig(); + for (Map.Entry<ConfigKey<?>, Object> entry : config.entrySet()) { + ConfigKey<?> key = checkNotNull(entry.getKey(), "config=%s", config); + Object value = configValueToPersistable(entry.getValue()); + builder.config.put(key.getName(), value); + } + builder.config.putAll(FlagUtils.getFieldsWithFlagsExcludingModifiers(enricher, Modifier.STATIC ^ Modifier.TRANSIENT)); + return builder; + + } + + protected static Object configValueToPersistable(Object value) { + // TODO Swapping an attributeWhenReady task for the actual value, if completed. + // Long-term, want to just handle task-persistence properly. + if (value instanceof Task) { + Task<?> task = (Task<?>) value; + if (task.isDone() && !task.isError()) { + return task.getUnchecked(); + } else { + // TODO how to record a completed but errored task? + return null; + } + } + return value; } } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/80b05c15/core/src/main/java/brooklyn/management/internal/LocalEntityManager.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/management/internal/LocalEntityManager.java b/core/src/main/java/brooklyn/management/internal/LocalEntityManager.java index eaca3d2..c806645 100644 --- a/core/src/main/java/brooklyn/management/internal/LocalEntityManager.java +++ b/core/src/main/java/brooklyn/management/internal/LocalEntityManager.java @@ -92,6 +92,11 @@ public class LocalEntityManager implements EntityManagerInternal { return entityFactory; } + public InternalPolicyFactory getPolicyFactory() { + if (!isRunning()) throw new IllegalStateException("Management context no longer running"); + return policyFactory; + } + @Override public EntityTypeRegistry getEntityTypeRegistry() { if (!isRunning()) throw new IllegalStateException("Management context no longer running"); http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/80b05c15/core/src/main/java/brooklyn/management/internal/LocalManagementContext.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/management/internal/LocalManagementContext.java b/core/src/main/java/brooklyn/management/internal/LocalManagementContext.java index 643583b..96ccce7 100644 --- a/core/src/main/java/brooklyn/management/internal/LocalManagementContext.java +++ b/core/src/main/java/brooklyn/management/internal/LocalManagementContext.java @@ -25,6 +25,7 @@ import brooklyn.entity.drivers.downloads.BasicDownloadsManager; import brooklyn.entity.effector.Effectors; import brooklyn.entity.proxying.InternalEntityFactory; import brooklyn.entity.proxying.InternalLocationFactory; +import brooklyn.entity.proxying.InternalPolicyFactory; import brooklyn.internal.storage.DataGridFactory; import brooklyn.location.Location; import brooklyn.management.AccessController; @@ -222,6 +223,11 @@ public class LocalManagementContext extends AbstractManagementContext { } @Override + public InternalPolicyFactory getPolicyFactory() { + return getEntityManager().getPolicyFactory(); + } + + @Override public synchronized LocalLocationManager getLocationManager() { if (!isRunning()) throw new IllegalStateException("Management context no longer running"); return locationManager; http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/80b05c15/core/src/main/java/brooklyn/management/internal/ManagementContextInternal.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/management/internal/ManagementContextInternal.java b/core/src/main/java/brooklyn/management/internal/ManagementContextInternal.java index fee519d..fd51427 100644 --- a/core/src/main/java/brooklyn/management/internal/ManagementContextInternal.java +++ b/core/src/main/java/brooklyn/management/internal/ManagementContextInternal.java @@ -12,6 +12,7 @@ import brooklyn.entity.basic.BrooklynTaskTags; import brooklyn.entity.basic.ConfigKeys; import brooklyn.entity.proxying.InternalEntityFactory; import brooklyn.entity.proxying.InternalLocationFactory; +import brooklyn.entity.proxying.InternalPolicyFactory; import brooklyn.internal.storage.BrooklynStorage; import brooklyn.location.Location; import brooklyn.management.ManagementContext; @@ -60,6 +61,8 @@ public interface ManagementContextInternal extends ManagementContext { InternalLocationFactory getLocationFactory(); + InternalPolicyFactory getPolicyFactory(); + /** * Registers an entity that has been created, but that has not yet begun to be managed. * <p> http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/80b05c15/core/src/main/java/brooklyn/management/internal/NonDeploymentManagementContext.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/management/internal/NonDeploymentManagementContext.java b/core/src/main/java/brooklyn/management/internal/NonDeploymentManagementContext.java index fcc7d10..7f49629 100644 --- a/core/src/main/java/brooklyn/management/internal/NonDeploymentManagementContext.java +++ b/core/src/main/java/brooklyn/management/internal/NonDeploymentManagementContext.java @@ -22,6 +22,7 @@ import brooklyn.entity.drivers.EntityDriverManager; import brooklyn.entity.drivers.downloads.DownloadResolverManager; import brooklyn.entity.proxying.InternalEntityFactory; import brooklyn.entity.proxying.InternalLocationFactory; +import brooklyn.entity.proxying.InternalPolicyFactory; import brooklyn.entity.rebind.ChangeListener; import brooklyn.entity.rebind.RebindExceptionHandler; import brooklyn.entity.rebind.RebindManager; @@ -134,6 +135,12 @@ public class NonDeploymentManagementContext implements ManagementContextInternal } @Override + public InternalPolicyFactory getPolicyFactory() { + checkInitialManagementContextReal(); + return initialManagementContext.getPolicyFactory(); + } + + @Override public EntityManager getEntityManager() { return entityManager; }
