-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow some tasks waiting for node per stage before blocking scheduling #11740
Conversation
b84e9f8
to
a165d03
Compare
a165d03
to
48659f5
Compare
core/trino-main/src/main/java/io/trino/execution/scheduler/FaultTolerantStageScheduler.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/execution/scheduler/FaultTolerantStageScheduler.java
Outdated
Show resolved
Hide resolved
48659f5
to
18741f2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % comment
@@ -153,6 +153,7 @@ | |||
public static final String QUERY_RETRY_ATTEMPTS = "query_retry_attempts"; | |||
public static final String TASK_RETRY_ATTEMPTS_OVERALL = "task_retry_attempts_overall"; | |||
public static final String TASK_RETRY_ATTEMPTS_PER_TASK = "task_retry_attempts_per_task"; | |||
public static final String MAX_TASKS_WAITING_FOR_NODE_PER_STAGE = "max_tasks_waiting_for_node_per_stage"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe prefix it with FAULT_TOLERANT_EXECUTIO_
(similar to other Tardigrade related configs)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it gets very long then :) Also session properties above are not prefixed - and also tardigrade specific .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. It looks like we are somehow inconsistent. Some of the properties are prefixed when some of them are not. I wonder what should be the guideline. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If property is only applicable to fault-tolerant execution I am option for prefixing. THough it would be nice to have something shorter than fault_tolerant_execution_
:) No idea for alternative though.
On the other hand at some point we may migrate to just having a single execution mode - then prefixing is not needed. Then dropping prefix already is more future proof :)
CI: #11876 |
18741f2
to
ab53e91
Compare
ab53e91
to
9bbf603
Compare
CI: #11893 |
Description
Allow some tasks waiting for node per stage before blocking scheduling
improvement
core query engine
Related issues, pull requests, and links
Fixes #10808
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
(x) No release notes entries required.
( ) Release notes entries required with the following suggested text: