brooklyn-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From aleds...@apache.org
Subject [1/2] brooklyn-server git commit: Fixes BrooklynFeatureEnablement setting defaults
Date Mon, 04 Jul 2016 11:31:15 GMT
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 <aled.sage@gmail.com>
Authored: Fri Jul 1 09:58:43 2016 +0100
Committer: Aled Sage <aled.sage@gmail.com>
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);


Mime
View raw message