Make ModuleConfig immutable 61/29761/5
authorTom Pantelis <tpanteli@brocade.com>
Mon, 16 Nov 2015 10:49:05 +0000 (05:49 -0500)
committerGerrit Code Review <gerrit@opendaylight.org>
Wed, 25 Nov 2015 18:33:03 +0000 (18:33 +0000)
The ModuleConfig class has mutator methods for
ModuleShardConfigProviders to initially construct instances. However
once supplied to the CondifuratonImpl they are intended to be immutable
yet the mutator methods expose loopholes around it. Therefore I added a
Builder to ModuleConfig and made ModuleConfig truly immutable.

Change-Id: I0b8070ff3db1563427a6a70ff174053b2a66feca
Signed-off-by: Tom Pantelis <tpanteli@brocade.com>
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/config/ConfigurationImpl.java
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/config/FileModuleShardConfigProvider.java
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/config/ModuleConfig.java
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/config/ModuleShardConfigProvider.java
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/config/ShardConfig.java
opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/config/EmptyModuleShardConfigProvider.java
opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/entityownership/DistributedEntityOwnershipServiceTest.java
opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/utils/MockConfiguration.java

index b7bcc59f2ffd9e6c4f0a3eff0a8dc0afbf69e164..d88301c308d39a7944d3d058ed4cb5825c5d60f3 100644 (file)
@@ -35,7 +35,12 @@ public class ConfigurationImpl implements Configuration {
     }
 
     public ConfigurationImpl(final ModuleShardConfigProvider provider) {
-        this.moduleConfigMap = ImmutableMap.copyOf(provider.retrieveModuleConfigs(this));
+        ImmutableMap.Builder<String, ModuleConfig> mapBuilder = ImmutableMap.builder();
+        for(Map.Entry<String, ModuleConfig.Builder> e: provider.retrieveModuleConfigs(this).entrySet()) {
+            mapBuilder.put(e.getKey(), e.getValue().build());
+        }
+
+        this.moduleConfigMap = mapBuilder.build();
 
         this.allShardNames = createAllShardNames(moduleConfigMap.values());
         this.namespaceToModuleName = createNamespaceToModuleName(moduleConfigMap.values());
@@ -135,14 +140,12 @@ public class ConfigurationImpl implements Configuration {
     public synchronized void addModuleShardConfiguration(ModuleShardConfiguration config) {
         Preconditions.checkNotNull(config, "ModuleShardConfiguration should not be null");
 
-        ModuleConfig moduleConfig = new ModuleConfig(config.getModuleName());
-        moduleConfig.setNameSpace(config.getNamespace().toASCIIString());
-        moduleConfig.setShardStrategy(createShardStrategy(config.getModuleName(), config.getShardStrategyName()));
-
-        moduleConfig.addShardConfig(config.getShardName(), ImmutableSet.copyOf(config.getShardMemberNames()));
+        ModuleConfig moduleConfig = ModuleConfig.builder(config.getModuleName()).
+                nameSpace(config.getNamespace().toASCIIString()).
+                shardStrategy(createShardStrategy(config.getModuleName(), config.getShardStrategyName())).
+                shardConfig(config.getShardName(), config.getShardMemberNames()).build();
 
-        moduleConfigMap = ImmutableMap.<String, ModuleConfig>builder().putAll(moduleConfigMap).
-                put(config.getModuleName(), moduleConfig).build();
+        updateModuleConfigMap(moduleConfig);
 
         namespaceToModuleName = ImmutableMap.<String, String>builder().putAll(namespaceToModuleName).
                 put(moduleConfig.getNameSpace(), moduleConfig.getName()).build();
@@ -167,11 +170,9 @@ public class ConfigurationImpl implements Configuration {
         for(ModuleConfig moduleConfig: moduleConfigMap.values()) {
             ShardConfig shardConfig = moduleConfig.getShardConfig(shardName);
             if(shardConfig != null) {
-                ModuleConfig newModuleConfig = new ModuleConfig(moduleConfig);
-                Set<String> replica = new HashSet<>(shardConfig.getReplicas());
-                replica.add(newMemberName);
-                newModuleConfig.addShardConfig(shardName, ImmutableSet.copyOf(replica));
-                updateModuleConfigMap(newModuleConfig);
+                Set<String> replicas = new HashSet<>(shardConfig.getReplicas());
+                replicas.add(newMemberName);
+                updateModuleConfigMap(ModuleConfig.builder(moduleConfig).shardConfig(shardName, replicas).build());
                 return;
             }
         }
@@ -185,20 +186,17 @@ public class ConfigurationImpl implements Configuration {
         for(ModuleConfig moduleConfig: moduleConfigMap.values()) {
             ShardConfig shardConfig = moduleConfig.getShardConfig(shardName);
             if(shardConfig != null) {
-                ModuleConfig newModuleConfig = new ModuleConfig(moduleConfig);
-                Set<String> replica = new HashSet<>(shardConfig.getReplicas());
-                replica.remove(newMemberName);
-                newModuleConfig.addShardConfig(shardName, ImmutableSet.copyOf(replica));
-                updateModuleConfigMap(newModuleConfig);
+                Set<String> replicas = new HashSet<>(shardConfig.getReplicas());
+                replicas.remove(newMemberName);
+                updateModuleConfigMap(ModuleConfig.builder(moduleConfig).shardConfig(shardName, replicas).build());
                 return;
             }
         }
     }
 
     private void updateModuleConfigMap(ModuleConfig moduleConfig) {
-        HashMap<String, ModuleConfig> newModuleConfigMap = new HashMap<>(moduleConfigMap);
+        Map<String, ModuleConfig> newModuleConfigMap = new HashMap<>(moduleConfigMap);
         newModuleConfigMap.put(moduleConfig.getName(), moduleConfig);
-        moduleConfigMap = ImmutableMap.<String, ModuleConfig>builder().putAll(newModuleConfigMap).build();
-        return;
+        moduleConfigMap = ImmutableMap.copyOf(newModuleConfigMap);
     }
 }
index 2651744d9af255a421b0627bb64fc29496c39a05..80fc09c97a1a57ddadbe242c41c3dc3cb46e3ea0 100644 (file)
@@ -7,7 +7,6 @@
  */
 package org.opendaylight.controller.cluster.datastore.config;
 
-import com.google.common.collect.ImmutableSet;
 import com.typesafe.config.Config;
 import com.typesafe.config.ConfigFactory;
 import com.typesafe.config.ConfigObject;
@@ -36,7 +35,7 @@ public class FileModuleShardConfigProvider implements ModuleShardConfigProvider
     }
 
     @Override
-    public Map<String, ModuleConfig> retrieveModuleConfigs(Configuration configuration) {
+    public Map<String, ModuleConfig.Builder> retrieveModuleConfigs(Configuration configuration) {
         File moduleShardsFile = new File("./configuration/initial/" + moduleShardsConfigPath);
         File modulesFile = new File("./configuration/initial/" + modulesConfigPath);
 
@@ -58,13 +57,13 @@ public class FileModuleShardConfigProvider implements ModuleShardConfigProvider
             modulesConfig = ConfigFactory.load(modulesConfigPath);
         }
 
-        Map<String, ModuleConfig> moduleConfigMap = readModuleShardsConfig(moduleShardsConfig);
+        Map<String, ModuleConfig.Builder> moduleConfigMap = readModuleShardsConfig(moduleShardsConfig);
         readModulesConfig(modulesConfig, moduleConfigMap, configuration);
 
         return moduleConfigMap;
     }
 
-    private static void readModulesConfig(final Config modulesConfig, Map<String, ModuleConfig> moduleConfigMap,
+    private static void readModulesConfig(final Config modulesConfig, Map<String, ModuleConfig.Builder> moduleConfigMap,
             Configuration configuration) {
         List<? extends ConfigObject> modulesConfigObjectList = modulesConfig.getObjectList("modules");
 
@@ -72,26 +71,26 @@ public class FileModuleShardConfigProvider implements ModuleShardConfigProvider
             ConfigObjectWrapper w = new ConfigObjectWrapper(o);
 
             String moduleName = w.stringValue("name");
-            ModuleConfig moduleConfig = moduleConfigMap.get(moduleName);
-            if(moduleConfig == null) {
-                moduleConfig = new ModuleConfig(moduleName);
-                moduleConfigMap.put(moduleName, moduleConfig);
+            ModuleConfig.Builder builder = moduleConfigMap.get(moduleName);
+            if(builder == null) {
+                builder = ModuleConfig.builder(moduleName);
+                moduleConfigMap.put(moduleName, builder);
             }
 
-            moduleConfig.setNameSpace(w.stringValue("namespace"));
-            moduleConfig.setShardStrategy(ShardStrategyFactory.newShardStrategyInstance(moduleName,
+            builder.nameSpace(w.stringValue("namespace"));
+            builder.shardStrategy(ShardStrategyFactory.newShardStrategyInstance(moduleName,
                     w.stringValue("shard-strategy"), configuration));
         }
     }
 
-    private static Map<String, ModuleConfig> readModuleShardsConfig(final Config moduleShardsConfig) {
+    private static Map<String, ModuleConfig.Builder> readModuleShardsConfig(final Config moduleShardsConfig) {
         List<? extends ConfigObject> moduleShardsConfigObjectList =
             moduleShardsConfig.getObjectList("module-shards");
 
-        Map<String, ModuleConfig> moduleConfigMap = new HashMap<>();
+        Map<String, ModuleConfig.Builder> moduleConfigMap = new HashMap<>();
         for(ConfigObject moduleShardConfigObject : moduleShardsConfigObjectList){
             String moduleName = moduleShardConfigObject.get("name").unwrapped().toString();
-            ModuleConfig moduleConfig = new ModuleConfig(moduleName);
+            ModuleConfig.Builder builder = ModuleConfig.builder(moduleName);
 
             List<? extends ConfigObject> shardsConfigObjectList =
                 moduleShardConfigObject.toConfig().getObjectList("shards");
@@ -99,10 +98,10 @@ public class FileModuleShardConfigProvider implements ModuleShardConfigProvider
             for(ConfigObject shard : shardsConfigObjectList){
                 String shardName = shard.get("name").unwrapped().toString();
                 List<String> replicas = shard.toConfig().getStringList("replicas");
-                moduleConfig.addShardConfig(shardName, ImmutableSet.copyOf(replicas));
+                builder.shardConfig(shardName, replicas);
             }
 
-            moduleConfigMap.put(moduleName, moduleConfig);
+            moduleConfigMap.put(moduleName, builder);
         }
 
         return moduleConfigMap;
index b54946774d2a4bc7cd266bf93370a91f5ccf0d77..97be5622a423ed307facecc9e21d70abde8f4cca 100644 (file)
@@ -7,11 +7,13 @@
  */
 package org.opendaylight.controller.cluster.datastore.config;
 
-import com.google.common.collect.ImmutableSet;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableMap;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.Map;
-import java.util.Set;
+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
 import org.opendaylight.controller.cluster.datastore.shardstrategy.ShardStrategy;
 
 /**
@@ -21,61 +23,98 @@ import org.opendaylight.controller.cluster.datastore.shardstrategy.ShardStrategy
  */
 public class ModuleConfig {
     private final String name;
-    private String nameSpace;
-    private ShardStrategy shardStrategy;
-    private final Map<String, ShardConfig> shardConfigs = new HashMap<>();
+    private final String nameSpace;
+    private final ShardStrategy shardStrategy;
+    private final Map<String, ShardConfig> shardConfigs;
 
-    public ModuleConfig(final String name) {
+    private ModuleConfig(String name, String nameSpace, ShardStrategy shardStrategy,
+            Map<String, ShardConfig> shardConfigs) {
         this.name = name;
+        this.nameSpace = nameSpace;
+        this.shardStrategy = shardStrategy;
+        this.shardConfigs = shardConfigs;
     }
 
-    public ModuleConfig(ModuleConfig moduleConfig) {
-        this.name = moduleConfig.getName();
-        this.nameSpace = moduleConfig.getNameSpace();
-        this.shardStrategy = moduleConfig.getShardStrategy();
-        for (ShardConfig shardConfig : moduleConfig.getShardConfigs()) {
-            shardConfigs.put(shardConfig.getName(), new ShardConfig(shardConfig.getName(),
-                ImmutableSet.copyOf(shardConfig.getReplicas())));
-        }
-    }
-
+    @Nonnull
     public String getName() {
         return name;
     }
 
+    @Nullable
     public String getNameSpace() {
         return nameSpace;
     }
 
+    @Nullable
     public ShardStrategy getShardStrategy() {
         return shardStrategy;
     }
 
+    @Nullable
     public ShardConfig getShardConfig(String name) {
         return shardConfigs.get(name);
     }
 
+    @Nonnull
     public Collection<ShardConfig> getShardConfigs() {
         return shardConfigs.values();
     }
 
+    @Nonnull
     public Collection<String> getShardNames() {
         return shardConfigs.keySet();
     }
 
-    public void addShardConfig(String name, Set<String> replicas) {
-        shardConfigs.put(name, new ShardConfig(name, replicas));
+    public static Builder builder(String name) {
+        return new Builder(name);
     }
 
-    public void setNameSpace(String nameSpace) {
-        this.nameSpace = nameSpace;
+    public static Builder builder(ModuleConfig moduleConfig) {
+        return new Builder(moduleConfig);
     }
 
-    public void setShardStrategy(ShardStrategy shardStrategy) {
-        this.shardStrategy = shardStrategy;
-    }
+    public static class Builder {
+        private String name;
+        private String nameSpace;
+        private ShardStrategy shardStrategy;
+        private final Map<String, ShardConfig> shardConfigs = new HashMap<>();
 
-    public ShardConfig removeShardConfig(String name) {
-        return shardConfigs.remove(name);
+        private Builder(String name) {
+            this.name = name;
+        }
+
+        private Builder(ModuleConfig moduleConfig) {
+            this.name = moduleConfig.getName();
+            this.nameSpace = moduleConfig.getNameSpace();
+            this.shardStrategy = moduleConfig.getShardStrategy();
+            for (ShardConfig shardConfig : moduleConfig.getShardConfigs()) {
+                shardConfigs.put(shardConfig.getName(), shardConfig);
+            }
+        }
+
+        public Builder name(String name) {
+            this.name = name;
+            return this;
+        }
+
+        public Builder nameSpace(String nameSpace) {
+            this.nameSpace = nameSpace;
+            return this;
+        }
+
+        public Builder shardStrategy(ShardStrategy shardStrategy) {
+            this.shardStrategy = shardStrategy;
+            return this;
+        }
+
+        public Builder shardConfig(String name, Collection<String> replicas) {
+            shardConfigs.put(name, new ShardConfig(name, replicas));
+            return this;
+        }
+
+        public ModuleConfig build() {
+            return new ModuleConfig(Preconditions.checkNotNull(name), nameSpace, shardStrategy,
+                    ImmutableMap.copyOf(shardConfigs));
+        }
     }
 }
index 6ff708163a5247228d637c54ca37359a12dd8eb1..1fc31f52ff65ad8d73c684e78ef6c9a9d9267a3a 100644 (file)
@@ -17,7 +17,7 @@ import javax.annotation.Nonnull;
  */
 public interface ModuleShardConfigProvider {
     /**
-     * Returns a Map of ModuleConfig instances keyed by module name.
+     * Returns a Map of ModuleConfig Builder instances keyed by module name.
      */
-    @Nonnull Map<String, ModuleConfig> retrieveModuleConfigs(@Nonnull Configuration configuration);
+    @Nonnull Map<String, ModuleConfig.Builder> retrieveModuleConfigs(@Nonnull Configuration configuration);
 }
index 0eb02aa2f8c2f33daadb7c762472912eeaafde45..e5ccab64608fc8c006eab80343c8cce5fc06c912 100644 (file)
@@ -7,7 +7,11 @@
  */
 package org.opendaylight.controller.cluster.datastore.config;
 
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableSet;
+import java.util.Collection;
 import java.util.Set;
+import javax.annotation.Nonnull;
 
 /**
  * Encapsulated configuration for a shard.
@@ -16,15 +20,17 @@ public class ShardConfig {
     private final String name;
     private final Set<String> replicas;
 
-    public ShardConfig(final String name, final Set<String> replicas) {
-        this.name = name;
-        this.replicas = replicas;
+    public ShardConfig(@Nonnull final String name, @Nonnull final Collection<String> replicas) {
+        this.name = Preconditions.checkNotNull(name);
+        this.replicas = ImmutableSet.copyOf(Preconditions.checkNotNull(replicas));
     }
 
+    @Nonnull
     public String getName() {
         return name;
     }
 
+    @Nonnull
     public Set<String> getReplicas() {
         return replicas;
     }
index 4f8ec359bf2a7e7f0347feed0c4e59d0635e2538..71fe923cdffe0b51aff4c1f199cd2911d12ef73c 100644 (file)
@@ -18,7 +18,7 @@ import java.util.Map;
 public class EmptyModuleShardConfigProvider implements ModuleShardConfigProvider {
 
     @Override
-    public Map<String, ModuleConfig> retrieveModuleConfigs(Configuration configuration) {
+    public Map<String, ModuleConfig.Builder> retrieveModuleConfigs(Configuration configuration) {
         return Collections.emptyMap();
     }
 }
index 100e6dec01250e510b1aa4ac24821fd43243a37c..cedd942219ba4e9d34b72d3405f5edc0009134e7 100644 (file)
@@ -29,8 +29,6 @@ import com.google.common.base.Optional;
 import com.google.common.collect.Sets;
 import com.google.common.util.concurrent.Uninterruptibles;
 import java.util.Collection;
-import java.util.Collections;
-import java.util.Map;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicReference;
@@ -44,8 +42,7 @@ import org.opendaylight.controller.cluster.datastore.DistributedDataStore;
 import org.opendaylight.controller.cluster.datastore.ShardDataTree;
 import org.opendaylight.controller.cluster.datastore.config.Configuration;
 import org.opendaylight.controller.cluster.datastore.config.ConfigurationImpl;
-import org.opendaylight.controller.cluster.datastore.config.ModuleConfig;
-import org.opendaylight.controller.cluster.datastore.config.ModuleShardConfigProvider;
+import org.opendaylight.controller.cluster.datastore.config.EmptyModuleShardConfigProvider;
 import org.opendaylight.controller.cluster.datastore.entityownership.messages.RegisterCandidateLocal;
 import org.opendaylight.controller.cluster.datastore.entityownership.messages.RegisterListenerLocal;
 import org.opendaylight.controller.cluster.datastore.entityownership.messages.UnregisterCandidateLocal;
@@ -88,13 +85,7 @@ public class DistributedEntityOwnershipServiceTest extends AbstractEntityOwnersh
         DatastoreContext datastoreContext = DatastoreContext.newBuilder().dataStoreType(dataStoreType).
                 shardInitializationTimeout(10, TimeUnit.SECONDS).build();
 
-        ModuleShardConfigProvider configProvider = new ModuleShardConfigProvider() {
-            @Override
-            public Map<String, ModuleConfig> retrieveModuleConfigs(Configuration configuration) {
-                return Collections.emptyMap();
-            }
-        };
-        Configuration configuration = new ConfigurationImpl(configProvider) {
+        Configuration configuration = new ConfigurationImpl(new EmptyModuleShardConfigProvider()) {
             @Override
             public Collection<String> getUniqueMemberNamesForAllShards() {
                 return Sets.newHashSet("member-1");
index 77097352489fa97a153761bb16817fcdd9fd6521..24d8cfd406e3a5288c949b977ff4bcfca5cc3c78 100644 (file)
@@ -8,7 +8,6 @@
 
 package org.opendaylight.controller.cluster.datastore.utils;
 
-import com.google.common.collect.Sets;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
@@ -27,13 +26,11 @@ public class MockConfiguration extends ConfigurationImpl {
     public MockConfiguration(final Map<String, List<String>> shardMembers) {
         super(new ModuleShardConfigProvider() {
             @Override
-            public Map<String, ModuleConfig> retrieveModuleConfigs(Configuration configuration) {
-                Map<String, ModuleConfig> retMap = new HashMap<String, ModuleConfig>();
+            public Map<String, ModuleConfig.Builder> retrieveModuleConfigs(Configuration configuration) {
+                Map<String, ModuleConfig.Builder> retMap = new HashMap<>();
                 for(Map.Entry<String, List<String>> e : shardMembers.entrySet()) {
                     String shardName = e.getKey();
-                    ModuleConfig mc = new ModuleConfig(shardName);
-                    mc.addShardConfig(shardName, Sets.newHashSet(e.getValue()));
-                    retMap.put(mc.getName(), mc);
+                    retMap.put(shardName, ModuleConfig.builder(shardName).shardConfig(shardName, e.getValue()));
                 }
 
                 return retMap;