Enable Trino to detect and fail queries that have tasks stuck in long running JONI parsing.

[groupcache4321]

Description

Enable Trino to detect and fail tasks that are stuck in long running JONI parsing.

Is this change a fix, improvement, new feature, refactoring, or other?

Fix

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Query Engine

How would you describe this change to a non-technical end user or system administrator?

Trino is a time shared multi-tenant system. For context switching, trino relies on the threadpool workers cooperatively yield itself to the scheduling logic. It is not like an operating system which used a hard interrupt signal to force the process to do the context switch.

Most of the split processing can be finished in a relative short interval, whereas the JONI processing is an exception.

Furthermore, some of the trino features: gathering statistics, relaying on the trino to do callback function(to execute splitFinished() after yield) after the context switch, since there is no context switch in this case, trino can't gather accurate CPU usage for that split.

This PR allows trino to detect and fail tasks that are stuck in long running JONI parsing.

Documentation

( ) No documentation is needed.
(x) 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

( ) No release notes entries required.
(x) Release notes entries required with the following suggested text:

Enable Trino to detect and fail queries that have tasks stuck in long running JONI parsing.

[cla-bot]

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: yluan.
This is most likely caused by a git client misconfiguration; please make sure to:

1. check if your git client is configured with an email to sign commits git config --list | grep email
2. If not, set it up using git config --global user.email email@example.com
3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

[cla-bot]

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: yluan.
This is most likely caused by a git client misconfiguration; please make sure to:

1. check if your git client is configured with an email to sign commits git config --list | grep email
2. If not, set it up using git config --global user.email email@example.com
3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

[losipiuk]

A few editorials, but major concern is there is no synchronization between code which interrupts Split runner threads and the threads themselves. It very much looks like we may interrupt thread which already moved to another split.

[phd3]

We also have a warning about the runaway splits, currently hardcoded. IMO we should validate the two thresholds are coherent (i.e. warning <= timeout), which would require making the warning configurable too

[groupcache4321]

got it, will configure: "LONG_SPLIT_WARNING_THRESHOLD"

[groupcache4321]

agree we should make warning <= timeout, because if the thread runtime for the split > than timeout, the system had already interrupted the split.

[groupcache4321]

I also felt "/v1/maxActiveSplits" is unnecessary.. Since this PR will print out the run away splits in the log..

[phd3]

is 1s based off of SPLIT_RUN_QUANTA? if so let's just use that

[groupcache4321]

ok.

[groupcache4321]

Hey, I found out the issue is very similar to #7213, in which we want to limit the total time for SQLQueryExecution. In that case, the thread is reused between queries, in our case, the thread is reused between tasks. In either case, I abstract the following programming pattern:

1. When assigning task to the thread:

AtomicReference<Thread> sharedThread = new AtomicReference<>(currentThread())

2. After 1, when processing:

try {
	process using the shared thread for the assigned task
} finally {
	synchronized (exectuorObject) {
		sharedThread = null;
		Thread.interrupted();
	}
}

3. When interrupting:

{
	if can interrupt();
	synchronized (exectuorObject) {
// double check once get into the critical section, 
// to avoid race condition happening at the short interval between the check and get into the race condition. 
                if can interrupt();  
		thread = sharedThread.get();
		if (thread != null) {
			thread.interrupt();
		}
	}
}

From my personally understanding, the AtomicReference can also be replaced by volatile keyword.

[cla-bot]

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: yluan.
This is most likely caused by a git client misconfiguration; please make sure to:

1. check if your git client is configured with an email to sign commits git config --list | grep email
2. If not, set it up using git config --global user.email email@example.com
3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

[jhlodin]

Docs LGTM, just need to word wrap at 80 characters

[phd3]

lgtm, mostly nits

[arhimondr]

Generally looks good to me % nits.

Please update the commit message according to the guideline: https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#format-git-commit-messages

[arhimondr]

nit: Store the task id and split info directly.

[arhimondr]

Please respond (let me know if you missed it or not going to address)

[arhimondr]

Discussed offline. The split info cannot be cached as it changes as the split execution progresses.

[groupcache4321]

Hi, I have addressed all the comments.

I also changed the minimum value for the configs. Due to the async nature, the check based on the walltime is inaccurate when the timeout is at a similar scale to the time quota. It can only detect splits that significantly run longer than the split's 1 second time quota

[arhimondr]

LGTM % comments

[arhimondr]

Please respond (let me know if you missed it or not going to address)

[phd3]

Merged, thanks!