Interrupt runaway splits by aweisberg · Pull Request #16111 · prestodb/presto

aweisberg · 2021-05-17T22:55:51Z

Test plan - Included unit test, deploy to a cluster and test with a known bad query

We do use thread interruption as part of the query cancellation mechanism so it is probably safer then I thought it was in some earlier discussions.

What this is really implementing is eager split termination where we don't honor the user specified query timeout. Because of that I am limiting this to only killing a split that looks like it is blocked in Joni. We can add more if we find other common culprits.

@arhimondr brought up the possibility of edge cases like JDBC where we might have an expensive query that might not yield a page for 10 minutes as it tries to generate a small number of rows from a system that isn't very fast. Even with Hive + HDFS if it is needle in a haystack type stuff this could occur. I think a targeted approach of interrupting allow listed things is a good place to start.

== RELEASE NOTES ==

General Changes
* Add interruption for runaway splits blocked in known situations controlled by ``task.interrupt-runaway-splits-timeout`` property which defaults to ``600s``.

arhimondr

Some comments

presto-main/src/main/java/com/facebook/presto/execution/TaskManagerConfig.java

presto-main/src/main/java/com/facebook/presto/execution/executor/TaskExecutor.java

aweisberg · 2021-05-18T16:03:05Z

I didn't add reviewers to this because it isn't ready.

aweisberg · 2021-05-18T20:32:48Z

This does work in practice 20210518_201931_00000_y9r9j

aweisberg · 2021-05-19T00:21:12Z

OK, open season!

highker · 2021-05-19T02:05:18Z

presto-main/src/main/java/com/facebook/presto/operator/scalar/JoniRegexpFunctions.java

We can remove option in the param and put Option.DEFAULT directly in the body.

This commit is going to be landed by @kaikalur in #16109
I'll ask for it there.

presto-main/src/main/java/com/facebook/presto/execution/TaskManagerConfig.java

highker · 2021-05-19T02:34:22Z

presto-main/src/main/java/com/facebook/presto/execution/executor/TaskExecutor.java

why not config.getInterruptRunawaySplitsTimeout() directly?

This is a test constructor and there is no accessible TaskManagerConfig I think? Or you mean instantiate the config and then request the value? I can do that if it is what you are thinking.

Yes, given this is for test only, we can do that

highker · 2021-05-19T02:43:26Z

presto-main/src/main/java/com/facebook/presto/execution/executor/TaskExecutor.java

Instead of a blacklist, shall we use TaskManager::getAllTaskInfo to decide if a task is still alive and tracked to decide if we want to preempt the current thread?

If the task is no longer alive then the threads have already been interrupted so there is maybe no point in interrupting them again. So to some extent just making the Joni regex functions interruptible is already a win because it will make query cancellation and timeout work better.

What this brings to the table is that in a specific allow listed cases we will interrupt the splits with a timeout that is different (shorter) from the full query timeout which may be in the many hours range. The idea being this a step towards more automatically and aggressively remediating runaway splits.

highker

nits only

highker · 2021-05-20T17:15:52Z

presto-main/src/main/java/com/facebook/presto/execution/executor/TaskExecutor.java

If these defaults are not going to be changed, I guess we don't need to pass them in through parameters? Just directly use them in the body of the class?

Different ones are passed in for unit tests so the tests can complete quickly.

highker · 2021-05-20T17:17:35Z

presto-main/src/main/java/com/facebook/presto/execution/executor/TaskExecutor.java

Yes, given this is for test only, we can do that

highker · 2021-05-20T17:17:55Z

presto-main/src/main/java/com/facebook/presto/execution/executor/TaskExecutor.java

same here; we could avoid all defaults through parameters

Same thing RE making the unit test run quickly

presto-main/src/main/java/com/facebook/presto/execution/executor/TaskExecutor.java

presto-main/src/main/java/com/facebook/presto/execution/TaskManagerConfig.java

presto-main/src/main/java/com/facebook/presto/execution/executor/TaskExecutor.java

aweisberg · 2021-05-24T14:24:18Z

@highker I am not 100% clear on what you are asking for with removing default parameters that aren't in TaskConfig? If it's not parameterized how should the unit test change the value so the test completes quickly?

viczhang861

typo in release note task.task.interrupt-runaway-splits-timeout

presto-main/src/main/java/com/facebook/presto/execution/executor/TaskExecutor.java

highker · 2021-05-25T06:04:40Z

There is a conflict; maybe rebase and we can merge this PR?

viczhang861

Check property name in release note.

Summary: OS link for Ref: prestodb#16111 The OS change interrupts long running spits stuck on JoniRegexpFunctions, internally we encounter issue on HashBuilderOperator.buildLookupSource. So added as an additional interrupt condition. Reviewers: #ldap_presto-core, sgurmeet Reviewed By: #ldap_presto-core, sgurmeet Subscribers: sgurmeet, O4263 subscribe to presto changes JIRA Issues: PRESTO-3695 Differential Revision: https://code.uberinternal.com/D6427367

aweisberg force-pushed the interrupt_runaway_splits branch from cee4828 to d3978a4 Compare May 17, 2021 23:12

arhimondr reviewed May 18, 2021

View reviewed changes

aweisberg force-pushed the interrupt_runaway_splits branch 5 times, most recently from 0235e3f to 4c3cab2 Compare May 18, 2021 23:10

aweisberg requested review from arhimondr and highker May 19, 2021 00:21

aweisberg force-pushed the interrupt_runaway_splits branch 2 times, most recently from e734eff to 4782de6 Compare May 19, 2021 02:04

highker reviewed May 19, 2021

View reviewed changes

aweisberg force-pushed the interrupt_runaway_splits branch from 4782de6 to 473288a Compare May 19, 2021 11:22

highker self-requested a review May 20, 2021 07:39

highker reviewed May 20, 2021

View reviewed changes

aweisberg force-pushed the interrupt_runaway_splits branch from 473288a to a607931 Compare May 24, 2021 14:21

aweisberg requested a review from highker May 24, 2021 14:22

aweisberg mentioned this pull request May 24, 2021

Use searchInterruptible for Joni regexp matching #16109

Merged

viczhang861 reviewed May 24, 2021

View reviewed changes

presto-main/src/main/java/com/facebook/presto/execution/executor/TaskExecutor.java Outdated Show resolved Hide resolved

aweisberg force-pushed the interrupt_runaway_splits branch 2 times, most recently from fce61da to ac0e0e7 Compare May 24, 2021 21:16

highker approved these changes May 25, 2021

View reviewed changes

highker self-assigned this May 25, 2021

Interrupt runaway splits in known locations

e7c97d7

aweisberg force-pushed the interrupt_runaway_splits branch from ac0e0e7 to e7c97d7 Compare May 25, 2021 18:53

aweisberg requested a review from viczhang861 May 25, 2021 22:28

viczhang861 approved these changes May 26, 2021

View reviewed changes

highker merged commit 8e8a546 into prestodb:master May 26, 2021

jainxrohit mentioned this pull request Jun 5, 2021

Add release notes for 0.255 #16216

Merged

4 tasks

ajaygeorge mentioned this pull request Jun 9, 2021

Add release notes for 0.255 #16232

Closed

4 tasks

findepi mentioned this pull request May 16, 2022

Enable Trino to detect and fail queries that have tasks stuck in long running JONI parsing. trinodb/trino#12392

Merged

Conversation

aweisberg commented May 17, 2021 • 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!

aweisberg commented May 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aweisberg commented May 18, 2021

Uh oh!

aweisberg commented May 19, 2021

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!

highker left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aweisberg commented May 24, 2021

Uh oh!

viczhang861 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

highker commented May 25, 2021

Uh oh!

viczhang861 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aweisberg commented May 17, 2021 •

edited

Loading

aweisberg commented May 18, 2021 •

edited

Loading