Resource Management related cleanup by tdcmeehan · Pull Request #14104 · prestodb/presto

tdcmeehan · 2020-02-14T07:49:30Z

Rebase of #12801

== NO RELEASE NOTE ==

wenleix

"Add query id to NoSuchElementException".

LGTM by comparing with be6f54d

wenleix

"Remove system startup minimum worker requirement" % one small difference after comparing with bfa2524

wenleix · 2020-03-03T05:21:59Z

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

@@ -174,6 +171,8 @@ public SqlQueryManager(

        this.clusterSizeMonitor = requireNonNull(clusterSizeMonitor, "clusterSizeMonitor is null");


note this line get removed in bfa2524 :)

wenleix

"Add DISPATCHING query states". LGTM compared with 270504e

wenleix

"Split out queued phase from QueryManager" LGTM compared with b846616

Note the removal of DistributedQueryRunner#getQueryInfo doesn't seem to be included in this PR?
(b846616#diff-179c18855b05981dabfa4d6e458f8695L418)

wenleix · 2020-03-03T06:20:06Z

presto-main/src/main/java/com/facebook/presto/dispatcher/QueuedStatementResource.java

Just double check: now slug is provided as a parameter, while in the old PR, slug is part of the path? (See b846616#diff-ef10213de38c18dc49c5303d7eea77a4R234)

Yes, I saw from our commit history we did this because our HTTP logging seems to exclude query params from the logs.

wenleix · 2020-03-03T06:24:27Z

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

This is not just a rebase conflict, looks like all references to unboundedExecutorService (was: queryExecutor) should have been removed entirely.

wenleix · 2020-03-03T06:28:11Z

presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java

Didn't see "slugValid" related method in this commit? (see b846616#diff-d65ad4547dcbc09fc621ac2ceddce6cdR217)

This is because we cherry picked this earlier in #13110

wenleix · 2020-03-03T06:29:13Z

presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java

Why change this to nextToken? :)

This was supposed to happen in a later commit. Fixed it.

wenleix

"Improve query event stats for immediately failed queries". LGTM (ref 5841142)

wenleix

"Remove Optional from QueryStateMachine resourceGroup". LGTM (ref e7989ed)

wenleix

"Simplify DispatchInfo construction" . LGTM. (ref 6872c94)

arhimondr

A lot of abstractions that makes the change a little bit hard to follow, but generally looks good. It looks like the queries map in the ExecutingStatementResource is not being cleaned. It should be fixed to avoid memory leaks.

arhimondr · 2020-03-04T18:57:31Z

presto-main/src/main/java/com/facebook/presto/dispatcher/DispatchManager.java

Why is this needed? Why not simply Futures.nonCancellationPropagating()?

Turns out it's a different semantic. An attempt to cancel this future has no effect, whereas cancelling the returned future from nonCancellationPropagating may be cancelled (however this will not be propagated to the inner future).

arhimondr · 2020-03-04T19:33:10Z

presto-main/src/main/java/com/facebook/presto/server/protocol/ExecutingStatementResource.java

Since PurgeQueriesRunnable is gone who's responsible for cleaning up the queries map?

arhimondr · 2020-03-04T19:35:11Z

presto-main/src/main/java/com/facebook/presto/server/protocol/ExecutingStatementResource.java

How about calling it notFound? badRequest implies 400 status.

What I'll do is, because every method that calls this passes in NOT_FOUND for the status, is I'll hardcode that NOT_FOUND and rename to notFound.

wenleix

"Fix handling of failures during query creation": LGTM (ref 3561452)

wenleix

"Simplify query manager stats tracking" LGTM (ref 9d53fc7)

wenleix

"Rename SqlQueryManagerStats to QueryManagerStats": LGTM. (ref a9a527c)

wenleix

"Add LocalCoordinatorLocation". LGTM (ref 1568cfa)

wenleix

"Add peak tasks to BasicQueryStats". LGTM. Do you need to add operator allocation now as well? (added in #14191)

wenleix

"Change local dispatch to finish immediately after query submission" . LGTM (ref ecea8fc)

wenleix

"Cleanup dispatcher executor management". LGTM (ref cdbe6a1). We use a BoundExecutor which seems to make sense.

wenleix · 2020-03-06T00:33:54Z

presto-main/src/main/java/com/facebook/presto/dispatcher/DispatchExecutor.java

Is it possible to also export the number of active threads in BoundedExecutor? -- this has been an operation pain when we want to monitor when BoundedExecutor is full.

BoundedExecutor does not expose the number of active threads, however the underlying executor itself does export itself as an MBean via ThreadPoolExecutorMBean

wenleix

"Catch errors from LocalDispatchQuery querySubmitter": LGTM (ref e5b414a)

wenleix

The last two commits LGTM:

"Fix result caching in protocol Query"
"Simplify token management in protocol Query "

tdcmeehan · 2020-03-06T20:38:45Z

A lot of abstractions that makes the change a little bit hard to follow, but generally looks good. It looks like the queries map in the ExecutingStatementResource is not being cleaned. It should be fixed to avoid memory leaks.

This is a really great catch. Thank you.

Co-authored-by: Dain Sundstrom <dain@iq80.com> Co-authored-by: Raghav Sethi <raghavsethi.rs@gmail.com>

The normal minimum worker requirement applied to all queries is sufficient to cover this case. Co-authored-by: Dain Sundstrom <dain@iq80.com> Co-authored-by: Raghav Sethi <raghavsethi.rs@gmail.com>

Co-authored-by: Dain Sundstrom <dain@iq80.com> Co-authored-by: Raghav Sethi <raghavsethi.rs@gmail.com>

resourceGroup is already required in QueryStateMachine Co-authored-by: Dain Sundstrom <dain@iq80.com> Co-authored-by: Raghav Sethi <raghavsethi.rs@gmail.com>

Co-authored-by: Dain Sundstrom <dain@iq80.com> Co-authored-by: Raghav Sethi <raghavsethi.rs@gmail.com>

querySubmitter should never throw, but if it does fail the query immediately Co-authored-by: Dain Sundstrom <dain@iq80.com> Co-authored-by: Raghav Sethi <raghavsethi.rs@gmail.com>

Previously, the cache was effectively disabled for the first result, so a retry on first request resulted in a 410 gone. Co-authored-by: Dain Sundstrom <dain@iq80.com> Co-authored-by: Raghav Sethi <raghavsethi.rs@gmail.com>

Co-authored-by: Dain Sundstrom <dain@iq80.com> Co-authored-by: Raghav Sethi <raghavsethi.rs@gmail.com>

Co-authored-by: Dain Sundstrom <dain@iq80.com>

arhimondr

LGTM

wenleix · 2020-03-11T17:17:28Z

Merged! Thanks for the contribution! 😃 😃 😃

tdcmeehan mentioned this pull request Feb 14, 2020

Dispatcher Phase 1 #12801

Closed

tdcmeehan force-pushed the resourcem branch 3 times, most recently from d5e7a3e to 2079065 Compare February 17, 2020 03:11

tdcmeehan force-pushed the resourcem branch from 2079065 to 8b254b1 Compare February 27, 2020 04:49

tdcmeehan requested review from arhimondr and yingsu00 February 27, 2020 04:55

tdcmeehan changed the title ~~[WIP] Resource Management related cleanup~~ Resource Management related cleanup Feb 27, 2020

tdcmeehan requested a review from swapsmagic February 27, 2020 22:07

wenleix reviewed Mar 3, 2020

View reviewed changes

arhimondr reviewed Mar 4, 2020

View reviewed changes

wenleix reviewed Mar 5, 2020

View reviewed changes

tdcmeehan requested a review from cemcayiroglu March 6, 2020 00:44

wenleix reviewed Mar 6, 2020

View reviewed changes

tdcmeehan and others added 2 commits March 6, 2020 15:33

Add query id to NoSuchElementException

0fb77dc

Co-authored-by: Dain Sundstrom <dain@iq80.com> Co-authored-by: Raghav Sethi <raghavsethi.rs@gmail.com>

Remove system startup minimum worker requirement

9c42121

The normal minimum worker requirement applied to all queries is sufficient to cover this case. Co-authored-by: Dain Sundstrom <dain@iq80.com> Co-authored-by: Raghav Sethi <raghavsethi.rs@gmail.com>

tdcmeehan force-pushed the resourcem branch 6 times, most recently from 0262287 to 6a2c70c Compare March 8, 2020 07:05

tdcmeehan and others added 15 commits March 8, 2020 22:25

Split out queued phase from QueryManager

2a35115

Co-authored-by: Dain Sundstrom <dain@iq80.com> Co-authored-by: Raghav Sethi <raghavsethi.rs@gmail.com>

Add peak tasks to BasicQueryStats

1aedb21

Add LocalCoordinatorLocation

7510297

Co-authored-by: Dain Sundstrom <dain@iq80.com> Co-authored-by: Raghav Sethi <raghavsethi.rs@gmail.com>

Improve query event stats for immediately failed queries

6329d90

Co-authored-by: Dain Sundstrom <dain@iq80.com> Co-authored-by: Raghav Sethi <raghavsethi.rs@gmail.com>

Remove Optional from QueryStateMachine resourceGroup

d641e7b

resourceGroup is already required in QueryStateMachine Co-authored-by: Dain Sundstrom <dain@iq80.com> Co-authored-by: Raghav Sethi <raghavsethi.rs@gmail.com>

Simplify DispatchInfo construction

1c6a43f

Co-authored-by: Dain Sundstrom <dain@iq80.com> Co-authored-by: Raghav Sethi <raghavsethi.rs@gmail.com>

Rename SqlQueryManagerStats to QueryManagerStats

848f23a

Co-authored-by: Dain Sundstrom <dain@iq80.com> Co-authored-by: Raghav Sethi <raghavsethi.rs@gmail.com>

Simplify query manager stats tracking

a4d2f17

Co-authored-by: Dain Sundstrom <dain@iq80.com> Co-authored-by: Raghav Sethi <raghavsethi.rs@gmail.com>

Fix handling of failures during query creation

1b91f7a

Co-authored-by: Dain Sundstrom <dain@iq80.com> Co-authored-by: Raghav Sethi <raghavsethi.rs@gmail.com>

Cleanup dispatcher executor management

eb07101

Co-authored-by: Dain Sundstrom <dain@iq80.com> Co-authored-by: Raghav Sethi <raghavsethi.rs@gmail.com>

Change local dispatch to finish immediately after query submission

73918d1

Co-authored-by: Dain Sundstrom <dain@iq80.com> Co-authored-by: Raghav Sethi <raghavsethi.rs@gmail.com>

Catch errors from LocalDispatchQuery querySubmitter

1a4ef68

querySubmitter should never throw, but if it does fail the query immediately Co-authored-by: Dain Sundstrom <dain@iq80.com> Co-authored-by: Raghav Sethi <raghavsethi.rs@gmail.com>

Fix result caching in protocol Query

a855e13

Previously, the cache was effectively disabled for the first result, so a retry on first request resulted in a 410 gone. Co-authored-by: Dain Sundstrom <dain@iq80.com> Co-authored-by: Raghav Sethi <raghavsethi.rs@gmail.com>

Simplify token management in protocol Query

823a0e5

Co-authored-by: Dain Sundstrom <dain@iq80.com> Co-authored-by: Raghav Sethi <raghavsethi.rs@gmail.com>

Add purger to ExecutingStatementResource

95e5b4c

Co-authored-by: Dain Sundstrom <dain@iq80.com>

tdcmeehan force-pushed the resourcem branch from 3bf347c to 95e5b4c Compare March 9, 2020 05:28

tdcmeehan requested review from arhimondr and wenleix March 9, 2020 19:30

wenleix mentioned this pull request Mar 10, 2020

Keep queries in the queue when task count is high #14194

Merged

arhimondr approved these changes Mar 11, 2020

View reviewed changes

wenleix approved these changes Mar 11, 2020

View reviewed changes

wenleix merged commit 7bfc203 into prestodb:master Mar 11, 2020

kevintang2022 mentioned this pull request Feb 18, 2025

Add checkQueryIntegrity call to DispatchManager #24574

Merged

6 tasks

		@@ -174,6 +171,8 @@ public SqlQueryManager(

		this.clusterSizeMonitor = requireNonNull(clusterSizeMonitor, "clusterSizeMonitor is null");

Conversation

tdcmeehan commented Feb 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wenleix left a comment

Choose a reason for hiding this comment

Uh oh!

wenleix left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenleix left a comment

Choose a reason for hiding this comment

Uh oh!

wenleix 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!

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!

wenleix left a comment

Choose a reason for hiding this comment

Uh oh!

wenleix left a comment

Choose a reason for hiding this comment

Uh oh!

wenleix left a comment

Choose a reason for hiding this comment

Uh oh!

arhimondr 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenleix left a comment

Choose a reason for hiding this comment

Uh oh!

wenleix left a comment

Choose a reason for hiding this comment

Uh oh!

wenleix left a comment

Choose a reason for hiding this comment

Uh oh!

wenleix left a comment

Choose a reason for hiding this comment

Uh oh!

wenleix left a comment

Choose a reason for hiding this comment

Uh oh!

wenleix left a comment

Choose a reason for hiding this comment

Uh oh!

wenleix left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tdcmeehan commented Feb 14, 2020 •

edited

Loading

arhimondr left a comment •

edited

Loading