From 7ee7a1a7c61d766e0e6f84a22f14e7ede1613450 Mon Sep 17 00:00:00 2001 From: Karol Sobczak Date: Mon, 5 Aug 2024 16:14:04 +0200 Subject: [PATCH] Remove query.low-memory-killer.delay query.low-memory-killer.delay is not needed: * It does not prevent cascades of query kills. That is achieved by isLastKillTargetGone check in ClusterMemoryManager * OOM blocked worker cannot be unblocked by other means other than killing the query. Revocable memory (and spill to disk) also won't cause node to be considered out-of-memory. Hence low-memory-killer does not interfere with spill-to-disk. Having query.low-memory-killer.delay causes reduced concurrency on a cluster with higher concurrency and under low-memory situations. --- .../io/trino/memory/ClusterMemoryManager.java | 15 +---------- .../io/trino/memory/MemoryManagerConfig.java | 27 ++----------------- .../io/trino/server/ServerMainModule.java | 4 --- .../trino/memory/TestMemoryManagerConfig.java | 9 ++----- .../io/trino/memory/TestMemoryManager.java | 1 - 5 files changed, 5 insertions(+), 51 deletions(-) diff --git a/core/trino-main/src/main/java/io/trino/memory/ClusterMemoryManager.java b/core/trino-main/src/main/java/io/trino/memory/ClusterMemoryManager.java index ba4a31691815..e818b569f20b 100644 --- a/core/trino-main/src/main/java/io/trino/memory/ClusterMemoryManager.java +++ b/core/trino-main/src/main/java/io/trino/memory/ClusterMemoryManager.java @@ -27,7 +27,6 @@ import io.airlift.json.JsonCodec; import io.airlift.log.Logger; import io.airlift.units.DataSize; -import io.airlift.units.Duration; import io.trino.execution.LocationFactory; import io.trino.execution.QueryExecution; import io.trino.execution.QueryInfo; @@ -72,7 +71,6 @@ import static com.google.common.collect.Sets.difference; import static io.airlift.concurrent.Threads.daemonThreadsNamed; import static io.airlift.units.DataSize.succinctBytes; -import static io.airlift.units.Duration.nanosSince; import static io.trino.ExceededMemoryLimitException.exceededGlobalTotalLimit; import static io.trino.ExceededMemoryLimitException.exceededGlobalUserLimit; import static io.trino.SystemSessionProperties.RESOURCE_OVERCOMMIT; @@ -103,7 +101,6 @@ public class ClusterMemoryManager private final DataSize maxQueryMemory; private final DataSize maxQueryTotalMemory; private final List lowMemoryKillers; - private final Duration killOnOutOfMemoryDelay; private final AtomicLong totalAvailableProcessors = new AtomicLong(); private final AtomicLong clusterUserMemoryReservation = new AtomicLong(); private final AtomicLong clusterTotalMemoryReservation = new AtomicLong(); @@ -119,9 +116,6 @@ public class ClusterMemoryManager private final ClusterMemoryPool pool; - @GuardedBy("this") - private long lastTimeNotOutOfMemory = System.nanoTime(); - @GuardedBy("this") private Optional lastKillTarget = Optional.empty(); @@ -151,7 +145,6 @@ public ClusterMemoryManager( queryLowMemoryKiller); this.maxQueryMemory = config.getMaxQueryMemory(); this.maxQueryTotalMemory = config.getMaxQueryTotalMemory(); - this.killOnOutOfMemoryDelay = config.getKillOnOutOfMemoryDelay(); verify(maxQueryMemory.toBytes() <= maxQueryTotalMemory.toBytes(), "maxQueryMemory cannot be greater than maxQueryTotalMemory"); @@ -182,9 +175,6 @@ public synchronized void process(Iterable runningQueries, Suppli memoryLeakDetector.checkForMemoryLeaks(allQueryInfoSupplier, pool.getQueryMemoryReservations()); boolean outOfMemory = isClusterOutOfMemory(); - if (!outOfMemory) { - lastTimeNotOutOfMemory = System.nanoTime(); - } boolean queryKilled = false; long totalUserMemoryBytes = 0L; @@ -228,10 +218,7 @@ public synchronized void process(Iterable runningQueries, Suppli clusterUserMemoryReservation.set(totalUserMemoryBytes); clusterTotalMemoryReservation.set(totalMemoryBytes); - if (!lowMemoryKillers.isEmpty() && - outOfMemory && - !queryKilled && - nanosSince(lastTimeNotOutOfMemory).compareTo(killOnOutOfMemoryDelay) > 0) { + if (!lowMemoryKillers.isEmpty() && outOfMemory && !queryKilled) { if (isLastKillTargetGone()) { callOomKiller(runningQueries); } diff --git a/core/trino-main/src/main/java/io/trino/memory/MemoryManagerConfig.java b/core/trino-main/src/main/java/io/trino/memory/MemoryManagerConfig.java index 211b9db89ee0..598f317b6580 100644 --- a/core/trino-main/src/main/java/io/trino/memory/MemoryManagerConfig.java +++ b/core/trino-main/src/main/java/io/trino/memory/MemoryManagerConfig.java @@ -17,7 +17,6 @@ import io.airlift.configuration.ConfigDescription; import io.airlift.configuration.DefunctConfig; import io.airlift.units.DataSize; -import io.airlift.units.Duration; import jakarta.validation.constraints.NotNull; import static com.google.common.base.Preconditions.checkArgument; @@ -25,13 +24,12 @@ import static io.airlift.units.DataSize.succinctBytes; import static java.lang.String.format; import static java.util.Locale.ENGLISH; -import static java.util.concurrent.TimeUnit.MINUTES; -import static java.util.concurrent.TimeUnit.SECONDS; @DefunctConfig({ "experimental.cluster-memory-manager-enabled", "query.low-memory-killer.enabled", - "resources.reserved-system-memory"}) + "resources.reserved-system-memory", + "query.low-memory-killer.delay"}) public class MemoryManagerConfig { // enforced against user memory allocations @@ -47,8 +45,6 @@ public class MemoryManagerConfig private DataSize faultTolerantExecutionEagerSpeculativeTasksNodeMemoryOvercommit = DataSize.of(20, GIGABYTE); private LowMemoryQueryKillerPolicy lowMemoryQueryKillerPolicy = LowMemoryQueryKillerPolicy.TOTAL_RESERVATION_ON_BLOCKED_NODES; private LowMemoryTaskKillerPolicy lowMemoryTaskKillerPolicy = LowMemoryTaskKillerPolicy.TOTAL_RESERVATION_ON_BLOCKED_NODES; - // default value is overwritten for fault tolerant execution in {@link #applyFaultTolerantExecutionDefaults()}} - private Duration killOnOutOfMemoryDelay = new Duration(30, SECONDS); @NotNull public DataSize getMaxQueryMemory() @@ -199,25 +195,6 @@ public MemoryManagerConfig setLowMemoryTaskKillerPolicy(LowMemoryTaskKillerPolic return this; } - @NotNull - public Duration getKillOnOutOfMemoryDelay() - { - return killOnOutOfMemoryDelay; - } - - @Config("query.low-memory-killer.delay") - @ConfigDescription("Delay between cluster running low on memory and invoking killer") - public MemoryManagerConfig setKillOnOutOfMemoryDelay(Duration killOnOutOfMemoryDelay) - { - this.killOnOutOfMemoryDelay = killOnOutOfMemoryDelay; - return this; - } - - public void applyFaultTolerantExecutionDefaults() - { - killOnOutOfMemoryDelay = new Duration(0, MINUTES); - } - public enum LowMemoryQueryKillerPolicy { NONE, diff --git a/core/trino-main/src/main/java/io/trino/server/ServerMainModule.java b/core/trino-main/src/main/java/io/trino/server/ServerMainModule.java index b89c5f3f1792..a731ae0da289 100644 --- a/core/trino-main/src/main/java/io/trino/server/ServerMainModule.java +++ b/core/trino-main/src/main/java/io/trino/server/ServerMainModule.java @@ -293,10 +293,6 @@ protected void setup(Binder binder) newExporter(binder).export(PauseMeter.class).withGeneratedName(); configBinder(binder).bindConfig(MemoryManagerConfig.class); - if (retryPolicy == TASK) { - configBinder(binder).bindConfigDefaults(MemoryManagerConfig.class, MemoryManagerConfig::applyFaultTolerantExecutionDefaults); - } - configBinder(binder).bindConfig(NodeMemoryConfig.class); binder.bind(LocalMemoryManager.class).in(Scopes.SINGLETON); binder.bind(LocalMemoryManagerExporter.class).in(Scopes.SINGLETON); diff --git a/core/trino-main/src/test/java/io/trino/memory/TestMemoryManagerConfig.java b/core/trino-main/src/test/java/io/trino/memory/TestMemoryManagerConfig.java index 81bfd04de176..25a6d1e668a4 100644 --- a/core/trino-main/src/test/java/io/trino/memory/TestMemoryManagerConfig.java +++ b/core/trino-main/src/test/java/io/trino/memory/TestMemoryManagerConfig.java @@ -15,7 +15,6 @@ import com.google.common.collect.ImmutableMap; import io.airlift.units.DataSize; -import io.airlift.units.Duration; import io.trino.memory.MemoryManagerConfig.LowMemoryQueryKillerPolicy; import io.trino.memory.MemoryManagerConfig.LowMemoryTaskKillerPolicy; import org.junit.jupiter.api.Test; @@ -27,7 +26,6 @@ import static io.airlift.configuration.testing.ConfigAssertions.recordDefaults; import static io.airlift.units.DataSize.Unit.GIGABYTE; import static io.airlift.units.DataSize.Unit.MEGABYTE; -import static java.util.concurrent.TimeUnit.SECONDS; public class TestMemoryManagerConfig { @@ -45,8 +43,7 @@ public void testDefaults() .setFaultTolerantExecutionMemoryRequirementIncreaseOnWorkerCrashEnabled(true) .setFaultTolerantExecutionEagerSpeculativeTasksNodeMemoryOvercommit(DataSize.of(20, GIGABYTE)) .setLowMemoryQueryKillerPolicy(LowMemoryQueryKillerPolicy.TOTAL_RESERVATION_ON_BLOCKED_NODES) - .setLowMemoryTaskKillerPolicy(LowMemoryTaskKillerPolicy.TOTAL_RESERVATION_ON_BLOCKED_NODES) - .setKillOnOutOfMemoryDelay(new Duration(30, SECONDS))); + .setLowMemoryTaskKillerPolicy(LowMemoryTaskKillerPolicy.TOTAL_RESERVATION_ON_BLOCKED_NODES)); } @Test @@ -64,7 +61,6 @@ public void testExplicitPropertyMappings() .put("fault-tolerant-execution-eager-speculative-tasks-node_memory-overcommit", "21GB") .put("query.low-memory-killer.policy", "none") .put("task.low-memory-killer.policy", "none") - .put("query.low-memory-killer.delay", "20s") .buildOrThrow(); MemoryManagerConfig expected = new MemoryManagerConfig() @@ -78,8 +74,7 @@ public void testExplicitPropertyMappings() .setFaultTolerantExecutionMemoryRequirementIncreaseOnWorkerCrashEnabled(false) .setFaultTolerantExecutionEagerSpeculativeTasksNodeMemoryOvercommit(DataSize.of(21, GIGABYTE)) .setLowMemoryQueryKillerPolicy(LowMemoryQueryKillerPolicy.NONE) - .setLowMemoryTaskKillerPolicy(LowMemoryTaskKillerPolicy.NONE) - .setKillOnOutOfMemoryDelay(new Duration(20, SECONDS)); + .setLowMemoryTaskKillerPolicy(LowMemoryTaskKillerPolicy.NONE); assertFullMapping(properties, expected); } diff --git a/testing/trino-tests/src/test/java/io/trino/memory/TestMemoryManager.java b/testing/trino-tests/src/test/java/io/trino/memory/TestMemoryManager.java index 18b5fb2b73ae..9a89392ef37d 100644 --- a/testing/trino-tests/src/test/java/io/trino/memory/TestMemoryManager.java +++ b/testing/trino-tests/src/test/java/io/trino/memory/TestMemoryManager.java @@ -118,7 +118,6 @@ public void testOutOfMemoryKiller() throws Exception { Map properties = ImmutableMap.builder() - .put("query.low-memory-killer.delay", "5s") .put("query.low-memory-killer.policy", "total-reservation") .buildOrThrow();