[incubator-uniffle] branch master updated: Fix potenial race condition when registering remote storage info (#481)

[zuston]

This is an automated email from the ASF dual-hosted git repository.

zuston pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-uniffle.git


The following commit(s) were added to refs/heads/master by this push:
     new 654d8c77 Fix potenial race condition when registering remote storage 
info (#481)
654d8c77 is described below

commit 654d8c77d491029d5c0c40911b35f4d9212adad0
Author: Junfan Zhang <zus...@apache.org>
AuthorDate: Mon Jan 16 13:56:37 2023 +0800

    Fix potenial race condition when registering remote storage info (#481)
    
    
    ### What changes were proposed in this pull request?
    
    1. Use the concurrentHashMap's computeIfAbsent to keep thread safe
    
    ### Why are the changes needed?
    1. To fix potential race condition when registering remote storage info
    
    ### Does this PR introduce _any_ user-facing change?
    No
    
    ### How was this patch tested?
    Existing UTs
---
 .../uniffle/server/storage/HdfsStorageManager.java      | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git 
a/server/src/main/java/org/apache/uniffle/server/storage/HdfsStorageManager.java
 
b/server/src/main/java/org/apache/uniffle/server/storage/HdfsStorageManager.java
index 3864f774..7c4af1d8 100644
--- 
a/server/src/main/java/org/apache/uniffle/server/storage/HdfsStorageManager.java
+++ 
b/server/src/main/java/org/apache/uniffle/server/storage/HdfsStorageManager.java
@@ -119,23 +119,20 @@ public class HdfsStorageManager extends 
SingleStorageManager {
   @Override
   public void registerRemoteStorage(String appId, RemoteStorageInfo 
remoteStorageInfo) {
     String remoteStorage = remoteStorageInfo.getPath();
-    Map<String, String> remoteStorageConf = remoteStorageInfo.getConfItems();
-    if (!pathToStorages.containsKey(remoteStorage)) {
+    pathToStorages.computeIfAbsent(remoteStorage, key -> {
+      Map<String, String> remoteStorageConf = remoteStorageInfo.getConfItems();
       Configuration remoteStorageHadoopConf = new Configuration(hadoopConf);
       if (remoteStorageConf != null && remoteStorageConf.size() > 0) {
         for (Map.Entry<String, String> entry : remoteStorageConf.entrySet()) {
           remoteStorageHadoopConf.setStrings(entry.getKey(), entry.getValue());
         }
       }
-      pathToStorages.putIfAbsent(remoteStorage, new HdfsStorage(remoteStorage, 
remoteStorageHadoopConf));
-      // registerRemoteStorage may be called in different threads,
-      // make sure metrics won't be created duplicated
-      // there shouldn't have performance issue because
-      // it will be called only few times according to the number of remote 
storage
-      String storageHost = pathToStorages.get(remoteStorage).getStorageHost();
+      HdfsStorage hdfsStorage = new HdfsStorage(remoteStorage, 
remoteStorageHadoopConf);
+      String storageHost = hdfsStorage.getStorageHost();
       ShuffleServerMetrics.addDynamicCounterForRemoteStorage(storageHost);
-    }
-    appIdToStorages.putIfAbsent(appId, pathToStorages.get(remoteStorage));
+      return hdfsStorage;
+    });
+    appIdToStorages.computeIfAbsent(appId, key -> 
pathToStorages.get(remoteStorage));
   }
 
   @Override