Planning time limit

[skrzypo987]

No description provided.

[skrzypo987]

Added a test that freezes during planning. To my amazement it got interrupted properly. Changed docs so that setting this limit will not guarantee instant termination of the query
Also changed the default to 100 days. Let's keep it backward-compatible and move the default value discussion to subsequent PR.

[findepi]

move the default value discussion to subsequent PR.

do you plan to make it?

[skrzypo987]

do you plan to make it?

Sure, it is easy. Just need to open a PR and propose an absurd default value. Then a lot of of Trino seniors will come arguing about the actual value and settle with something.
(Disclaimer: That was a joke)

[sopel39]

what did you mean here?

[skrzypo987]

The test needs to finish between 5s and 10s time window. Any task running in the background may increase chance of flakiness.

[sopel39]

I think the comment is not complete

[sopel39]

please review @mosabua

[m57lyra]

LGTM.

[sopel39]

I think the comment is not complete

[sopel39]

This might not be a planning thread anymore (when planning is finished) when query goes to FAILED state. In fact, this executor thread might be running planning for some other query. I think interruption needs to run in critical section:

1. planning thread is setting planningThread=null when planning is finished
2. other thread is calling interrupt

[skrzypo987]

The listener will fire only when the state changes from PLANNING to FAILED. Once planning is complete the state changes immediately to STARTING and the thread will not get interrupted.

[skrzypo987]

Ok, you are right, the thread might be reused. I will sync it.

[skrzypo987]

Fixed. PTAL

[sopel39]

lgtm % comments

[sopel39]

nit: why not 0? Duration itself can be 0.

[skrzypo987]

Removed the constraint to be consistent with max-execution-time

[sopel39]

This is racy, planning thread might have hiccup and set interrupted = true later than queryRunner.execute returns.

Use https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/CountDownLatch.html instead.

[martint]

Why is this a field in the class?

[skrzypo987]

It is used in async lambdas and cannot be a variable. So I could either decorate it in some AtomicReference type of object of make it a field.

[martint]

It doesn't need to be a field, then. You just need to do:

final Thread planningThread = currentThread();
stateMachine.getStateChange(PLANNING).addListener(() -> {
    if (stateMachine.getQueryState() == FAILED) {
        synchronized (this) {
            planningThread.interrupt();
        }
    }
}, directExecutor());

[skrzypo987]

It is being set to null later on

[martint]

It's not clear to me why we need that. The only reason would be to prevent the wrong thread from being interrupted, but that can't really happen. If the query transitions to FAILED during planning, it must still be executing the start() method, since all the planning methods are synchronous. It is only then that the thread is interrupted, which means it will be the right thread.

I can't think of a combination of actions that would result in start() completing and the query not already be in a state other than PLANNING, which means the interruption wouldn't happen beyond that point.

[skrzypo987]

This was implemented to prevent the interruption in an unlikely case of the thread being already reused for another query. This can be simulated by placing long sleep just before the interrupt() method. That way we can interrupt the next query executed in this thread.

[sopel39]

State change listener can execute any time in the future (even when planningThread started serving another query)

[martint]

Fair enough. The use of directExecutor() in the addListener call threw me off.

However, if can happen only if the planning fails from the call to fail() in the catch at the bottom of this method.

Still, I would write this code as follows to keep the state and logic self-contained and not have to reason about a class-level field, it's lifetime beyond this method (or other methods, etc). Ideally, we should be able to remove listeners, but there's no facility to do so:

final AtomicReference<Thread> planningThread = new AtomicReference<>(currentThread());
stateMachine.getStateChange(PLANNING).addListener(() -> {
    if (stateMachine.getQueryState() == FAILED) {
        synchronized (this) {
            Thread thread = planningThread.get();
            if (thread != null) {
                thread.interrupt();
            }
        }
    }
}, directExecutor());

try {
    PlanRoot plan = planQuery();
    // DynamicFilterService needs plan for query to be registered.
    // Query should be registered before dynamic filter suppliers are requested in distribution planning.
    registerDynamicFilteringQuery(plan);
    planDistribution(plan);
}
finally {
    synchronized (this) {
        planningThread.set(null);
        // clear the interrupted flag in case there was a race condition where
        // the planning thread was interrupted right after planning completes above
        Thread.interrupted();
    }
}

[sopel39]

However, if can happen only if the planning fails from the call to fail() in the catch at the bottom of this method.

Why only then? Query might be interrupted via timeout and thread.interrupt(); might be delivered in arbitrary time after that.

Other than that keeping logic self-contained within method makes sense

[martint]

Why only then? Query might be interrupted via timeout and thread.interrupt(); might be delivered in arbitrary time after that.

Ah yes, scratch that. I was thinking of the case where the interrupt causes those methods to throw.

[findepi]

How does this relate to the interrupt delivered here

trino/core/trino-main/src/main/java/io/trino/dispatcher/LocalDispatchQuery.java

Line 102 in f13283b

queryExecutionFuture.cancel(true);

?

[skrzypo987]

It is a different thread. queryExecutionFuture.cancel(true) interrupts thread started in LocalDispatchQueryFactory::createDispatchQuery.
The actual planning thread is started from that thread in LocalDispatchQuery::startExecution

[sopel39]

Did you push you changes?

[findepi]

(just skimming)

[skrzypo987]

@martint PTAL if the current state is ok for you

[martint]

It doesn't need to be a field, then. You just need to do:

final Thread planningThread = currentThread();
stateMachine.getStateChange(PLANNING).addListener(() -> {
    if (stateMachine.getQueryState() == FAILED) {
        synchronized (this) {
            planningThread.interrupt();
        }
    }
}, directExecutor());

[skrzypo987]

@martint I used the planningTime from QueryStateTimer to check the timeout. One commit less and not using executionStartTime. PTAL

[martint]

We need to make this much tighter for this test or come up with a better way to test this.

[skrzypo987]

What exactly do you mean?
The feature is about making best effort to stop the planning, so ensuring it works in some cases should be enough IMO

[martint]

I'm saying that 5s is to long to sleep in a test.

[martint]

This shouldn't be necessary. It should be taken care of by the thread pool.

[sopel39]

This thread does not go into pool immediately. This might not be a problem, but it seems cleaner to clear interrupted flag since we interrupt planning specifically. wdyt?

[martint]

If planning is interrupted before this line, the above calls (planQuery, registerDynamicFilteringQuery, planDistribution) will throw and this will not be reached.

The only case where it can happen is if there is a race condition where the planning completes just about the time the timeout kicks in and interrupts the thread right after planDistribution returns or while it's in some part that doesn't get affected by the interrupt so it doesn't throw. It definitely deserves a comment, and the debug statement is not very useful. Also see my comment above on how I'd restructure this section.

[sopel39]

If planning is interrupted before this line, the above calls (planQuery, registerDynamicFilteringQuery, planDistribution) will throw and this will not be reached.

That will only happen if these methods will throw interrupted exception on IO which might not be the case.

Also see my comment above on how I'd restructure this section.

+1

[martint]

That will only happen if these methods will throw interrupted exception on IO which might not be the case.

That should not happen unless the code can do something reasonable with a lack of result from that IO. If we're silently ignoring those interruptions and making results up, that's a bigger problem.

[martint]

It's not clear to me why we need that. The only reason would be to prevent the wrong thread from being interrupted, but that can't really happen. If the query transitions to FAILED during planning, it must still be executing the start() method, since all the planning methods are synchronous. It is only then that the thread is interrupted, which means it will be the right thread.

I can't think of a combination of actions that would result in start() completing and the query not already be in a state other than PLANNING, which means the interruption wouldn't happen beyond that point.

[martint]

Looks good, but let's change the default max planning time to something more reasonable (e.g., 10 minutes). 100 days is meaningless -- we might as well make it optional and default to no limit specified.

[sopel39]

mind automation

[skrzypo987]

There are some problems with GHA at the moment, I will restart it later