Remove SqlQueryScheduler#abort method #13416
Conversation
| // directly since the callback may have already fired | ||
| if (stateMachine.isDone()) { | ||
| scheduler.abort(); | ||
| queryScheduler.set(null); |
There was a problem hiding this comment.
Shouldn't queryScheduler.set(null); be called anyway (e.g. to free resources)?
There was a problem hiding this comment.
The queryScheduler variable is nullified in buildQueryInfo after building a final query info. It is currently not nullified in case of a failure or completion anywhere else, thus it seemed to be weird to nullify it here.
There was a problem hiding this comment.
It is currently not nullified in case of a failure or completion anywhere else, thus it seemed to be weird to nullify it here.
So was this a race between buildQueryInfo and nullification here?
There was a problem hiding this comment.
I doubt there any. It looked very weird. Later on there's another check where the scheduler is not getting nullified: https://github.com/trinodb/trino/blob/master/core/trino-main/src/main/java/io/trino/execution/SqlQueryExecution.java#L423.
| } | ||
|
|
||
| // query is now done, so abort any work that is still running | ||
| SqlQueryScheduler scheduler = queryScheduler.get(); |
There was a problem hiding this comment.
Do we have test for aborting? Can it be that this listener covers some flow that is not covered by SqlQueryScheduler?
There was a problem hiding this comment.
Today the SqlQueryScheduler is responsible of attaching the listeners that terminate the scheduling process upon query completion (due to a failure / cancellation or early finish): https://github.com/trinodb/trino/blob/master/core/trino-main/src/main/java/io/trino/execution/scheduler/SqlQueryScheduler.java#L303. SqlQueryScheduler attaches the listener before the scheduling process starts and uses an internal lock to prevent race conditions.
These listeners do exactly what the abort method does. Having an external listener calling the abort method is not necessary and might be confusing for the readers.
| // directly since the callback may have already fired | ||
| if (stateMachine.isDone()) { | ||
| scheduler.abort(); | ||
| queryScheduler.set(null); |
There was a problem hiding this comment.
It is currently not nullified in case of a failure or completion anywhere else, thus it seemed to be weird to nullify it here.
So was this a race between buildQueryInfo and nullification here?
SqlQueryScheduler registers a query state listener internally to terminate query scheduling when the query is cancelled, failed or finished early
arhimondr
force-pushed
the
drop-scheduler-abort-method
branch
from
ba50d50 to
671aeb9
Compare
Description
SqlQueryScheduler registers a query state listener internally to
terminate query scheduling when the query is cancelled, failed or
finished early
Refactor
Core engine
Non user visible
Related issues, pull requests, and links
-Documentation
(X) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.
Release notes
(X) No release notes entries required.
( ) Release notes entries required with the following suggested text: