Clean up SqlQueryManager by dain · Pull Request #11518 · prestodb/presto

dain · 2018-09-19T00:49:17Z

Fix warnings
Extract out complex book keeping like query timeout tracking
Simplify session creation code
Prepare manager for split into dispatcher and coordinator

facebook-github-bot · 2018-09-22T00:08:48Z

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired.

Before we can review or merge your code, we need you to email cla@fb.com with your details so we can update your status.

raghavsethi

Fix IntelliJ warnings LGTM

raghavsethi

Move worker specific bindings to WorkerModule LGTM

raghavsethi

Fix raw use of ResourceGroupManager type LGTM. Thank you for cleaning up my mess.

raghavsethi

Add abstraction for applying default session properties LGTM with nits.

presto-main/src/main/java/com/facebook/presto/Session.java

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

dain · 2018-09-26T20:07:59Z

The changes to auto commit handling depend on #11511. The TestTpchDistributedStats ends up using com.facebook.presto.tests.DistributedQueryRunner#createPlan which cancel's the query, triggering the auto commit cleanup, so later when com.facebook.presto.tests.statistics.MetricComparator#getEstimatedValuesInternal tries to calculate stats and access the metadata, and error is thrown because the transaction is gone.

raghavsethi

Add abstraction for processing prepared statement has minor comments.

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

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

raghavsethi

Add abstraction to manage abandonment and expiration of queries LGTM. As far as I can tell, this a pure refactor, no functional changes, right?

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

raghavsethi · 2018-09-27T00:10:35Z

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

How would this appear in debugging/ops tools? What would the object name be?

Actually, this is totally wrong. QueryTracker is not exported to JMX, so it won't show up at all. I'm going to change query tracker to take the executor in the constructor, and will use the existing executor in the SqlQueryManager, so there will be no change to the existing JMX exports.

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

raghavsethi

Simplify FailedQueryExecution has some question marks

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

raghavsethi · 2018-10-01T17:29:31Z

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

I've never seen a public logger shared between classes. Not sure why we went in this direction.

The QUERY_STATE_LOG is a special log that all of the query state changes are logged to, and I want to maintain that behavior.

raghavsethi · 2018-10-01T17:30:49Z

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

Don't all failed queries become a FailedQueryExecution (including ones with resource groups)?

It makes the code a bit messier. How important is it to have this?

raghavsethi

Inline FailedQueryExecution static factory LGTM

raghavsethi

Remove unnecessary metadata cleanup in SqlQueryManager LGTM assuming you're sure that there is no case where metadata is created for a failed execution.

electrum · 2018-10-02T00:23:30Z

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

I thought there might be a race between the two calls, but it seems the only way to remove the transaction is to call asyncAbort which is guarded by the state machine.

the extra call is required for ROLLBACK which immediately removes the transaction from the manager, so without the exists check the call to isAutoCommit fails. I looked as having an explicit check for ROLLBACK here, but that would require breaking to many abstractions. I also looked at making isAutoCommit, return Optional, or just false, for unknown queries, but that makes is difficult to work with in the normal case.

electrum · 2018-10-02T00:28:27Z

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

I think we can get rid of transactionControl now, here and all the way up, including in DataDefinitionTask.

@electrum transactionControl is used on the next line of this code: if (!session.getTransactionId().isPresent() && !transactionControl) {

dain · 2018-10-09T00:16:23Z

@raghavsethi changes applied. Please review again.

electrum · 2018-10-09T20:21:19Z

I reviewed the two transaction related commits and they look good.

raghavsethi · 2018-10-16T21:31:00Z

Looks good to merge after rebase + tests.

raghavsethi

Looks good.

Application of defaults during session creation conflicts with resource group selection and query parsing. Separating out the application of default session properties allows us to refactor query creation in SqlQueryManager.

Processing and validation of prepared statements is complex and performed in several locations.

Instead of using a QueryStateMachine to create QueryInfo, add helpers to directly create a QueryInfo for a failed query. Also add cleanup performed by QueryStateMachine to FailedQueryExecution.

When a query fails during construction of QueryExecution, the metadata system for the query has not be initialized, so clean up of metadata is not necessary.

facebook-github-bot added the CLA Signed label Sep 19, 2018

raghavsethi self-assigned this Sep 19, 2018

raghavsethi self-requested a review September 19, 2018 00:51

dain force-pushed the SqlQueryManager-cleanup branch 2 times, most recently from 8c52864 to f406efc Compare September 19, 2018 00:58

raghavsethi mentioned this pull request Sep 20, 2018

Fix raw usages of ResourceGroupManager #10532

Closed

dain force-pushed the SqlQueryManager-cleanup branch from f406efc to 0161718 Compare September 26, 2018 00:19

raghavsethi reviewed Sep 26, 2018

View reviewed changes

raghavsethi suggested changes Sep 26, 2018

View reviewed changes

raghavsethi reviewed Sep 27, 2018

View reviewed changes

raghavsethi reviewed Oct 1, 2018

View reviewed changes

electrum reviewed Oct 2, 2018

View reviewed changes

dain force-pushed the SqlQueryManager-cleanup branch 3 times, most recently from c883d82 to 497e2e5 Compare October 8, 2018 22:49

dain force-pushed the SqlQueryManager-cleanup branch 4 times, most recently from c5faf04 to 83568a2 Compare October 12, 2018 21:55

dain force-pushed the SqlQueryManager-cleanup branch from 83568a2 to 26c7586 Compare October 17, 2018 00:25

dain force-pushed the SqlQueryManager-cleanup branch from 26c7586 to 921fcf2 Compare October 17, 2018 01:13

raghavsethi approved these changes Oct 17, 2018

View reviewed changes

dain added 11 commits October 16, 2018 20:54

Fix IntelliJ warnings

7c3a7b5

Move worker specific bindings to WorkerModule

5a083ab

Fix raw use of ResourceGroupManager type

730e50a

Add abstraction for applying default session properties

aa2a75f

Application of defaults during session creation conflicts with resource group selection and query parsing. Separating out the application of default session properties allows us to refactor query creation in SqlQueryManager.

Add abstraction for processing prepared statement

7ea3cd5

Processing and validation of prepared statements is complex and performed in several locations.

Add abstraction to manage abandonment and expiration of queries

8dc7e5a

Simplify FailedQueryExecution

18b15b7

Instead of using a QueryStateMachine to create QueryInfo, add helpers to directly create a QueryInfo for a failed query. Also add cleanup performed by QueryStateMachine to FailedQueryExecution.

Inline FailedQueryExecution static factory

dd48fd6

Remove unnecessary metadata cleanup in SqlQueryManager

b508b01

When a query fails during construction of QueryExecution, the metadata system for the query has not be initialized, so clean up of metadata is not necessary.

Remove extra tracking of auto commit in QueryStateMachine

bcf0324

Move transaction activation to SqlQueryManager

2002a15

dain force-pushed the SqlQueryManager-cleanup branch from 921fcf2 to 2002a15 Compare October 17, 2018 03:54

dain closed this Oct 17, 2018

dain deleted the SqlQueryManager-cleanup branch October 17, 2018 03:54

dain merged commit 2002a15 into prestodb:master Oct 17, 2018

wenleix mentioned this pull request Aug 27, 2019

Enforce task count limit #13228

Merged

Conversation

dain commented Sep 19, 2018

Uh oh!

facebook-github-bot commented Sep 22, 2018

Uh oh!

raghavsethi left a comment

Choose a reason for hiding this comment

Uh oh!

raghavsethi left a comment

Choose a reason for hiding this comment

Uh oh!

raghavsethi left a comment

Choose a reason for hiding this comment

Uh oh!

raghavsethi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dain commented Sep 26, 2018

Uh oh!

raghavsethi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

raghavsethi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dain commented Oct 9, 2018

Uh oh!

electrum commented Oct 9, 2018

Uh oh!

raghavsethi commented Oct 16, 2018

Uh oh!

raghavsethi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone