Repository: brooklyn-server Updated Branches: refs/heads/master ce1192f9f -> 706fdb7b6
Fixes BrooklynFeatureEnablement setting defaults Previously if a downstream project set a default during static init, that was indistinguishable from subsequent explicit calls to setEnablement (or from brooklyn properties). Therefore the brooklyn.properties didnât overwrite the default. Project: http://git-wip-us.apache.org/repos/asf/brooklyn-server/repo Commit: http://git-wip-us.apache.org/repos/asf/brooklyn-server/commit/b32ca776 Tree: http://git-wip-us.apache.org/repos/asf/brooklyn-server/tree/b32ca776 Diff: http://git-wip-us.apache.org/repos/asf/brooklyn-server/diff/b32ca776 Branch: refs/heads/master Commit: b32ca776359eff57625a42ffd33888a9ff4bcf55 Parents: a941963 Author: Aled Sage <[email protected]> Authored: Fri Jul 1 09:58:43 2016 +0100 Committer: Aled Sage <[email protected]> Committed: Fri Jul 1 09:58:43 2016 +0100 ---------------------------------------------------------------------- .../core/BrooklynFeatureEnablement.java | 118 +++++++++++++------ ...rooklynFeatureEnablementPerformanceTest.java | 56 +++++++++ .../core/BrooklynFeatureEnablementTest.java | 25 +++- 3 files changed, 157 insertions(+), 42 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/b32ca776/core/src/main/java/org/apache/brooklyn/core/BrooklynFeatureEnablement.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/BrooklynFeatureEnablement.java b/core/src/main/java/org/apache/brooklyn/core/BrooklynFeatureEnablement.java index 1fb4767..48418a3 100644 --- a/core/src/main/java/org/apache/brooklyn/core/BrooklynFeatureEnablement.java +++ b/core/src/main/java/org/apache/brooklyn/core/BrooklynFeatureEnablement.java @@ -28,15 +28,22 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import com.google.common.annotations.Beta; +import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Objects; import com.google.common.collect.Maps; /** * For enabling/disabling experimental features. - * They can be enabled via java system properties, or by explicitly calling {@link #setEnablement(String, boolean)}. + * Feature enablement can be set in a number of ways, with the following precedence (most important first): + * <ol> + * <li>Explicit call to {@link #setEnablement(String, boolean)} (or to {@link #enable(String)} or {@link #disable(String)})). + * <li>Java system properties + * <li>Brooklyn properties (passed using {@link #init(BrooklynProperties)}) + * <li>Defaults set explicitly by calling {@link #setDefault(String, boolean)} + * <li>Hard-coded defaults + * </ol> * <p> - * For example, start brooklyn with {@code -Dbrooklyn.experimental.feature.policyPersistence=true} - * - * @author aled + * For example, start Brooklyn with {@code -Dbrooklyn.executionManager.renameThreads=true} */ @Beta public class BrooklynFeatureEnablement { @@ -103,9 +110,23 @@ public class BrooklynFeatureEnablement { public static final String FEATURE_SSH_ASYNC_EXEC = FEATURE_PROPERTY_PREFIX+".ssh.asyncExec"; public static final String FEATURE_VALIDATE_LOCATION_SSH_KEYS = "brooklyn.validate.locationSshKeys"; - + + /** + * Values explicitly set by Java calls. + */ private static final Map<String, Boolean> FEATURE_ENABLEMENTS = Maps.newLinkedHashMap(); + /** + * Values set from brooklyn.properties + */ + private static final Map<String, Boolean> FEATURE_ENABLEMENTS_PROPERTIES = Maps.newLinkedHashMap(); + + /** + * Defaults (e.g. set by the static block's call to {@link #setDefaults()}, or by downstream projects + * calling {@link #setDefault(String, boolean)}). + */ + private static final Map<String, Boolean> FEATURE_ENABLEMENT_DEFAULTS = Maps.newLinkedHashMap(); + private static final Object MUTEX = new Object(); static void setDefaults() { @@ -131,45 +152,53 @@ public class BrooklynFeatureEnablement { setDefaults(); } + public static boolean isEnabled(String property) { + synchronized (MUTEX) { + if (FEATURE_ENABLEMENTS.containsKey(property)) { + return FEATURE_ENABLEMENTS.get(property); + } else if (System.getProperty(property) != null) { + String rawVal = System.getProperty(property); + return Boolean.parseBoolean(rawVal); + } else if (FEATURE_ENABLEMENTS_PROPERTIES.containsKey(property)) { + return FEATURE_ENABLEMENTS_PROPERTIES.get(property); + } else if (FEATURE_ENABLEMENT_DEFAULTS.containsKey(property)) { + return FEATURE_ENABLEMENT_DEFAULTS.get(property); + } else { + return false; + } + } + } + /** * Initialises the feature-enablement from brooklyn properties. For each * property, prefer a system-property if present; otherwise use the value * from brooklyn properties. */ public static void init(BrooklynProperties props) { - boolean changed = false; - for (Map.Entry<String, Object> entry : props.asMapWithStringKeys().entrySet()) { - String property = entry.getKey(); - if (property.startsWith(FEATURE_PROPERTY_PREFIX)) { - if (!FEATURE_ENABLEMENTS.containsKey(property)) { - Object rawVal = System.getProperty(property); - if (rawVal == null) { - rawVal = entry.getValue(); - } - boolean val = Boolean.parseBoolean(""+rawVal); - FEATURE_ENABLEMENTS.put(property, val); + boolean found = false; + synchronized (MUTEX) { + for (Map.Entry<String, Object> entry : props.asMapWithStringKeys().entrySet()) { + String property = entry.getKey(); + if (property.startsWith(FEATURE_PROPERTY_PREFIX)) { + found = true; + Boolean oldVal = isEnabled(property); + boolean val = Boolean.parseBoolean(""+entry.getValue()); + FEATURE_ENABLEMENTS_PROPERTIES.put(property, val); + Boolean newVal = isEnabled(property); - changed = true; - LOG.debug("Init feature enablement of "+property+" set to "+val); + if (Objects.equal(oldVal, newVal)) { + LOG.debug("Enablement of "+property+" set to "+val+" from brooklyn properties (no-op as continues to resolve to "+oldVal+")"); + } else { + LOG.debug("Enablement of "+property+" set to "+val+" from brooklyn properties (resolved value previously "+oldVal+")"); + } } } - } - if (!changed) { - LOG.debug("Init feature enablement did nothing, as no settings in brooklyn properties"); - } - } - - public static boolean isEnabled(String property) { - synchronized (MUTEX) { - if (!FEATURE_ENABLEMENTS.containsKey(property)) { - String rawVal = System.getProperty(property); - boolean val = Boolean.parseBoolean(rawVal); - FEATURE_ENABLEMENTS.put(property, val); + if (!found) { + LOG.debug("Init feature enablement did nothing, as no settings in brooklyn properties"); } - return FEATURE_ENABLEMENTS.get(property); } } - + public static boolean enable(String property) { return setEnablement(property, true); } @@ -182,27 +211,38 @@ public class BrooklynFeatureEnablement { synchronized (MUTEX) { boolean oldVal = isEnabled(property); FEATURE_ENABLEMENTS.put(property, val); + + if (val == oldVal) { + LOG.debug("Enablement of "+property+" set to explicit "+val+" (no-op as resolved to same value previously)"); + } else { + LOG.debug("Enablement of "+property+" set to explicit "+val+" (previously resolved to "+oldVal+")"); + } return oldVal; } } public static void setDefault(String property, boolean val) { synchronized (MUTEX) { - if (!FEATURE_ENABLEMENTS.containsKey(property)) { - String rawVal = System.getProperty(property); - if (rawVal == null) { - FEATURE_ENABLEMENTS.put(property, val); - LOG.debug("Default enablement of "+property+" set to "+val); + Boolean oldDefaultVal = FEATURE_ENABLEMENT_DEFAULTS.get(property); + FEATURE_ENABLEMENT_DEFAULTS.put(property, val); + if (oldDefaultVal != null) { + if (oldDefaultVal.equals(val)) { + LOG.debug("Default enablement of "+property+" set to "+val+" (no-op as same as previous default value)"); } else { - LOG.debug("Not setting default enablement of "+property+" to "+val+", because system property is "+rawVal); + LOG.debug("Default enablement of "+property+" set to "+val+" (overwriting previous default of "+oldDefaultVal+")"); } + } else { + LOG.debug("Default enablement of "+property+" set to "+val); } } } - + + @VisibleForTesting static void clearCache() { synchronized (MUTEX) { FEATURE_ENABLEMENTS.clear(); + FEATURE_ENABLEMENTS_PROPERTIES.clear(); + FEATURE_ENABLEMENT_DEFAULTS.clear(); setDefaults(); } } http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/b32ca776/core/src/test/java/org/apache/brooklyn/core/BrooklynFeatureEnablementPerformanceTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/brooklyn/core/BrooklynFeatureEnablementPerformanceTest.java b/core/src/test/java/org/apache/brooklyn/core/BrooklynFeatureEnablementPerformanceTest.java new file mode 100644 index 0000000..e029def --- /dev/null +++ b/core/src/test/java/org/apache/brooklyn/core/BrooklynFeatureEnablementPerformanceTest.java @@ -0,0 +1,56 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.brooklyn.core; + +import static org.testng.Assert.assertFalse; + +import org.apache.brooklyn.core.test.qa.performance.AbstractPerformanceTest; +import org.apache.brooklyn.test.performance.PerformanceTestDescriptor; +import org.testng.annotations.Test; + +public class BrooklynFeatureEnablementPerformanceTest extends AbstractPerformanceTest { + + protected int numIterations() { + return 10000; + } + + /** + * Expect this to be blazingly fast; double-checking because it's not efficiently written: + * <ul> + * <li>It doesn't cache the System.getProperty result, but I'd expect the JVM to do that! + * <li>It synchronizes on every access (rather than using a more efficient immutable copy/cache + * for example, which is then replaced atomically by a new immutable cache). + * </ul> + */ + @Test(groups={"Integration", "Acceptance"}) + public void testIsEnabled() { + int numIterations = numIterations(); + double minRatePerSec = 100000 * PERFORMANCE_EXPECTATION; + final String featureProperty = "brooklyn.experimental.feature.testIsEnabled.performance"; + + measure(PerformanceTestDescriptor.create() + .summary("EntityPerformanceTest.testUpdateAttributeWhenNoListeners") + .iterations(numIterations) + .minAcceptablePerSecond(minRatePerSec) + .job(new Runnable() { + public void run() { + assertFalse(BrooklynFeatureEnablement.isEnabled(featureProperty)); + }})); + } +} http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/b32ca776/core/src/test/java/org/apache/brooklyn/core/BrooklynFeatureEnablementTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/brooklyn/core/BrooklynFeatureEnablementTest.java b/core/src/test/java/org/apache/brooklyn/core/BrooklynFeatureEnablementTest.java index 4a8505d..f7bf6f9 100644 --- a/core/src/test/java/org/apache/brooklyn/core/BrooklynFeatureEnablementTest.java +++ b/core/src/test/java/org/apache/brooklyn/core/BrooklynFeatureEnablementTest.java @@ -94,6 +94,16 @@ public class BrooklynFeatureEnablementTest { @Test public void testCanSetDefaultWhichIsIgnoredIfBrooklynProps() throws Exception { String featureProperty = "brooklyn.experimental.feature.testCanSetDefaultWhichIsIgnoredIfBrooklynProps"; + BrooklynFeatureEnablement.setDefault(featureProperty, true); + BrooklynProperties props = BrooklynProperties.Factory.newEmpty(); + props.put(featureProperty, false); + BrooklynFeatureEnablement.init(props); + assertFalse(BrooklynFeatureEnablement.isEnabled(featureProperty)); + } + + @Test + public void testSetDefaultAfterBrooklynPropsDoesNotChangeValue() throws Exception { + String featureProperty = "brooklyn.experimental.feature.testSetDefaultAfterBrooklynPropsDoesNotChangeValue"; BrooklynProperties props = BrooklynProperties.Factory.newEmpty(); props.put(featureProperty, false); BrooklynFeatureEnablement.init(props); @@ -102,14 +112,23 @@ public class BrooklynFeatureEnablementTest { } @Test + public void testSetDefaultAfterCheckingIfEnabledChangesValue() throws Exception { + String featureProperty = "brooklyn.experimental.feature.testSetDefaultAfterCheckingIfEnabledChangesValue"; + assertFalse(BrooklynFeatureEnablement.isEnabled(featureProperty)); + + BrooklynFeatureEnablement.setDefault(featureProperty, true); + assertTrue(BrooklynFeatureEnablement.isEnabled(featureProperty)); + } + + @Test public void testPrefersSysPropOverBrooklynProps() throws Exception { String featureProperty = "brooklyn.experimental.feature.testPrefersSysPropOverBrooklynProps"; - BrooklynProperties props = BrooklynProperties.Factory.newEmpty(); - props.put(featureProperty, false); System.setProperty(featureProperty, "true"); try { - BrooklynFeatureEnablement.init(props); BrooklynFeatureEnablement.setDefault(featureProperty, true); + BrooklynProperties props = BrooklynProperties.Factory.newEmpty(); + props.put(featureProperty, false); + BrooklynFeatureEnablement.init(props); assertTrue(BrooklynFeatureEnablement.isEnabled(featureProperty)); } finally { System.clearProperty(featureProperty);
