Allow setting per task memory limit #10308

losipiuk · 2021-12-15T10:10:51Z

A building block for better memory management to be used mostly together with tasks level retries (#9818)

arhimondr

The change looks reasonable to me. Though I'm not an expert here. I wonder if there's anybody else who might want to take a look?

arhimondr · 2021-12-15T18:28:05Z

core/trino-main/src/main/java/io/trino/memory/NodeMemoryConfig.java

+    @Config(QUERY_MAX_MEMORY_PER_TASK_CONFIG)
+    public NodeMemoryConfig setMaxQueryMemoryPerTask(DataSize maxQueryMemoryPerTask)
+    {
+        if (maxQueryMemoryPerTask.toBytes() == 0) {


Should this be @Min("1B")?

Actually 0 has special meaning: "no-limit". Added description.

Should it be allowed to be empty instead?

It can be. I somewhat prefer it to be possible to explicitly set the value to "unlimited" in the config. Via session variable we can make unlimited if value is set to null - but I am not very convinced it is easier to comprehend that special 0.

@findepi any opinion here?

I remember generally we prefer not to have values with a special meaning. To avoid enforcing this limit not setting the limit seems to be more intuitive than setting it to 0.

Why would anyone set the property to 0 if default is no-limit?

Currently no. I was more thinking of case when (if) we decide that default is not no-limit. I can change it though.

I was more thinking of case when (if) we decide that default is not no-limit.

0 is odd, but then if we decide there should be some limit by default, should there be an option to run query without any per-task memory limit? User probably could set it to query mam memory per node.

lib/trino-memory-context/src/main/java/io/trino/memory/context/MemoryAllocationValidator.java

losipiuk · 2021-12-20T19:52:28Z

Rebase

losipiuk · 2021-12-20T19:58:31Z

@findepi PTAL

sopel39 · 2021-12-21T10:24:53Z

lib/trino-memory-context/src/main/java/io/trino/memory/context/MemoryAllocationValidator.java

+ */
+package io.trino.memory.context;
+
+public interface MemoryAllocationValidator


This is really similar to MemoryReservationHandler. Could that interface do the job? It seems that memory allocations via reserveMemory should return Future as it's caller intention is to wait for memory. The other method tryReserveMemory is for trying to allocate memory.

With interface like this, I don't see a reason for two methods to exist (other than exception customization), but then probably single callback is fine: #10308 (comment)

Future<> as a return type does not suite me well. No use for that. Sole purpose of validator is to "reject" the memory allocation if limit is exceeded. It is not handling the case when the limit is not exceeded but node run out of memory (pool is depleted).

Future<> as a return type does not suite me well. No use for that. Sole purpose of validator is to "reject" the memory allocation if limit is exceeded. It is not handling the case when the limit is not exceeded but node run out of memory (pool is depleted).

Ok, so why do we need an interface with two methods?

Two methods there so we do not need to do exception driven logic in ChildAggregatedMemoryContext.tryUpdateBytes where MemoryAllocationValidator.tryReserveMemory is used.
If I just had reserveMemory I would need to try/catch there. It is an option, but I think it is not ellegant.

It seems that we leak implementation details to interface. I don't think two methods are needed now that you have ValidatingLocalMemoryContext

I can make it one method which would return Optional<String>

empty if allocation is valid

containing allocation error description if allocation violates constraints
Then I can do the throwing in ValidatingLocalMemoryContext

Actually, I take that back. Then the ValidatingLocalMemoryContext need to decide on exception type to throw. I am not a big fan of hardcoding the ExceededMemoryLimitException(EXCEEDED_LOCAL_MEMORY_LIMIT, there. It makes class less reusable. And by design nothing is enforcing us to use the class only for TaskContext.

I suggest we keep interface as proposed.

lib/trino-memory-context/src/main/java/io/trino/memory/context/MemoryAllocationValidator.java

sopel39 · 2021-12-21T10:35:25Z

lib/trino-memory-context/src/main/java/io/trino/memory/context/AggregatedMemoryContext.java

@@ -27,6 +27,8 @@ static AggregatedMemoryContext newRootAggregatedMemoryContext(MemoryReservationH

    AggregatedMemoryContext newAggregatedMemoryContext();

+    AggregatedMemoryContext newAggregatedMemoryContext(MemoryAllocationValidator memoryAllocationValidator);


I wonder if this shouldn't be solved locally at task level via combination of newRootAggregatedMemoryContext and MemoryReservationHandler. Are there more use cases for newAggregatedMemoryContext(MemoryAllocationValidator memoryAllocationValidator) other than task allocation?

For example both OperatorContext and WorkProcessorPipelineSourceOperator both have their own InternalAggregatedMemoryContext. I'm not sure we should mix MemoryAllocationValidator memoryAllocationValidator into that picture (this adds complexity to the allocators structure)

@sopel39 PTAL now

lib/trino-memory-context/src/main/java/io/trino/memory/context/MemoryAllocationValidator.java

...no-memory-context/src/main/java/io/trino/memory/context/AbstractAggregatedMemoryContext.java

findepi

skimmed, lgtm

losipiuk · 2021-12-22T13:57:25Z

...trino-memory-context/src/main/java/io/trino/memory/context/ValidatingLocalMemoryContext.java

+    }
+
+    @Override
+    public ListenableFuture<Void> setBytes(long bytes)


do I need synchronization here and in trySetBytes?

core/trino-main/src/main/java/io/trino/SystemSessionProperties.java

sopel39 · 2021-12-22T14:35:42Z

core/trino-main/src/main/java/io/trino/memory/NodeMemoryConfig.java

+    @Config(QUERY_MAX_MEMORY_PER_TASK_CONFIG)
+    public NodeMemoryConfig setMaxQueryMemoryPerTask(DataSize maxQueryMemoryPerTask)
+    {
+        if (maxQueryMemoryPerTask.toBytes() == 0) {


Why would anyone set the property to 0 if default is no-limit?

sopel39 · 2021-12-22T14:38:44Z

lib/trino-memory-context/src/main/java/io/trino/memory/context/MemoryAllocationValidator.java

+ */
+package io.trino.memory.context;
+
+public interface MemoryAllocationValidator


Future<> as a return type does not suite me well. No use for that. Sole purpose of validator is to "reject" the memory allocation if limit is exceeded. It is not handling the case when the limit is not exceeded but node run out of memory (pool is depleted).

Ok, so why do we need an interface with two methods?

Two methods there so we do not need to do exception driven logic in ChildAggregatedMemoryContext.tryUpdateBytes where MemoryAllocationValidator.tryReserveMemory is used.
If I just had reserveMemory I would need to try/catch there. It is an option, but I think it is not ellegant.

It seems that we leak implementation details to interface. I don't think two methods are needed now that you have ValidatingLocalMemoryContext

sopel39 · 2021-12-22T14:40:09Z

...trino-memory-context/src/main/java/io/trino/memory/context/ValidatingLocalMemoryContext.java

+        long delta = bytes - delegate.getBytes();
+
+        // first consult validator if allocation is possible
+        memoryValidator.reserveMemory(allocationTag, delta);


you have absolute value at hand, just pass it to validator (single method) as in:

void enforceUserMemoryLimit(long allocated, long delta)

I do not have absolute value. Here I just know how many bytes are allocated by this LocalMemoryContext. And memory validator is accounting on AggregatedMemoryContext level.

sopel39 · 2021-12-22T14:42:04Z

...trino-memory-context/src/main/java/io/trino/memory/context/ValidatingLocalMemoryContext.java

+    {
+        long delta = bytes - delegate.getBytes();
+
+        if (!memoryValidator.tryReserveMemory(allocationTag, delta)) {


I think this should always throw exception if limit is exceeded (and not return false). This is how io.trino.memory.QueryContext#enforceUserMemoryLimit works and it makes sense.
Query needs X memory and X exceeds limit, so try should also fail

Actually enforceUserMemoryLimit is not called from trySetBytes. I think it should on successful allocation, but it is not. Or am I reading something wrong.

core/trino-main/src/main/java/io/trino/operator/TaskAllocationValidator.java

core/trino-main/src/main/java/io/trino/SystemSessionProperties.java

core/trino-main/src/main/java/io/trino/operator/TaskAllocationValidator.java

sopel39 · 2021-12-23T15:03:37Z

core/trino-main/src/main/java/io/trino/memory/NodeMemoryConfig.java

+    @Config(QUERY_MAX_MEMORY_PER_TASK_CONFIG)
+    public NodeMemoryConfig setMaxQueryMemoryPerTask(DataSize maxQueryMemoryPerTask)
+    {
+        if (maxQueryMemoryPerTask.toBytes() == 0) {


I was more thinking of case when (if) we decide that default is not no-limit.

0 is odd, but then if we decide there should be some limit by default, should there be an option to run query without any per-task memory limit? User probably could set it to query mam memory per node.

core/trino-main/src/main/java/io/trino/SystemSessionProperties.java

core/trino-main/src/main/java/io/trino/operator/TaskAllocationValidator.java

core/trino-main/src/main/java/io/trino/operator/TaskContext.java

sopel39

lgtm % comments

core/trino-main/src/main/java/io/trino/operator/TaskAllocationValidator.java

...trino-memory-context/src/main/java/io/trino/memory/context/ValidatingLocalMemoryContext.java

sopel39 · 2021-12-23T16:43:03Z

lib/trino-memory-context/src/main/java/io/trino/memory/context/ValidatingAggregateContext.java

+
+import static java.util.Objects.requireNonNull;
+
+public class ValidatingAggregateContext


maybe some meaningful name?

This is just parameter rename; no semantic changes

cla-bot bot added the cla-signed label Dec 15, 2021

losipiuk requested review from findepi, martint and arhimondr December 15, 2021 10:10

arhimondr approved these changes Dec 16, 2021

View reviewed changes

losipiuk self-assigned this Dec 20, 2021

losipiuk force-pushed the lo/per-task-memory-limits branch from 02918ac to b4d0365 Compare December 20, 2021 19:52

losipiuk force-pushed the lo/per-task-memory-limits branch from b4d0365 to 6621ee4 Compare December 20, 2021 19:58

losipiuk requested a review from sopel39 December 20, 2021 19:58

sopel39 reviewed Dec 21, 2021

View reviewed changes

findepi reviewed Dec 21, 2021

View reviewed changes

...no-memory-context/src/main/java/io/trino/memory/context/AbstractAggregatedMemoryContext.java Show resolved Hide resolved

findepi reviewed Dec 21, 2021

View reviewed changes

losipiuk force-pushed the lo/per-task-memory-limits branch from 6621ee4 to 3828952 Compare December 22, 2021 13:55

losipiuk commented Dec 22, 2021

View reviewed changes

sopel39 reviewed Dec 22, 2021

View reviewed changes

losipiuk force-pushed the lo/per-task-memory-limits branch from 3828952 to ee9eb86 Compare December 23, 2021 15:45

sopel39 reviewed Dec 23, 2021

View reviewed changes

sopel39 approved these changes Dec 23, 2021

View reviewed changes

losipiuk force-pushed the lo/per-task-memory-limits branch from ee9eb86 to 34a6352 Compare December 27, 2021 09:10

losipiuk added 4 commits December 27, 2021 10:10

Make updateBytes and tryUpdateBytes parameters consistent

c43cbee

This is just parameter rename; no semantic changes

Fix typo

5bf0821

Use static import

4ab56dc

Allow setting per task memory limit

5abc758

losipiuk force-pushed the lo/per-task-memory-limits branch from 34a6352 to 5abc758 Compare December 27, 2021 09:10

losipiuk merged commit c88d968 into trinodb:master Dec 27, 2021

losipiuk deleted the lo/per-task-memory-limits branch December 27, 2021 15:22

github-actions bot added this to the 368 milestone Dec 27, 2021

mosabua mentioned this pull request Dec 30, 2021

Add Trino 368 release notes #10433

Merged

losipiuk mentioned this pull request Jan 5, 2022

Release notes for 368 #10389

Closed

maltesander mentioned this pull request Apr 12, 2022

Provide and test trino-server-377 stackabletech/trino-operator#187

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow setting per task memory limit #10308

Allow setting per task memory limit #10308

losipiuk commented Dec 15, 2021 •

edited

Loading

arhimondr left a comment

arhimondr Dec 15, 2021

losipiuk Dec 20, 2021

arhimondr Dec 20, 2021

losipiuk Dec 21, 2021

arhimondr Dec 21, 2021

sopel39 Dec 22, 2021

losipiuk Dec 22, 2021

sopel39 Dec 23, 2021

losipiuk commented Dec 20, 2021

losipiuk commented Dec 20, 2021

sopel39 Dec 21, 2021 •

edited

Loading

losipiuk Dec 22, 2021

sopel39 Dec 22, 2021

losipiuk Dec 22, 2021

losipiuk Dec 22, 2021

sopel39 Dec 21, 2021

losipiuk Dec 22, 2021

findepi left a comment

losipiuk Dec 22, 2021

sopel39 Dec 22, 2021

sopel39 Dec 22, 2021

sopel39 Dec 22, 2021 •

edited

Loading

losipiuk Dec 22, 2021

sopel39 Dec 22, 2021 •

edited

Loading

losipiuk Dec 22, 2021

sopel39 Dec 23, 2021

sopel39 left a comment

sopel39 Dec 23, 2021

losipiuk Dec 27, 2021

		@@ -27,6 +27,8 @@ static AggregatedMemoryContext newRootAggregatedMemoryContext(MemoryReservationH

		AggregatedMemoryContext newAggregatedMemoryContext();

		AggregatedMemoryContext newAggregatedMemoryContext(MemoryAllocationValidator memoryAllocationValidator);


		import static java.util.Objects.requireNonNull;

		public class ValidatingAggregateContext

Allow setting per task memory limit #10308

Allow setting per task memory limit #10308

Conversation

losipiuk commented Dec 15, 2021 • edited Loading

arhimondr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

losipiuk commented Dec 20, 2021

losipiuk commented Dec 20, 2021

sopel39 Dec 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findepi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sopel39 Dec 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sopel39 Dec 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sopel39 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

losipiuk commented Dec 15, 2021 •

edited

Loading

sopel39 Dec 21, 2021 •

edited

Loading

sopel39 Dec 22, 2021 •

edited

Loading

sopel39 Dec 22, 2021 •

edited

Loading