You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@brooklyn.apache.org by he...@apache.org on 2015/04/13 03:11:48 UTC

[1/3] incubator-brooklyn git commit: make ConfigBag thread-safe

Repository: incubator-brooklyn
Updated Branches:
  refs/heads/master 04fc801d0 -> bf07fe1f0


make ConfigBag thread-safe


Project: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/commit/5e34f950
Tree: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/tree/5e34f950
Diff: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/diff/5e34f950

Branch: refs/heads/master
Commit: 5e34f950f1d6a7875224112b60ae2de810b18feb
Parents: 3ce25c9
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Authored: Thu Apr 9 12:26:18 2015 +0100
Committer: Alex Heneveld <al...@cloudsoftcorp.com>
Committed: Thu Apr 9 12:27:07 2015 +0100

----------------------------------------------------------------------
 .../java/brooklyn/util/config/ConfigBag.java    | 130 +++++++++----
 .../brooklyn/util/config/ConfigBagTest.java     | 191 +++++++++++++++++++
 2 files changed, 287 insertions(+), 34 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/5e34f950/core/src/main/java/brooklyn/util/config/ConfigBag.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/brooklyn/util/config/ConfigBag.java b/core/src/main/java/brooklyn/util/config/ConfigBag.java
index c57098b..220ec52 100644
--- a/core/src/main/java/brooklyn/util/config/ConfigBag.java
+++ b/core/src/main/java/brooklyn/util/config/ConfigBag.java
@@ -20,7 +20,7 @@ package brooklyn.util.config;
 
 import static com.google.common.base.Preconditions.checkNotNull;
 
-import java.util.Collections;
+import java.util.ConcurrentModificationException;
 import java.util.LinkedHashMap;
 import java.util.Map;
 
@@ -37,6 +37,7 @@ import brooklyn.util.flags.TypeCoercions;
 import brooklyn.util.guava.Maybe;
 import brooklyn.util.javalang.JavaClassNames;
 
+import com.google.common.annotations.Beta;
 import com.google.common.base.Objects;
 import com.google.common.collect.Sets;
 
@@ -47,7 +48,9 @@ import com.google.common.collect.Sets;
  * It is recommended to use {@link ConfigKey} instances to access,
  * although in some cases (such as setting fields from flags, or copying a map)
  * it may be necessary to mark things as used, or put, when only a string key is available.
- * 
+ * <p>
+ * This bag is order-preserving and thread-safe except where otherwise indicated.
+ * <p>
  * @author alex
  */
 public class ConfigBag {
@@ -71,6 +74,9 @@ public class ConfigBag {
 
     /**
      * Creates an instance that is backed by a "live map" (e.g. storage in a datagrid).
+     * The order-preserving nature of this class is only guaranteed if the
+     * provided storage has those properties. External modifications to the store can cause
+     * {@link ConcurrentModificationException} to be thrown, here or elsewhere. 
      */
     public static ConfigBag newLiveInstance(Map<String,Object> storage) {
         return new ConfigBag(checkNotNull(storage, "storage map must be specified"));
@@ -96,14 +102,42 @@ public class ConfigBag {
      * (note: this applies even for values which are overridden and the overridden value is used);
      * however subsequent uses in the original set will not be marked here
      */
-    public static ConfigBag newInstanceExtending(final ConfigBag configBag, Map<?,?> flags) {
+    @Beta
+    public static ConfigBag newInstanceExtending(final ConfigBag parentBag) {
+        return new ConfigBagExtendingParent(parentBag).copy(parentBag);
+    }
+
+    /** @see #newInstanceExtending(ConfigBag) */
+    public static class ConfigBagExtendingParent extends ConfigBag {
+        ConfigBag parentBag;
+        private ConfigBagExtendingParent(ConfigBag parentBag) {
+            this.parentBag = parentBag;
+        }
+        @Override
+        public void markUsed(String key) {
+            super.markUsed(key);
+            if (parentBag!=null)
+                parentBag.markUsed(key);
+        }
+    }
+    
+    /** As {@link #newInstanceExtending(ConfigBag)} but also putting the supplied values. */
+    @Beta
+    public static ConfigBag newInstanceExtending(final ConfigBag configBag, Map<?,?> optionalAdditionalValues) {
+        return newInstanceExtending(configBag).putAll(optionalAdditionalValues);
+    }
+
+    /** @deprecated since 0.7.0, not used; kept only for rebind compatibility where the inner class is used 
+     * (now replaced by a static class above) */
+    @Beta @Deprecated
+    public static ConfigBag newInstanceWithInnerClass(final ConfigBag configBag, Map<?,?> optionalAdditionalValues) {
         return new ConfigBag() {
             @Override
             public void markUsed(String key) {
                 super.markUsed(key);
                 configBag.markUsed(key);
             }
-        }.copy(configBag).putAll(flags);
+        }.copy(configBag).putAll(optionalAdditionalValues);
     }
 
     public ConfigBag() {
@@ -132,12 +166,12 @@ public class ConfigBag {
     
     /** current values for all entries 
      * @return non-modifiable map of strings to object */
-    public Map<String,Object> getAllConfig() {
-        return Collections.unmodifiableMap(config);
+    public synchronized Map<String,Object> getAllConfig() {
+        return MutableMap.copyOf(config).asUnmodifiable();
     }
 
     /** current values for all entries in a map where the keys are converted to {@link ConfigKey} instances */
-    public Map<ConfigKey<?>, ?> getAllConfigAsConfigKeyMap() {
+    public synchronized Map<ConfigKey<?>, ?> getAllConfigAsConfigKeyMap() {
         Map<ConfigKey<?>,Object> result = MutableMap.of();
         for (Map.Entry<String,Object> entry: config.entrySet()) {
             result.put(ConfigKeys.newConfigKey(Object.class, entry.getKey()), entry.getValue());
@@ -145,15 +179,18 @@ public class ConfigBag {
         return result;
     }
 
-    /** internal map containing the current values for all entries;
-     * for use where the caller wants to modify this directly and knows it is safe to do so */ 
+    /** Returns the internal map containing the current values for all entries;
+     * for use where the caller wants to modify this directly and knows it is safe to do so 
+     * <p>
+     * Accesses to the returned map must be synchronized on this bag if the 
+     * thread-safe behaviour is required. */ 
     public Map<String,Object> getAllConfigMutable() {
         if (live) {
             // TODO sealed no longer works as before, because `config` is the backing storage map.
             // Therefore returning it is dangerous! Even if we were to replace our field with an immutable copy,
             // the underlying datagrid's map would still be modifiable. We need a way to switch the returned
             // value's behaviour to sealable (i.e. wrapping the returned map).
-            return (sealed) ? Collections.unmodifiableMap(config) : config;
+            return (sealed) ? MutableMap.copyOf(config).asUnmodifiable() : config;
         } else {
             return config;
         }
@@ -161,12 +198,15 @@ public class ConfigBag {
 
     /** current values for all entries which have not yet been used 
      * @return non-modifiable map of strings to object */
-    public Map<String,Object> getUnusedConfig() {
-        return Collections.unmodifiableMap(unusedConfig);
+    public synchronized Map<String,Object> getUnusedConfig() {
+        return MutableMap.copyOf(unusedConfig).asUnmodifiable();
     }
 
-    /** internal map containing the current values for all entries which have not yet been used;
-     * for use where the caller wants to modify this directly and knows it is safe to do so */
+    /** Returns the internal map containing the current values for all entries which have not yet been used;
+     * for use where the caller wants to modify this directly and knows it is safe to do so 
+     * <p>
+     * Accesses to the returned map must be synchronized on this bag if the 
+     * thread-safe behaviour is required. */ 
     public Map<String,Object> getUnusedConfigMutable() {
         return unusedConfig;
     }
@@ -191,7 +231,7 @@ public class ConfigBag {
         return putIfAbsent(MutableMap.of(key, value));
     }
 
-    public ConfigBag putIfAbsent(Map<?, ?> propertiesToSet) {
+    public synchronized ConfigBag putIfAbsent(Map<?, ?> propertiesToSet) {
         if (propertiesToSet==null)
             return this;
         for (Map.Entry<?, ?> entry: propertiesToSet.entrySet()) {
@@ -242,7 +282,7 @@ public class ConfigBag {
         return this;
     }
     
-    protected void putAsStringKey(Object key, Object value) {
+    protected synchronized void putAsStringKey(Object key, Object value) {
         if (key instanceof HasConfigKey<?>) key = ((HasConfigKey<?>)key).getConfigKey();
         if (key instanceof ConfigKey<?>) key = ((ConfigKey<?>)key).getName();
         if (key instanceof String) {
@@ -262,7 +302,7 @@ public class ConfigBag {
     /** recommended to use {@link #put(ConfigKey, Object)} but there are times
      * (e.g. when copying a map) where we want to put a string key directly 
      */
-    public Object putStringKey(String key, Object value) {
+    public synchronized Object putStringKey(String key, Object value) {
         if (sealed) 
             throw new IllegalStateException("Cannot insert "+key+"="+value+": this config bag has been sealed and is now immutable.");
         boolean isNew = !config.containsKey(key);
@@ -285,14 +325,14 @@ public class ConfigBag {
     }
 
     public boolean containsKey(HasConfigKey<?> key) {
-        return config.containsKey(key.getConfigKey());
+        return containsKey(key.getConfigKey());
     }
 
     public boolean containsKey(ConfigKey<?> key) {
-        return config.containsKey(key.getName());
+        return containsKey(key.getName());
     }
 
-    public boolean containsKey(String key) {
+    public synchronized boolean containsKey(String key) {
         return config.containsKey(key);
     }
 
@@ -318,7 +358,7 @@ public class ConfigBag {
     }
 
     /** returns the first key in the list for which a value is explicitly set, then defaulting to defaulting value of preferred key */
-    public <T> T getFirst(ConfigKey<T> preferredKey, ConfigKey<T> ...otherCurrentKeysInOrderOfPreference) {
+    public synchronized <T> T getFirst(ConfigKey<T> preferredKey, ConfigKey<T> ...otherCurrentKeysInOrderOfPreference) {
         if (containsKey(preferredKey)) 
             return get(preferredKey);
         for (ConfigKey<T> key: otherCurrentKeysInOrderOfPreference) {
@@ -336,7 +376,7 @@ public class ConfigBag {
     /** returns the value for the first key in the list for which a value is set,
      * warning if any of the deprecated keys have a value which is different to that set on the first set current key
      * (including warning if a deprecated key has a value but no current key does) */
-    public Object getWithDeprecation(ConfigKey<?>[] currentKeysInOrderOfPreference, ConfigKey<?> ...deprecatedKeys) {
+    public synchronized Object getWithDeprecation(ConfigKey<?>[] currentKeysInOrderOfPreference, ConfigKey<?> ...deprecatedKeys) {
         // Get preferred key (or null)
         ConfigKey<?> preferredKeyProvidingValue = null;
         Object result = null;
@@ -399,10 +439,8 @@ public class ConfigBag {
     protected <T> T get(ConfigKey<T> key, boolean markUsed) {
         // TODO for now, no evaluation -- maps / closure content / other smart (self-extracting) keys are NOT supported
         // (need a clean way to inject that behaviour, as well as desired TypeCoercions)
-        if (config.containsKey(key.getName()))
-            return coerceFirstNonNullKeyValue(key, getStringKey(key.getName(), markUsed));
-        
-        return coerceFirstNonNullKeyValue(key);
+        // this method, and the coercion, is not synchronized, nor does it need to be, because the "get" is synchronized. 
+        return coerceFirstNonNullKeyValue(key, getStringKey(key.getName(), markUsed));
     }
 
     /** returns the first non-null value to be the type indicated by the key, or the keys default value if no non-null values are supplied */
@@ -415,7 +453,7 @@ public class ConfigBag {
     protected Object getStringKey(String key, boolean markUsed) {
         return getStringKeyMaybe(key, markUsed).orNull();
     }
-    protected Maybe<Object> getStringKeyMaybe(String key, boolean markUsed) {
+    protected synchronized Maybe<Object> getStringKeyMaybe(String key, boolean markUsed) {
         if (config.containsKey(key)) {
             if (markUsed) markUsed(key);
             return Maybe.of(config.get(key));
@@ -424,11 +462,11 @@ public class ConfigBag {
     }
 
     /** indicates that a string key in the config map has been accessed */
-    public void markUsed(String key) {
+    public synchronized void markUsed(String key) {
         unusedConfig.remove(key);
     }
 
-    public void clear() {
+    public synchronized void clear() {
         if (sealed) 
             throw new IllegalStateException("Cannot clear this config bag has been sealed and is now immutable.");
         config.clear();
@@ -440,7 +478,7 @@ public class ConfigBag {
         return this;
     }
 
-    public void remove(ConfigKey<?> key) {
+    public synchronized void remove(ConfigKey<?> key) {
         remove(key.getName());
     }
 
@@ -449,7 +487,7 @@ public class ConfigBag {
         return this;
     }
 
-    public void remove(String key) {
+    public synchronized void remove(String key) {
         if (sealed) 
             throw new IllegalStateException("Cannot remove "+key+": this config bag has been sealed and is now immutable.");
         config.remove(key);
@@ -457,6 +495,28 @@ public class ConfigBag {
     }
 
     public ConfigBag copy(ConfigBag other) {
+        // ensure locks are taken in a canonical order to prevent deadlock
+        if (other==null) {
+            synchronized (this) {
+                return copyWhileSynched(other);
+            }
+        }
+        if (System.identityHashCode(other) < System.identityHashCode(this)) {
+            synchronized (other) {
+                synchronized (this) {
+                    return copyWhileSynched(other);
+                }
+            }
+        } else {
+            synchronized (this) {
+                synchronized (other) {
+                    return copyWhileSynched(other);
+                }
+            }
+        }
+    }
+    
+    protected ConfigBag copyWhileSynched(ConfigBag other) {
         if (sealed) 
             throw new IllegalStateException("Cannot copy "+other+" to "+this+": this config bag has been sealed and is now immutable.");
         putAll(other.getAllConfig());
@@ -465,11 +525,11 @@ public class ConfigBag {
         return this;
     }
 
-    public int size() {
+    public synchronized int size() {
         return config.size();
     }
     
-    public boolean isEmpty() {
+    public synchronized boolean isEmpty() {
         return config.isEmpty();
     }
     
@@ -479,7 +539,7 @@ public class ConfigBag {
         return this;
     }
 
-    public boolean isUnused(ConfigKey<?> key) {
+    public synchronized boolean isUnused(ConfigKey<?> key) {
         return unusedConfig.containsKey(key.getName());
     }
     
@@ -499,6 +559,8 @@ public class ConfigBag {
         return this;
     }
 
+    // TODO why have both this and mutable
+    /** @see #getAllConfigMutable() */
     public Map<String, Object> getAllConfigRaw() {
         return getAllConfigMutable();
     }

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/5e34f950/core/src/test/java/brooklyn/util/config/ConfigBagTest.java
----------------------------------------------------------------------
diff --git a/core/src/test/java/brooklyn/util/config/ConfigBagTest.java b/core/src/test/java/brooklyn/util/config/ConfigBagTest.java
new file mode 100644
index 0000000..93cf6ed
--- /dev/null
+++ b/core/src/test/java/brooklyn/util/config/ConfigBagTest.java
@@ -0,0 +1,191 @@
+/*
+ * 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 brooklyn.util.config;
+
+import static org.testng.Assert.assertEquals;
+
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import brooklyn.config.ConfigKey;
+import brooklyn.entity.basic.ConfigKeys;
+import brooklyn.util.collections.MutableList;
+import brooklyn.util.collections.MutableMap;
+import brooklyn.util.exceptions.Exceptions;
+import brooklyn.util.time.Duration;
+
+public class ConfigBagTest {
+
+    @SuppressWarnings("unused")
+    private static final Logger log = LoggerFactory.getLogger(ConfigBagTest.class);
+    
+    private static final ConfigKey<String> K1 = ConfigKeys.newStringConfigKey("k1");
+    private static final ConfigKey<String> K2 = ConfigKeys.newStringConfigKey("k2");
+    private static final ConfigKey<String> K3 = ConfigKeys.newStringConfigKey("k3");
+    
+    @Test
+    public void testPutAndGet() {
+        ConfigBag bag = ConfigBag.newInstance();
+        bag.put(K1, "v1");
+        assertEquals(bag.get(K1), "v1");
+    }
+    
+    @Test
+    public void testPutStringAndGet() {
+        ConfigBag bag = ConfigBag.newInstance();
+        bag.putAsStringKey(K1.getName(), "v1");
+        assertEquals(bag.get(K1), "v1");
+    }
+    
+    @Test
+    public void testUnused() {
+        ConfigBag bag = ConfigBag.newInstance();
+        bag.put(K1, "v1");
+        bag.put(K2, "v2a");
+        assertEquals(bag.get(K1), "v1");
+        assertEquals(bag.getUnusedConfig().size(), 1);
+        assertEquals(bag.peek(K2), "v2a");
+        assertEquals(bag.getUnusedConfig().size(), 1);
+        assertEquals(bag.get(K2), "v2a");
+        Assert.assertTrue(bag.getUnusedConfig().isEmpty());
+    }
+
+    @Test
+    public void testOrder() {
+        ConfigBag bag = ConfigBag.newInstance();
+        bag.put(K1, "v1");
+        bag.put(K2, "v2");
+        bag.put(K3, "v3");
+        Assert.assertEquals(MutableList.copyOf(bag.getAllConfig().keySet()), MutableList.of(K1.getName(), K2.getName(), K3.getName()));
+        Assert.assertEquals(MutableList.copyOf(bag.getAllConfig().values()), MutableList.of("v1", "v2", "v3"));
+    }
+        
+    @Test
+    public void testCopyOverwriteAndGet() {
+        ConfigBag bag1 = ConfigBag.newInstance();
+        bag1.put(K1, "v1");
+        bag1.put(K2, "v2a");
+        bag1.put(K3, "v3");
+        assertEquals(bag1.get(K1), "v1");
+        
+        ConfigBag bag2 = ConfigBag.newInstanceCopying(bag1).putAll(MutableMap.of(K2, "v2b"));
+        assertEquals(bag1.getUnusedConfig().size(), 2);
+        assertEquals(bag2.getUnusedConfig().size(), 2);
+        
+        assertEquals(bag2.get(K1), "v1");
+        assertEquals(bag1.get(K2), "v2a");
+        assertEquals(bag1.getUnusedConfig().size(), 1);
+        assertEquals(bag2.getUnusedConfig().size(), 2);
+        
+        assertEquals(bag2.get(K2), "v2b");
+        assertEquals(bag2.getUnusedConfig().size(), 1);
+        
+        assertEquals(bag2.get(K3), "v3");
+        assertEquals(bag2.getUnusedConfig().size(), 0);
+        assertEquals(bag1.getUnusedConfig().size(), 1);
+    }
+    
+    @Test
+    public void testCopyExtendingAndGet() {
+        ConfigBag bag1 = ConfigBag.newInstance();
+        bag1.put(K1, "v1");
+        bag1.put(K2, "v2a");
+        bag1.put(K3, "v3");
+        assertEquals(bag1.get(K1), "v1");
+        
+        ConfigBag bag2 = ConfigBag.newInstanceExtending(bag1, null).putAll(MutableMap.of(K2, "v2b"));
+        assertEquals(bag1.getUnusedConfig().size(), 2);
+        assertEquals(bag2.getUnusedConfig().size(), 2, "unused are: "+bag2.getUnusedConfig());
+        
+        assertEquals(bag2.get(K1), "v1");
+        assertEquals(bag1.get(K2), "v2a");
+        assertEquals(bag1.getUnusedConfig().size(), 1);
+        assertEquals(bag2.getUnusedConfig().size(), 2);
+        
+        assertEquals(bag2.get(K2), "v2b");
+        assertEquals(bag2.getUnusedConfig().size(), 1);
+        
+        assertEquals(bag2.get(K3), "v3");
+        assertEquals(bag2.getUnusedConfig().size(), 0);
+        // when extended, the difference is that parent is also marked
+        assertEquals(bag1.getUnusedConfig().size(), 0);
+    }
+
+    @Test
+    public void testConcurrent() throws InterruptedException {
+        ConfigBag bag = ConfigBag.newInstance();
+        bag.put(K1, "v1");
+        bag.put(K2, "v2");
+        bag.put(K3, "v3");
+        runConcurrentTest(bag, 10, Duration.millis(50));
+    }
+    
+    @Test(groups="Integration")
+    public void testConcurrentBig() throws InterruptedException {
+        ConfigBag bag = ConfigBag.newInstance();
+        bag.put(K1, "v1");
+        bag.put(K2, "v2");
+        bag.put(K3, "v3");
+        runConcurrentTest(bag, 20, Duration.seconds(5));
+    }
+    
+    private void runConcurrentTest(final ConfigBag bag, int numThreads, Duration time) throws InterruptedException {
+        List<Thread> threads = MutableList.of();
+        final Map<Thread,Exception> exceptions = new ConcurrentHashMap<Thread,Exception>();
+        final AtomicInteger successes = new AtomicInteger();
+        for (int i=0; i<numThreads; i++) {
+            Thread t = new Thread() {
+                @Override
+                public void run() {
+                    try {
+                        while (!interrupted()) {
+                            if (Math.random()<0.9)
+                                bag.put(ConfigKeys.newStringConfigKey("k"+((int)(10*Math.random()))), "v"+((int)(10*Math.random())));
+                            if (Math.random()<0.8)
+                                bag.get(ConfigKeys.newStringConfigKey("k"+((int)(10*Math.random()))));
+                            if (Math.random()<0.2)
+                                bag.copy(bag);
+                            if (Math.random()<0.6)
+                                bag.remove(ConfigKeys.newStringConfigKey("k"+((int)(10*Math.random()))));
+                            successes.incrementAndGet();
+                        }
+                    } catch (Exception e) {
+                        exceptions.put(Thread.currentThread(), e);
+                        Exceptions.propagateIfFatal(e);
+                    }
+                }
+            };
+            t.setName("ConfigBagTest-concurrent-thread-"+i);
+            threads.add(t);
+        }
+        for (Thread t: threads) t.start();
+        time.countdownTimer().waitForExpiry();
+        for (Thread t: threads) t.interrupt();
+        for (Thread t: threads) t.join();
+        Assert.assertTrue(exceptions.isEmpty(), "Got "+exceptions.size()+"/"+numThreads+" exceptions ("+successes.get()+" successful): "+exceptions);
+    }
+    
+}


[3/3] incubator-brooklyn git commit: This closes #589

Posted by he...@apache.org.
This closes #589


Project: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/commit/bf07fe1f
Tree: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/tree/bf07fe1f
Diff: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/diff/bf07fe1f

Branch: refs/heads/master
Commit: bf07fe1f06f979fef6b5161e72c9ae43fbc3a551
Parents: 04fc801 4cffb0a
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Authored: Sun Apr 12 20:11:37 2015 -0500
Committer: Alex Heneveld <al...@cloudsoftcorp.com>
Committed: Sun Apr 12 20:11:37 2015 -0500

----------------------------------------------------------------------
 .../java/brooklyn/util/config/ConfigBag.java    | 132 +++++++++----
 .../brooklyn/util/config/ConfigBagTest.java     | 191 +++++++++++++++++++
 2 files changed, 289 insertions(+), 34 deletions(-)
----------------------------------------------------------------------



[2/3] incubator-brooklyn git commit: addressed code review comments on making ConfigBag thread safe

Posted by he...@apache.org.
addressed code review comments on making ConfigBag thread safe


Project: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/commit/4cffb0ac
Tree: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/tree/4cffb0ac
Diff: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/diff/4cffb0ac

Branch: refs/heads/master
Commit: 4cffb0acde90713bfad090524cbd71b32a32cd68
Parents: 5e34f95
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Authored: Sun Apr 12 20:11:03 2015 -0500
Committer: Alex Heneveld <al...@cloudsoftcorp.com>
Committed: Sun Apr 12 20:11:03 2015 -0500

----------------------------------------------------------------------
 core/src/main/java/brooklyn/util/config/ConfigBag.java | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/4cffb0ac/core/src/main/java/brooklyn/util/config/ConfigBag.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/brooklyn/util/config/ConfigBag.java b/core/src/main/java/brooklyn/util/config/ConfigBag.java
index 220ec52..606e2fc 100644
--- a/core/src/main/java/brooklyn/util/config/ConfigBag.java
+++ b/core/src/main/java/brooklyn/util/config/ConfigBag.java
@@ -49,7 +49,8 @@ import com.google.common.collect.Sets;
  * although in some cases (such as setting fields from flags, or copying a map)
  * it may be necessary to mark things as used, or put, when only a string key is available.
  * <p>
- * This bag is order-preserving and thread-safe except where otherwise indicated.
+ * This bag is order-preserving and thread-safe except where otherwise indicated,
+ * currently by synching on this instance (but that behaviour may change).
  * <p>
  * @author alex
  */
@@ -104,14 +105,15 @@ public class ConfigBag {
      */
     @Beta
     public static ConfigBag newInstanceExtending(final ConfigBag parentBag) {
-        return new ConfigBagExtendingParent(parentBag).copy(parentBag);
+        return new ConfigBagExtendingParent(parentBag);
     }
 
     /** @see #newInstanceExtending(ConfigBag) */
-    public static class ConfigBagExtendingParent extends ConfigBag {
+    private static class ConfigBagExtendingParent extends ConfigBag {
         ConfigBag parentBag;
         private ConfigBagExtendingParent(ConfigBag parentBag) {
             this.parentBag = parentBag;
+            copy(parentBag);
         }
         @Override
         public void markUsed(String key) {