(incubator-uniffle) branch master updated: [#1626] fix(server): Remove the meaningless eventOfUnderStorageManagers cache (#1627)

[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 a7a0b4ce4 [#1626] fix(server): Remove the meaningless 
eventOfUnderStorageManagers cache (#1627)
a7a0b4ce4 is described below

commit a7a0b4ce41a213f8f3e05a1e1198c41857706229
Author: RickyMa <rick...@tencent.com>
AuthorDate: Wed Apr 10 14:15:17 2024 +0800

    [#1626] fix(server): Remove the meaningless eventOfUnderStorageManagers 
cache (#1627)
    
    ### What changes were proposed in this pull request?
    
    Remove the meaningless eventOfUnderStorageManagers.
    
    ### Why are the changes needed?
    
    Fix https://github.com/apache/incubator-uniffle/issues/1626 & 
https://github.com/apache/incubator-uniffle/issues/1620.
    It's also a follow-up PR for 
https://github.com/apache/incubator-uniffle/pull/383.
    
    This cache only makes sense when the event retries after a failure. 
However, after the event fails, it is not appropriate to continue taking the 
original storageManager from the cache(because events usually fail due to high 
IO pressure or disk damage or disk full). In this case, the cache seems to be 
meaningless, so there is a contradiction here, we should remove it.
    
    ### Does this PR introduce _any_ user-facing change?
    
    No.
    
    ### How was this patch tested?
    
    Tested in our env
---
 .../java/org/apache/uniffle/server/ShuffleServerConf.java |  7 -------
 .../uniffle/server/storage/HybridStorageManager.java      | 15 +--------------
 2 files changed, 1 insertion(+), 21 deletions(-)

diff --git 
a/server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java 
b/server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java
index bd71c3bc4..a7b140003 100644
--- a/server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java
+++ b/server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java
@@ -386,13 +386,6 @@ public class ShuffleServerConf extends RssBaseConf {
           .defaultValue(128 * 1024 * 1024L)
           .withDescription("The threshold of single shuffle buffer flush");
 
-  public static final ConfigOption<Long> STORAGEMANAGER_CACHE_TIMEOUT =
-      
ConfigOptions.key("rss.server.hybrid.storage.storagemanager.cache.timeout")
-          .longType()
-          .defaultValue(60 * 1000L)
-          .withDescription("The timeout of the cache which record the mapping 
information")
-          
.withDeprecatedKeys("rss.server.multistorage.storagemanager.cache.timeout");
-
   public static final ConfigOption<Long> 
SERVER_LEAK_SHUFFLE_DATA_CHECK_INTERVAL =
       ConfigOptions.key("rss.server.leak.shuffledata.check.interval")
           .longType()
diff --git 
a/server/src/main/java/org/apache/uniffle/server/storage/HybridStorageManager.java
 
b/server/src/main/java/org/apache/uniffle/server/storage/HybridStorageManager.java
index ae83315f0..c1169f42f 100644
--- 
a/server/src/main/java/org/apache/uniffle/server/storage/HybridStorageManager.java
+++ 
b/server/src/main/java/org/apache/uniffle/server/storage/HybridStorageManager.java
@@ -20,10 +20,7 @@ package org.apache.uniffle.server.storage;
 import java.lang.reflect.Constructor;
 import java.util.Collection;
 import java.util.Map;
-import java.util.concurrent.TimeUnit;
 
-import com.google.common.cache.Cache;
-import com.google.common.cache.CacheBuilder;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -45,7 +42,6 @@ public class HybridStorageManager implements StorageManager {
 
   private final StorageManager warmStorageManager;
   private final StorageManager coldStorageManager;
-  private final Cache<ShuffleDataFlushEvent, StorageManager> 
eventOfUnderStorageManagers;
   private final StorageManagerSelector storageManagerSelector;
 
   HybridStorageManager(ShuffleServerConf conf) {
@@ -61,10 +57,6 @@ public class HybridStorageManager implements StorageManager {
     } catch (Exception e) {
       throw new RssException("Errors on loading selector manager.", e);
     }
-
-    long cacheTimeout = 
conf.getLong(ShuffleServerConf.STORAGEMANAGER_CACHE_TIMEOUT);
-    eventOfUnderStorageManagers =
-        CacheBuilder.newBuilder().expireAfterAccess(cacheTimeout, 
TimeUnit.MILLISECONDS).build();
   }
 
   private StorageManagerSelector loadManagerSelector(
@@ -129,17 +121,12 @@ public class HybridStorageManager implements 
StorageManager {
 
   private StorageManager selectStorageManager(ShuffleDataFlushEvent event) {
     StorageManager storageManager = storageManagerSelector.select(event);
-    eventOfUnderStorageManagers.put(event, storageManager);
-    event.addCleanupCallback(() -> 
eventOfUnderStorageManagers.invalidate(event));
     return storageManager;
   }
 
   @Override
   public boolean write(Storage storage, ShuffleWriteHandler handler, 
ShuffleDataFlushEvent event) {
-    StorageManager underStorageManager = 
eventOfUnderStorageManagers.getIfPresent(event);
-    if (underStorageManager == null) {
-      return false;
-    }
+    StorageManager underStorageManager = selectStorageManager(event);
     return underStorageManager.write(storage, handler, event);
   }