This is an automated email from the ASF dual-hosted git repository. roryqi pushed a commit to branch branch-0.9 in repository https://gitbox.apache.org/repos/asf/incubator-uniffle.git
commit ac69e8460bb63932052e5577406eeba7f9b4dfd4 Author: RickyMa <[email protected]> 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); }
