Add progress bar support for fault tolerant execution

[linzebing]

Description

Unlike pipelined execution, fault tolerant execution doesn't execute stages all at once. This means in the middle of execution, some stages will be in PLANNED state, which is seen as not scheduled. Therefore, no progress will be displayed until the very end for fault tolerant execution.

This PR adds progress bar support for fault tolerant execution.

Additional context and related issues

Fix #13072

Release notes

( ) 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.
(x) Release notes are required, with the following suggested text:

# Section
* Add progress bar support for fault tolerant execution. ({issue}`13072`)

[losipiuk]

This feels like a bit to much for this single usecase. Maybe instead let's have a flag if query is using FTE and a counter how many stages are already scheduled?

[linzebing]

I have thought about that. However, that may cause confusion to the end users as we won't be able to know whether we are using FTE here

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

Lines 282 to 287 in 49e4c06

	StatementStats.builder()
	.setState(state.toString())
	.setQueued(state == QUEUED)
	.setElapsedTimeMillis(elapsedTime.toMillis())
	.setQueuedTimeMillis(queuedTime.toMillis())
	.build(),

[losipiuk]

I do not think it is a big deal that we always return false here.
We can also make value OptionalBoolean.
Or we can have a String retryMode field where we can put
NONE, QUERY, TASK and UNKNOWN. We can then use UNKNOWN in this context

[losipiuk]

The formulas looks like. Let's settle on how we mark query as FTE in stats.

[arhimondr]

LGTM % nits

[arhimondr]

nit: maybe extract the implementation into a helper method (you can pass a boolean flag on what to compute)

[arhimondr]

nit: maybe have stats#getRunningPercentage that has a condition based on what execution mode is used?

[linzebing]

Not really feasible if we don't want to change the signature of formatProgressBar:

if (stats.isFaultTolerantExecution()) {
     progressBar = formatProgressBar(progressWidth, progressPercentage, stats.getFaultTolerantExecutionRunningPercentage(), 100);
 }
else {
    progressBar = formatProgressBar(progressWidth,
        stats.getCompletedSplits(),
        max(0, stats.getRunningSplits()),
        stats.getTotalSplits());
}

[losipiuk]

does this one guarantee that progress will not decrease? totalSplits can grow for stage, so I do not believe it is guaranteed. Am I missing something, or we just not care?

[linzebing]

Oh I didn't know totalSplits can grow per stage. But still, seems this is the best thing we can do now.

[losipiuk]

One question

[losipiuk]

I tested it out on tcph.sf10 with:

SELECT
  c.custkey,
  c.name,
  sum(l.extendedprice * (1 - l.discount)) AS revenue,
  c.acctbal,
  n.name,
  c.address,
  c.phone,
  c.comment
FROM
  "lineitem" AS l,
  "orders" AS o,
  "customer" AS c,
  "nation" AS n
WHERE
  c.custkey = o.custkey
  AND l.orderkey = o.orderkey
  AND o.orderdate >= DATE '1993-10-01'
  AND o.orderdate < DATE '1993-10-01' + INTERVAL '3' MONTH
  AND l.returnflag = 'R'
  AND c.nationkey = n.nationkey
GROUP BY
  c.custkey,
  c.name,
  c.acctbal,
  c.phone,
  n.name,
  c.address,
  c.comment
ORDER BY
  revenue DESC
LIMIT 20
;

For quite a few iterations the running splits are reported as 0 which make progress bar not hav '>' at the end. It is kinda accurate but does not look pretty.
I think the question is why we have 0 running splits for prolonged period of time and still get progress. @arhimondr ideas?

[arhimondr]

I think the question is why we have 0 running splits for prolonged period of time and still get progress.

Interesting. I don't really know how is it possible to have 0 running splits for prolonged period. Did we check what do running tasks report?

[electrum]

We should take this opportunity to move the progress percentage logic into the server. Rather than including a faultTolerantExecution flag, we simply add a progressPercentage property to the StatementStats constructor.

Many years ago, we added the getProgressPercentage() method with @JsonProperty, so the server returns it and this is used by the web UI and maybe other clients as well. We should complete this transition by moving the computation into the constructor, so that StatementStats directly accepts the computed value.

This will mean that any clients using the property will automatically get the FTE behavior.

[electrum]

Looking at this closer, seems like we need to add a runningPercentage stat as well.

Does the scheduled flag make sense for FTE? The CLI currently does

if (stats.isScheduled()) {

but we could change that to

if (stats.getProgressPercentage().isPresent()) {

since that already includes the scheduled flag.

[linzebing]

@electrum : we can't really. The logic for formatting a progress bar is different for streaming mode and FTE mode (to prevent progress bar from going back and forth): FTE mode uses runningPercentage but streaming mode doesn't. This means we anyways need to include the faultTolerantExecution flag.

[electrum]

What if we add runningPercentage for both modes and use it if present, otherwise falling back to the old mode (for new clients talking to older servers)?

The goal here is to keep as much of the logic on the server as possible. We don't want every client to have to understand all of these details. Does this make sense?

[linzebing]

OK, I will address this some time later

[arhimondr]

LGTM % There's a chance something may break we don't know of once we remove the scheduled field. Unless there's a good reason why to remove it I would recommend keeping it.