[GitHub] [accumulo] cshannon commented on a diff in pull request #3706: Avoid unnecessary copy of Hadoop config in getVolumeManagerConfiguration

["cshannon (via GitHub)" <gi...@apache.org>]

cshannon commented on code in PR #3706:
URL: https://github.com/apache/accumulo/pull/3706#discussion_r1297700836


##########
server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManagerImpl.java:
##########
@@ -387,17 +387,25 @@ public short getDefaultReplication(Path path) {
   private static Configuration getVolumeManagerConfiguration(AccumuloConfiguration conf,
       final Configuration hadoopConf, final String filesystemURI) {
 
-    final Configuration volumeConfig = new Configuration(hadoopConf);
-
-    conf.getAllPropertiesWithPrefixStripped(Property.INSTANCE_VOLUME_CONFIG_PREFIX).entrySet()
-        .stream().filter(e -> e.getKey().startsWith(filesystemURI + ".")).forEach(e -> {
-          String key = e.getKey().substring(filesystemURI.length() + 1);
-          String value = e.getValue();
-          log.info("Overriding property {} for volume {}", key, value, filesystemURI);
-          volumeConfig.set(key, value);
-        });
-
-    return volumeConfig;
+    Map<String,String> volumeHdfsConfigOverrides =
+        conf.getAllPropertiesWithPrefixStripped(Property.INSTANCE_VOLUME_CONFIG_PREFIX);
+
+    // Calling new Configuration(Configuration) will synchronize on the constructor parameter
+    // and can cause issues if many threads call this method concurrently.
+    if (!volumeHdfsConfigOverrides.isEmpty()) {
+      final Configuration volumeConfig = new Configuration(hadoopConf);
+
+      volumeHdfsConfigOverrides.entrySet().stream()
+          .filter(e -> e.getKey().startsWith(filesystemURI + ".")).forEach(e -> {

Review Comment:
   This works: 
   
   > I got it:
   > 
   > ```
   >     Map<String,String> volumeHdfsConfigOverrides = 
   >         conf.getAllPropertiesWithPrefixStripped(Property.INSTANCE_VOLUME_CONFIG_PREFIX).entrySet()
   >             .stream().filter(e -> e.getKey().startsWith(filesystemURI + "."))
   >             .collect(Collectors.toUnmodifiableMap(e -> e.getKey(), e -> e.getValue()));
   > ```
   
   I was just about to post it but a little different:
   
   ```
       Map<String,String> volumeHdfsConfigOverrides2 =
               conf.getAllPropertiesWithPrefixStripped(Property.INSTANCE_VOLUME_CONFIG_PREFIX).entrySet()
                       .stream().filter(e -> e.getKey().startsWith(filesystemURI + "."))
                       .collect(Collectors.toUnmodifiableMap(Entry::getKey, Entry::getValue));
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org