Transition to executing when begin planning

[JunhyungSong]

Description

Currently, /v1/statement/queued/{queryId}/{slug}/{token} get request returns queued query state even if the query is already planning. It will not be redirected to /v1/statement/executing/{queryId}/{slug}/{token} get request until all planning tasks including plan distribution is completed. When planning takes long time or fails during planning, it can mislead callers because of this. So, it needs to set submitted when transitioning to PLANNING.

Additional context and related issues

Release notes

(v) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

[findepi]

@JunhyungSong thanks for a very nice PR description, this is very useful!

Currently, /v1/statement/queued/{queryId}/{slug}/{token} get request returns queued query state even if the query is already planning. It will not be redirected to /v1/statement/executing/{queryId}/{slug}/{token} get request until all planning tasks including plan distribution is completed. When planning takes long time or fails during planning, it can mislead callers because of this.

Callers should not infer any particular meaning from the paths used in nextUri.

Callers (client tools, UIs), should inspect the query state, so we should make sure the query state returned as "PLANNING" if the query is in the planning state.

So, it needs to set submitted when transitioning to PLANNING.

What is the effect of this (from the query protocol perspective)?

[JunhyungSong]

Callers (client tools, UIs), should inspect the query state, so we should make sure the query state returned as "PLANNING" if the query is in the planning state.

This PR is for that. Since callers should not infer nextUri as you mentioned, the engine needs to provide a right nextUri within the response for the caller's requests.

What is the effect of this (from the query protocol perspective)?

This will mark dispatched as true. And it will set coordinator location and change the nextUri to ExecutingStatementResource instead of QueuedStatementResource. With that, ExecutingStatementResource will return QueryResult with a correct state(e.g. PLANNING or STARTING instead of QUEUED).

[findepi]

will return QueryResult with a correct state(e.g. PLANNING or STARTING instead of QUEUED).

that's sounds like something observable from end user perspective.

So, are you saying that queries in PLANNING state are not viewed as such by the client tools?
would it be possible to write a test for that?
for example with a mock connector you can insert a pause in some metadata method, allowing planning to take as much time as needed for test purposes.

[JunhyungSong]

So, are you saying that queries in PLANNING state are not viewed as such by the client tools?

Correct.

would it be possible to write a test for that?
for example with a mock connector you can insert a pause in some metadata method, allowing planning to take as much time as needed for test purposes.

Can you recommend tests that I can refer to?

[findepi]

I do not. @electrum and @dain will know better for protocol-level changes and tests

[JunhyungSong]

@electrum @dain do you have any ideas on how to add tests for this?

[github-actions]

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

[bitsondatadev]

@pettyjamesm would you also be able to review this one or approve if you've already done a review internally please?

[pettyjamesm]

A thrown RejectedExecutionException will now trigger query failure instead of a stuck query in the previous implementation- which is good and probably something we could test for. Also note that a failure in minimumWorkerFuture could also have RejectedExecutionException thrown from queryExecutor and should probably just call stateMachine.transitionToFailed(throwable) inline instead of queueing it through queryExecutor.

The above was incorrect, queryExecutor is a cached threadpool executor and will not throw.

[JunhyungSong]

Talked offline. DispatchExecutor will not throw RejectedExecutionException unless the server is shutting down.

[pettyjamesm]

I'm good with this formulation of submitting a new future to handle queryExecution.start(), but would it be equivalent to call submitted.set(null); queryExecution.start(); here without a shift to another thread in the same threadpool?

[JunhyungSong]

Makes sense.

[pettyjamesm]

I think closeExchangeIfNecessary(QueryInfo) needs to check queryInfo.getState().ordinal() > STARTING logic too, otherwise we could close the exchange before planning completes if that were to be called from another code path.

After that change, we should leave the closeExchangeIfNecessary and removePagesFromExchange as-is without needing to check isStarted at this point explicitly.

[JunhyungSong]

As chatted separately, removePagesFromExchange needs the condition check as well since it needs to return null for columns and types when the query is not started, yet. And the other code path of calling closeExchangeIfNecessary(QueryInfo) already checks a query completion.

[pettyjamesm]

I would say that relying on two separate code-paths to check the same condition before calling closeExchangeIfNecessary(QueryInfo) is riskier than just having closeExchangeIfNecessary do the check internally (more robust than refactoring).

[JunhyungSong]

Actually the condition here at https://github.com/trinodb/trino/blob/master/core/trino-main/src/main/java/io/trino/server/protocol/Query.java#L198 is a little different. And there will be three separate code-paths to check the same condition if we move the condition check. The first is inside closeExchangeIfNecessary. The second is inside removePagesFromExchange. And the third is for if (isStarted && (queryInfo.getOutputStage().isEmpty() || exchangeDataSource.isFinished())).

[pettyjamesm]

I'm good with this formulation of submitting a new future to handle queryExecution.start(), but would it be equivalent to call submitted.set(null); queryExecution.start(); here without a shift to another thread in the same threadpool?

[pettyjamesm]

I would say that relying on two separate code-paths to check the same condition before calling closeExchangeIfNecessary(QueryInfo) is riskier than just having closeExchangeIfNecessary do the check internally (more robust than refactoring).

[pettyjamesm]

There's a lot of ceremony going on here to override the whole ServerMainModule when we could just pass TestingQueryExecution directly into a LocalDispatchQuery constructor instead and assert off of that, which would be much simpler to verify.

[pettyjamesm]

If we change TestingQueryExecution to use a CountdownLatch inside of QueryExecution#start() then we can just await that countdown instead of having to do a sleep-wait loop here.

[pettyjamesm]

This change can be avoided (since it's only to make something overrideable in tests) by just passing the TestQueryExecution directly to the LocalDispatchQuery and skipping all the dependency injection ceremony.

[JunhyungSong]

The test failures seem irrelevant.

[dain]

For the Transition to executing when begin planning commit, it is really important for me to be able to understand the diff if the commit contains only the changes absolutely necessary for the desired change to the state transitions (the part of the code base is really complex). The other minor changes can be in commits before of after that one.

[JunhyungSong]

The test failure seems irrelevant.

[dain]

I'm still having some trouble understanding some of the additional changes in this. Also, I think the main commit can be simplified by rewriting the test.

[dain]

We don't use abbreviations like this

[dain]

We should not be making this field mutable for a test. I'll explain how in the test code below.

[dain]

This test is using TpchQueryRunner, which internally uses DistributedQueryRunner and you should just use that directly. Once you do that, you can add an additional Guice module to the runner. In the module you can register a new DDL task for some mocked up statement (see QueryExecutionFactoryModule). Then submit the fake statement directly using LocalDispatchQuery (which you can get from the injector the coordinator).

Alternatively, you can just create a LocalDispatchQuery directly and test the state changes you expect are happening on the state machine transitions.

[JunhyungSong]

The failure looks irrelevant.