Add LEAST_WASTED task memory killer by losipiuk · Pull Request #12242 · trinodb/trino

losipiuk · 2022-05-04T14:20:46Z

Fixes #11952

Description

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

Documentation

( ) No documentation is needed.
(x) 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
* Rename configuration property used for selecting a policy for low memory query killer from
  `query.low-memory-killer.policy` to `query.low-memory-query-killer.policy`. ({issue}`12242`)
* Introduce specific configuration property `query.low-memory-task-killer.policy` for selecting a policy 
  for low memory task killer. The policy is used if task level retries are enabled (`retry-policy=TASK`). ({issue}`12242`)
* Introduce `least-wasted` low memory task killer policy. This policy avoids killing tasks that are already
  executing for a long time, so a significant amount of work is not wasted.  ({issue}`12242`)

...no-main/src/main/java/io/trino/memory/TotalReservationOnBlockedNodesLowMemoryTaskKiller.java

...no-main/src/test/java/io/trino/memory/TestTotalReservationOnBlockedNodesLowMemoryKiller.java

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

linzebing · 2022-05-05T00:32:26Z

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

It's a bit weird to have multiple low memory killers, unless we plan to support pipelined execution and fault tolerant execution in the same cluster.

I agree that if properly used cluster only one should trigger. But currently, we are not forcibly forbidding users from running both types of workloads in the same cluster, so we need to handle that in code I think.

Does it make sense to remove the session property for retry-policy (or at least don't allow hybrid mode) right now? cc @arhimondr

Actually I don't mind removing the session property since currently we don't support mixed workloads. Then based on the retry-policy value we may inject the killer we need to keep it simple. However we would still probably need two configuration properties, one that defines what killer to use for killing tasks and the other one for queries as they are going to have different defaults. We can pick one based on the retry-policy then. It feels like it would make things simpler.

It could be done - but we would need to rework testing. In FTE tests we currently run query with and without retries. We switch the mode using session property. We would need to migrate approach to have two query runners - can be done but tests will be a bit longer and would require more resources (we need two query runners up and running).
I will see how easy it is to change it in a separate PR.

losipiuk · 2022-05-06T16:33:29Z

AC

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

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

core/trino-main/src/main/java/io/trino/server/CoordinatorModule.java

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

arhimondr · 2022-05-06T18:11:00Z

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

Actually I don't mind removing the session property since currently we don't support mixed workloads. Then based on the retry-policy value we may inject the killer we need to keep it simple. However we would still probably need two configuration properties, one that defines what killer to use for killing tasks and the other one for queries as they are going to have different defaults. We can pick one based on the retry-policy then. It feels like it would make things simpler.

...o-main/src/main/java/io/trino/memory/TotalReservationOnBlockedNodesLowMemoryQueryKiller.java

...no-main/src/main/java/io/trino/memory/TotalReservationOnBlockedNodesLowMemoryTaskKiller.java

docs/src/main/sphinx/admin/properties-query-management.rst

arhimondr · 2022-05-09T18:55:26Z

core/trino-main/src/main/java/io/trino/server/CoordinatorModule.java

Is it still needed?

arhimondr · 2022-05-09T18:56:07Z

core/trino-main/src/test/java/io/trino/memory/TestLeastWastedEffortLowMemoryTaskKiller.java

nit: TestLeastWastedEffortTaskLowMemoryKiller (same for other tests)

thx; others were fine

losipiuk · 2022-05-10T10:28:38Z

@jhlodin could you PTAL at doc changes?

jhlodin

Suggested some changes to the docs

jhlodin · 2022-05-10T14:53:13Z

docs/src/main/sphinx/admin/fault-tolerant-execution.rst

Seems like this should be "Set the query.low-memory-task-killer.policy". It's in the context of "recommended changes for a TASK retry policy" so we can be specific about which one.

In this situation, are any changes recommended to the query task killer policy?

We do not care about query.low-memory-query-killer.policy after we split properties. Fixed the typo.

docs/src/main/sphinx/admin/properties-query-management.rst

jhlodin · 2022-05-10T14:59:48Z

docs/src/main/sphinx/admin/properties-query-management.rst

What's the interaction between this and the other low memory killer property if both are set?

From context it seems like this property is only used when retry-policy is set to TASK. If that's the case, should add a sentence to this description saying that, and whether the query low memory killer does anything in that case.

What we should be sayint to users is:
query.low-memory-task-killer.policy is used whenretry_policy is TASK.
query.low-memory-query-killer.policy is used otherwise.

In reality it is that:
query.low-memory-task-killer.policy is used for queries which were run with retry_policy set to tasks and query.low-memory-query-killer.policy for other queries - but we officially do not support mixed workloads so we should not word it like that.

O - actualy we use such wording already:

Does not apply for queries with task level retries enabled (``retry-policy=TASK``)

I will keep it consistent for now then

jhlodin · 2022-05-10T15:01:56Z

docs/src/main/sphinx/admin/properties-query-management.rst

a value different than NONE

jhlodin · 2022-05-10T15:04:21Z

What will happen to clusters that currently have an explicit configuration of the now-legacy query.low-memory-killer.policy property? Is this a breaking change for them until they specify either the query or task config?

losipiuk · 2022-05-11T11:55:13Z

What will happen to clusters that currently have an explicit configuration of the now-legacy query.low-memory-killer.policy property? Is this a breaking change for them until they specify either the query or task config?

Nope - you can still use legacy config keys - you will just get a warning during startup in logs.

Add task low memory killer which picks ups a task which has highest used-memory/task-runtime ratio.

cla-bot bot added the cla-signed label May 4, 2022

losipiuk requested review from arhimondr and linzebing May 4, 2022 14:21

github-actions bot added the docs label May 4, 2022

linzebing reviewed May 5, 2022

View reviewed changes

losipiuk force-pushed the lo/smarter-memory-killer branch from f33410e to 576bc64 Compare May 6, 2022 16:30

losipiuk requested a review from linzebing May 6, 2022 16:33

arhimondr reviewed May 6, 2022

View reviewed changes

linzebing approved these changes May 6, 2022

View reviewed changes

losipiuk force-pushed the lo/smarter-memory-killer branch 2 times, most recently from b1fb8ed to e31b684 Compare May 9, 2022 11:39

losipiuk requested a review from arhimondr May 9, 2022 11:50

arhimondr approved these changes May 9, 2022

View reviewed changes

losipiuk added 3 commits May 10, 2022 12:21

Add missing new lines at end of file

a805439

Rename config property

93d9173

Rename method

a52df3a

losipiuk force-pushed the lo/smarter-memory-killer branch from e31b684 to a088ee2 Compare May 10, 2022 10:28

jhlodin reviewed May 10, 2022

View reviewed changes

losipiuk added 4 commits May 11, 2022 16:13

Separate config for task and query low memory killers

ab4c432

Remove unused methods

defe57a

Pass more information to LowMemoryKiller

1e81caa

Add LEAST_WASTED task memory killer

2c1025e

Add task low memory killer which picks ups a task which has highest used-memory/task-runtime ratio.

losipiuk force-pushed the lo/smarter-memory-killer branch from a088ee2 to 2c1025e Compare May 11, 2022 14:22

losipiuk merged commit a58fc50 into trinodb:master May 12, 2022

github-actions bot added this to the 381 milestone May 12, 2022

mosabua mentioned this pull request May 12, 2022

Add Trino 381 release notes #12277

Merged

Conversation

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

Uh oh!

Description

Documentation

Release notes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

losipiuk commented May 6, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

losipiuk commented May 10, 2022

Uh oh!

jhlodin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhlodin commented May 10, 2022

Uh oh!

losipiuk commented May 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

losipiuk commented May 4, 2022 •

edited

Loading

jhlodin left a comment •

edited

Loading