Change in asterixdb[master]: [ASTERIXDB-2091][HYR][CLUS] Fix unsafe concurrent access to ...

[Michael Blow (Code Review)]

Michael Blow has uploaded a new change for review.

  https://asterix-gerrit.ics.uci.edu/2007

Change subject: [ASTERIXDB-2091][HYR][CLUS] Fix unsafe concurrent access to 
config on NC registration
......................................................................

[ASTERIXDB-2091][HYR][CLUS] Fix unsafe concurrent access to config on NC 
registration

Change-Id: I399d26d128794a0a52eff419f59ca28d64b17375
---
M 
hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/config/ConfigManager.java
1 file changed, 11 insertions(+), 8 deletions(-)


  git pull ssh://asterix-gerrit.ics.uci.edu:29418/asterixdb 
refs/changes/07/2007/1

diff --git 
a/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/config/ConfigManager.java
 
b/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/config/ConfigManager.java
index 2e04e13..8675604 100644
--- 
a/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/config/ConfigManager.java
+++ 
b/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/config/ConfigManager.java
@@ -34,6 +34,7 @@
 import java.util.Set;
 import java.util.SortedMap;
 import java.util.TreeMap;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.function.BiConsumer;
 import java.util.function.Consumer;
 import java.util.function.Function;
@@ -66,19 +67,21 @@
     private static final Logger LOGGER = 
Logger.getLogger(ConfigManager.class.getName());
 
     private HashSet<IOption> registeredOptions = new HashSet<>();
-    private HashMap<IOption, Object> definedMap = new HashMap<>();
-    private HashMap<IOption, Object> defaultMap = new HashMap<>();
+    private ConcurrentHashMap<IOption, Object> definedMap = new 
ConcurrentHashMap<>();
+    private ConcurrentHashMap<IOption, Object> defaultMap = new 
ConcurrentHashMap<>();
     private CompositeMap<IOption, Object> configurationMap = new 
CompositeMap<>(definedMap, defaultMap,
             new NoOpMapMutator());
     private EnumMap<Section, Map<String, IOption>> sectionMap = new 
EnumMap<>(Section.class);
-    private TreeMap<String, Map<IOption, Object>> nodeSpecificMap = new 
TreeMap<>();
+    @SuppressWarnings("squid:S1948") // TreeMap is serializable, and therefore 
so is its synchronized map
+    private Map<String, ConcurrentHashMap<IOption, Object>> nodeSpecificMap = 
Collections
+            .synchronizedMap(new TreeMap<>());
     private transient ArrayListValuedHashMap<IOption, IConfigSetter> 
optionSetters = new ArrayListValuedHashMap<>();
     private final String[] args;
     private ConfigManagerApplicationConfig appConfig = new 
ConfigManagerApplicationConfig(this);
     private Set<String> allSections = new HashSet<>();
     private transient Collection<Consumer<List<String>>> argListeners = new 
ArrayList<>();
     private transient Collection<IOption> iniPointerOptions = new 
ArrayList<>();
-    private transient Collection<Section> cmdLineSections = new ArrayList<>();;
+    private transient Collection<Section> cmdLineSections = new ArrayList<>();
     private transient OptionHandlerFilter usageFilter;
     private transient SortedMap<Integer, List<IConfigurator>> configurators = 
new TreeMap<>();
     private boolean configured;
@@ -164,9 +167,9 @@
         nodeSpecificMap.computeIfAbsent(nodeId, this::createNodeSpecificMap);
     }
 
-    private Map<IOption, Object> createNodeSpecificMap(String nodeId) {
+    private ConcurrentHashMap<IOption, Object> createNodeSpecificMap(String 
nodeId) {
         LOGGER.fine("createNodeSpecificMap: " + nodeId);
-        return Collections.synchronizedMap(new HashMap<>());
+        return new ConcurrentHashMap<>();
     }
 
     @Override
@@ -347,7 +350,7 @@
             if (entry.getKey() == Section.NC) {
                 entry.getValue().values().forEach(option -> getNodeNames()
                         .forEach(node -> 
getOrDefault(getNodeEffectiveMap(node), option, node)));
-                for (Map.Entry<String, Map<IOption, Object>> nodeMap : 
nodeSpecificMap.entrySet()) {
+                for (Map.Entry<String, ConcurrentHashMap<IOption, Object>> 
nodeMap : nodeSpecificMap.entrySet()) {
                     entry.getValue().values()
                             .forEach(option -> getOrDefault(
                                     new CompositeMap<>(nodeMap.getValue(), 
definedMap, new NoOpMapMutator()), option,
@@ -455,7 +458,7 @@
                 ini.add(option.section().sectionName(), option.ini(), 
option.type().serializeToIni(entry.getValue()));
             }
         }
-        for (Map.Entry<String, Map<IOption, Object>> nodeMapEntry : 
nodeSpecificMap.entrySet()) {
+        for (Map.Entry<String, ConcurrentHashMap<IOption, Object>> 
nodeMapEntry : nodeSpecificMap.entrySet()) {
             String section = Section.NC.sectionName() + "/" + 
nodeMapEntry.getKey();
             for (Map.Entry<IOption, Object> entry : 
nodeMapEntry.getValue().entrySet()) {
                 if (entry.getValue() != null) {

-- 
To view, visit https://asterix-gerrit.ics.uci.edu/2007
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I399d26d128794a0a52eff419f59ca28d64b17375
Gerrit-PatchSet: 1
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Michael Blow <mb...@apache.org>