Allow assigning exclusive node for task execution by losipiuk · Pull Request #10432 · trinodb/trino

losipiuk · 2021-12-29T22:47:19Z

Description

If the task fails due to an out-of-memory error on the next retry Trino will try to allocate the full node for execution.

General information

Is this change a fix, improvement, new feature, refactoring, or other?

improvement

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

core query engine

How would you describe this change to a non-technical end user or system administrator?

N/A

Documentation

(x) No documentation is needed. (will be added separately for whole fault-tolerance feature)
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
(x) Release notes entries required with the following suggested text:

# General
* Improved handling for queries where individual tasks require lots of memory. 
  If the task fails due to out-of-memory error it is retried with a full node at its disposition.
  Mechanism is only active if task based retries are enabled (``retry-policy`` is set to ``TASK``) ({issue}`10432`)

losipiuk · 2021-12-29T22:51:02Z

@arhimondr @linzebing give it some initial read when you have chance please.

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

core/trino-main/src/main/java/io/trino/execution/scheduler/FaultTolerantStageScheduler.java

core/trino-main/src/main/java/io/trino/execution/scheduler/BinPackingNodeAllocatorService.java

losipiuk · 2022-01-17T21:56:12Z

@arhimondr I added a way to define max number of retries per task + a simple integration test which validates that memory-burst execution actually happens.

arhimondr

Generally looks good to me. Great work @losipiuk .

Questions / comments are mostly minor and most of them are around configuration/naming details

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

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

core/trino-main/src/main/java/io/trino/execution/QueryManagerConfig.java

...ino-main/src/main/java/io/trino/execution/scheduler/FullNodeCapableNodeAllocatorService.java

.../trino-main/src/test/java/io/trino/execution/scheduler/TestFullNodeCapableNodeAllocator.java

losipiuk · 2022-01-24T12:13:32Z

(rebased)

losipiuk · 2022-01-24T23:46:26Z

some comments remain to be addressed

losipiuk · 2022-03-03T19:10:33Z

Major changes:

Rebased on top of Support for killing individual task if cluster is out of memory #11129.
Dropped mechanism of per-task memory limit. We are not enforcing limits right now. We are estimating tasks size, and bin-packing based on that, but we allow it to go above. If memory pool is exhaust memory kill will intervene and kill one of the tasks
I dropped integration test. With memory-killer approach I was not able to make it work (deterministically enforce that one of the tasks would require full not allocation)
I get workers memory from ClusterMemoryManager (not extending InternalNode with extra field).

@arhimondr PTAL once again

linzebing · 2022-03-04T01:22:17Z

.../trino-main/src/test/java/io/trino/execution/scheduler/TestFullNodeCapableNodeAllocator.java

Should we have a test covering the logic around sharedAllocatedMemory?

what exactly you think is not covered. The testcases which allocated "shared" note exercise exactly that.

losipiuk · 2022-03-08T15:31:50Z

Pushed updated version. I needed to move NodeAllocator guice bindings to CoordinatorModule as one of the implementations depends now on ClusterMemoryManager which is not bound on workers. This better place for those anyway, as we do not need NodeAllocator on workers neither.

It turned out we do not need that functionality after all for task-level retries. Removing as currently we do not see the benefit of the mechanism and it increases complexity.

Different implementations of NodeAllocator require different scheme of seleting memory requirements for a partition on retries. Commit introduce PartitionMemoryEstimator interface and two separate implementations. * ConstantPartitionMemoryEstimator to be used with FixedCountNodeAllocator * FallbackToFullNodePartitionMemoryEstimator to be used with FullNodeCapableNodeAllocator

arhimondr · 2022-03-09T20:07:50Z

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

The target input size is set to 1GB. I wonder if it is better to set it to something higher, maybe 2 or 3 GB?

I think default should be fraction on HEAP, not an absolute value. Will change that in a followup.

arhimondr · 2022-03-09T20:12:01Z

testing/trino-testing/src/main/java/io/trino/testing/BaseFailureRecoveryTest.java

Is it still needed?

Nope - actually tests failed because of that :)

losipiuk · 2022-03-10T10:44:17Z

CI: #5892

mosabua · 2022-03-10T18:12:25Z

@losipiuk for the release notes text please use markdown syntax for the code highlighting .. so single ` only

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

losipiuk requested review from arhimondr and linzebing December 29, 2021 22:50

losipiuk force-pushed the lo/retry-memory-burst branch 3 times, most recently from afd344b to c1ddffc Compare December 31, 2021 11:04

arhimondr mentioned this pull request Jan 4, 2022

Support Failure Recovery #9101

Closed

31 tasks

losipiuk force-pushed the lo/retry-memory-burst branch from c1ddffc to d9dfe98 Compare January 10, 2022 13:25

losipiuk mentioned this pull request Jan 10, 2022

[WIP] Implement exchange spooling #10376

Closed

arhimondr reviewed Jan 11, 2022

View reviewed changes

losipiuk force-pushed the lo/retry-memory-burst branch 2 times, most recently from 6e8efd7 to f42b6cd Compare January 13, 2022 15:47

losipiuk requested a review from arhimondr January 13, 2022 15:51

github-actions bot added the tests:hive label Jan 13, 2022

losipiuk force-pushed the lo/retry-memory-burst branch 3 times, most recently from 3f66f2e to 667edd4 Compare January 17, 2022 21:54

losipiuk force-pushed the lo/retry-memory-burst branch from 667edd4 to 91a4f4b Compare January 21, 2022 10:16

losipiuk changed the title ~~WIP: Allow assigning exclusive node for task execution~~ Allow assigning exclusive node for task execution Jan 21, 2022

losipiuk marked this pull request as ready for review January 21, 2022 10:17

losipiuk self-assigned this Jan 21, 2022

arhimondr reviewed Jan 21, 2022

View reviewed changes

losipiuk force-pushed the lo/retry-memory-burst branch from 91a4f4b to 178ab5d Compare January 24, 2022 12:13

losipiuk force-pushed the lo/retry-memory-burst branch 2 times, most recently from dc6d4d6 to 00df605 Compare January 24, 2022 23:46

losipiuk force-pushed the lo/retry-memory-burst branch from 00df605 to e56c891 Compare January 25, 2022 13:12

losipiuk force-pushed the lo/retry-memory-burst branch from d1ef1a1 to d6f53b9 Compare March 3, 2022 19:05

losipiuk force-pushed the lo/retry-memory-burst branch from d6f53b9 to 0cd5d69 Compare March 3, 2022 20:10

github-actions bot added the docs label Mar 3, 2022

linzebing reviewed Mar 4, 2022

View reviewed changes

losipiuk force-pushed the lo/retry-memory-burst branch 2 times, most recently from ca0b3b6 to d7e7c11 Compare March 8, 2022 15:30

losipiuk closed this Mar 8, 2022

losipiuk reopened this Mar 8, 2022

losipiuk force-pushed the lo/retry-memory-burst branch from d7e7c11 to d39e8b2 Compare March 9, 2022 12:28

losipiuk added 10 commits March 9, 2022 15:01

Fix statement ordering

9f8d86b

Rename session property getter

634b038

Remove reduntant 'else' clauses

af08f9a

Introduce NodeAllocatorService

32b9f24

Add NodeInfo wrapper for nodes allocated via NodeAllocator

45f046c

Drop mechanism of per memory task limit

83b7c62

It turned out we do not need that functionality after all for task-level retries. Removing as currently we do not see the benefit of the mechanism and it increases complexity.

Use explicit lease object in NodeAllocator

b9f82a3

Add memory to task requirements passed to node allocator

4442111

Add node allocator supporting full-node acquisitions

2ccf7f6

losipiuk force-pushed the lo/retry-memory-burst branch from d39e8b2 to 1806e3b Compare March 9, 2022 14:08

arhimondr approved these changes Mar 9, 2022

View reviewed changes

Separate retries configuration for queries and tasks

cebfae3

losipiuk force-pushed the lo/retry-memory-burst branch from 1806e3b to cebfae3 Compare March 9, 2022 22:54

losipiuk merged commit 99ef4ec into trinodb:master Mar 10, 2022

github-actions bot added this to the 374 milestone Mar 10, 2022

mosabua mentioned this pull request Mar 10, 2022

Add Trino 374 release notes #11417

Merged

Conversation

losipiuk commented Dec 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

General information

Documentation

Release notes

Uh oh!

losipiuk commented Dec 29, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

losipiuk commented Jan 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arhimondr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

losipiuk commented Jan 24, 2022

Uh oh!

losipiuk commented Jan 24, 2022

Uh oh!

losipiuk commented Mar 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linzebing Mar 4, 2022

Choose a reason for hiding this comment

Uh oh!

losipiuk Mar 4, 2022

Choose a reason for hiding this comment

Uh oh!

losipiuk commented Mar 8, 2022

Uh oh!

arhimondr Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

losipiuk Mar 10, 2022

Choose a reason for hiding this comment

Uh oh!

arhimondr Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

losipiuk Mar 10, 2022

Choose a reason for hiding this comment

Uh oh!

losipiuk commented Mar 10, 2022

Uh oh!

mosabua commented Mar 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

losipiuk commented Dec 29, 2021 •

edited

Loading

losipiuk commented Jan 17, 2022 •

edited

Loading

losipiuk commented Mar 3, 2022 •

edited

Loading