[#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
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 bd71c3b..a7b1400 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 @@
.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 ae83315..c1169f4 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 @@
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 @@
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 @@
} 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 @@
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);
}
