Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/1051#discussion_r153403438
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java ---
@@ -873,105 +791,133 @@ public void close() throws Exception {
}
}
- private void moveToState(final QueryState newState, final Exception
exception) {
- logger.debug(queryIdString + ": State change requested {} --> {}",
state, newState,
- exception);
+ public synchronized void moveToState(final QueryState newState, final
Exception exception) {
+ logger.debug(queryIdString + ": State change requested {} --> {}",
state, newState, exception);
switch (state) {
- case ENQUEUED:
- switch (newState) {
- case FAILED:
- Preconditions.checkNotNull(exception, "exception cannot be null
when new state is failed");
- recordNewState(newState);
- foremanResult.setFailed(exception);
- foremanResult.close();
- return;
+ case PLANNING:
+ switch (newState) {
+ case ENQUEUED:
+ recordNewState(newState);
+ enqueuedQueries.inc();
+ return;
+ case CANCELLATION_REQUESTED:
+ assert exception == null;
+ recordNewState(newState);
+ foremanResult.setCompleted(QueryState.CANCELED);
+ foremanResult.close();
+ return;
+ case FAILED:
+ assert exception != null;
+ recordNewState(newState);
+ foremanResult.setFailed(exception);
+ foremanResult.close();
+ return;
+ }
+ break;
+ case ENQUEUED:
+ enqueuedQueries.dec();
+ queryManager.markQueueWaitEndTime();
+ switch (newState) {
--- End diff --
This is exactly the kind of double-switch statement that is hard to
understand. Better, a method for each transition: `enqueue()`, `start()`, etc.
Each transition is then easier to reason about. What are the valid from states?
What are the accounting operations needed in each transition?
Even better would be to combine the logic for the state transition (adding
the query to the queue, say) along with the state transition logic, since often
the new state is the result not simply of a transition, but of some condition
(such as, when queueing, moving to either ENQUEUED, RUNNING or FAILED depending
on the queue result.)
Otherwise the logic has to be essentially duplicated.
---
