Dispatcher by dain · Pull Request #95 · trinodb/trino

dain · 2019-01-29T06:45:15Z

Move queued phase of query from QueryManager to a new dispatcher service. This
change is in preparation for adding a optional new server that moves the queue
phase to a separate process.

Ref prestodb/presto#12176

dain · 2019-01-29T06:51:24Z

Remove system startup minimum worker requirement

is this removing a functionality or moving it elsewhere?

@findepi I added a new system which waits for a minimum number of workers after queuing is complete and before execution starts. The min worker count is configured with query-manager.required-workers-max-wait. This is removes the old system which failed queries until the cluster started.

sopel39 · 2019-01-29T09:37:08Z

The min worker count is configured with query-manager.required-workers-max-wait

So the semantics is that queries will be queued until there is min number of workers. What if workers leave the cluster after the cluster is started? Will the new queries be on hold too? This is useful in case of downscaling cluster to 0 nodes for idle periods in cloud environment.

dain · 2019-01-30T05:41:16Z

The min worker count is configured with query-manager.required-workers-max-wait

So the semantics is that queries will be queued until there is min number of workers. What if workers leave the cluster after the cluster is started? Will the new queries be on hold too? This is useful in case of downscaling cluster to 0 nodes for idle periods in cloud environment.

Yes. The new behavior is designed for cloud environments that scale down to zero when there is no traffic and then scale back up when there are queries. BTW, that feature is already checked in, so you can use it now.

electrum · 2019-02-22T22:44:50Z

First three commits look good

raghavsethi · 2019-02-27T03:32:30Z

Can we add a commit prior that adds a specific error code (not generic insufficient resources), so that clients can deal with that situation properly?

raghavsethi · 2019-02-28T01:27:15Z

Remote Optional -> Remove Optional

raghavsethi

These commits LGTM

Add query id to NoSuchElementException
Remove system startup minimum worker requirement
Add DISPATCHING query states

raghavsethi · 2019-03-14T23:24:54Z

requireNonNull for queryManagerConfig

raghavsethi · 2019-03-15T17:12:57Z

This export has no effect. I looked in JMX and the counter does not appear.

findepi · 2019-03-29T07:55:22Z

+        private final SessionContext sessionContext;
+        private final DispatchManager dispatchManager;
+        private final QueryId queryId;
+        private final String slug = format("%016x%016x", ThreadLocalRandom.current().nextLong(), ThreadLocalRandom.current().nextLong());


#561 (comment)

findepi · 2019-03-29T07:56:30Z

+
    QueryInfo getQueryInfo();

+    String getSlug();


#561 (comment)

findepi · 2019-03-29T07:56:55Z

+            return queryId;
+        }
+
+        public String getSlug()


#561 (comment) ?

raghavsethi

Following commits LGTM:

Remove system startup minimum worker requirement
Add DISPATCHING query states
Split out queued phase from QueryManager
Add query id to NoSuchElementException
Improve query event stats for immediately failed queries
Remove Optional from QueryStateMachine resourceGroup
Change local dispatch to finish immediately after query submission

raghavsethi · 2019-04-03T00:20:48Z

For my own reference: here's the bug from yesterday.

raghavsethi · 2019-04-03T00:26:05Z

Nit? Edge case for static import?

raghavsethi

Following commits look good:

Remove Optional from QueryStateMachine resourceGroup
Simplify DispatchInfo construction
Fix handling of failures during query creation
Simplify query manager stats tracking

raghavsethi · 2019-04-16T19:00:28Z

Nit: If you named these more specifically (eg queuedDispatchInfo), you could static import.

I go back and forth on this. In this case I like the FQN.

raghavsethi

Following commits LGTM % nits:

Rename SqlQueryManagerStats to QueryManagerStats
Cleanup dispatcher executor management

raghavsethi · 2019-04-26T18:36:26Z

Are we moving to the closer vs the annotation pattern?

I'm not sure what you mean. This class uses a closer and an @PreDestroy

raghavsethi

Following commits look good:

Fixup! Cleanup dispatcher executor management
Remove bad call to recordHeartbeat in dispatch query
Fix visibility of failed queries in LocalDispatchQuery
Make protocol Query public
Catch errors from LocalDispatchQuery querySubmitter

wenleix · 2019-04-30T20:15:23Z

Curious: Why this is called LocalDispatchQuery ?

raghavsethi · 2019-05-13T17:09:00Z

raghavsethi · 2019-05-13T17:09:09Z

The normal minimum worker requirement applied to all queries is sufficient to cover this case.

A query will be in the DISPATCHING state during handoff to a query execution coordinator.

resourceGroup is already required in QueryStateMachine

querySubmitter should never throw, but if it does fail the query immediately

Previously, the cache was effectively disabled for the first result, so a retry on first request resulted in a 410 gone.

cla-bot bot added the cla-signed label Jan 29, 2019

dain self-assigned this Jan 30, 2019

dain force-pushed the dispatcher branch 2 times, most recently from 69c5323 to 5bc1cd7 Compare February 5, 2019 04:21

raghavsethi reviewed Feb 5, 2019

View reviewed changes

Comment thread presto-main/src/main/java/io/prestosql/execution/QueryState.java Outdated

dain force-pushed the dispatcher branch from 5bc1cd7 to a07b6dc Compare February 7, 2019 16:12

dain force-pushed the dispatcher branch from a07b6dc to 35148b9 Compare February 23, 2019 20:00

dain force-pushed the dispatcher branch from 35148b9 to aa36fdd Compare February 28, 2019 00:52

raghavsethi reviewed Feb 28, 2019

View reviewed changes

dain mentioned this pull request Mar 6, 2019

High Availability #391

Open

dain force-pushed the dispatcher branch 5 times, most recently from d461996 to c512833 Compare March 12, 2019 05:37

raghavsethi reviewed Mar 15, 2019

View reviewed changes

dain force-pushed the dispatcher branch 2 times, most recently from fe558b1 to fdbc795 Compare March 15, 2019 23:08

findepi reviewed Mar 29, 2019

View reviewed changes

dain force-pushed the dispatcher branch from 55f9a42 to 6472ee9 Compare March 30, 2019 21:31

raghavsethi reviewed Apr 3, 2019

View reviewed changes

raghavsethi reviewed Apr 17, 2019

View reviewed changes

sopel39 mentioned this pull request Apr 17, 2019

The number of workers is not perfectly right if more than one coordinators exist in the same cluster #641

Open

raghavsethi reviewed Apr 29, 2019

View reviewed changes

raghavsethi mentioned this pull request Apr 29, 2019

Limit query submition threads prestodb/presto#12738

Merged

dain force-pushed the dispatcher branch from 8a41247 to a7a1b4e Compare April 29, 2019 23:51

wenleix reviewed Apr 30, 2019

View reviewed changes

dain force-pushed the dispatcher branch from a7a1b4e to 108fa64 Compare May 12, 2019 22:03

raghavsethi approved these changes May 13, 2019

View reviewed changes

raghavsethi mentioned this pull request May 14, 2019

Dispatcher Phase 1 prestodb/presto#12801

Closed

dain added 16 commits May 16, 2019 11:18

Add query id to NoSuchElementException

3820120

Remove system startup minimum worker requirement

b892703

The normal minimum worker requirement applied to all queries is sufficient to cover this case.

Add DISPATCHING query states

85470d3

A query will be in the DISPATCHING state during handoff to a query execution coordinator.

Split out queued phase from QueryManager

a4df8dd

Add LocalCoordinatorLocation

f5a9f5a

Improve query event stats for immediately failed queries

869fd21

Remove Optional from QueryStateMachine resourceGroup

922b340

resourceGroup is already required in QueryStateMachine

Simplify DispatchInfo construction

6635539

Rename SqlQueryManagerStats to QueryManagerStats

f8094f7

Simplify query manager stats tracking

436471d

Fix handling of failures during query creation

cff9551

Cleanup dispatcher executor management

6583705

Change local dispatch to finish immediately after query submission

82f60e8

Catch errors from LocalDispatchQuery querySubmitter

a756e64

querySubmitter should never throw, but if it does fail the query immediately

Fix result caching in protocol Query

fce9bff

Previously, the cache was effectively disabled for the first result, so a retry on first request resulted in a 410 gone.

Simplify token management in protocol Query

ece063a

dain force-pushed the dispatcher branch from 108fa64 to ece063a Compare May 16, 2019 18:23

dain merged commit a5b6169 into trinodb:master May 16, 2019

dain deleted the dispatcher branch June 16, 2019 03:31

yohengyang mentioned this pull request May 11, 2024

Proposal to Optimize Trino Hive Metastore Query Latency by Caching createMetastoreClient() #21671

Open

Conversation

dain commented Jan 29, 2019

Uh oh!

dain commented Jan 29, 2019

Uh oh!

sopel39 commented Jan 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dain commented Jan 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

electrum commented Feb 22, 2019

Uh oh!

raghavsethi commented Feb 27, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

raghavsethi Mar 15, 2019 • 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!

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

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

Uh oh!

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

Uh oh!

Uh oh!

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

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

sopel39 commented Jan 29, 2019 •

edited

Loading

dain commented Jan 30, 2019 •

edited

Loading

raghavsethi left a comment •

edited

Loading

raghavsethi Mar 15, 2019 •

edited

Loading

raghavsethi left a comment •

edited

Loading