-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Only schedule FTE task if it received non-replicated splits #17790
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
Only schedule FTE task if it received non-replicated splits #17790
Conversation
...-main/src/main/java/io/trino/execution/scheduler/EventDrivenFaultTolerantQueryScheduler.java
Outdated
Show resolved
Hide resolved
795423d
to
6c954bd
Compare
8102efe
to
5251d10
Compare
...-main/src/main/java/io/trino/execution/scheduler/EventDrivenFaultTolerantQueryScheduler.java
Outdated
Show resolved
Hide resolved
5251d10
to
7621870
Compare
16ed103
to
e7db557
Compare
e7db557
to
8b3fdfc
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.
This is not a great name; at this point task is not scheduled, just merly put in the scheduling queue for scheduling late. Let's keep for now as it at least is in-line with PrioritizedScheduledTask
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.
Yeah I was using taskCreated
, but @arhimondr suggested to change to taskScheduled
:)
...-main/src/main/java/io/trino/execution/scheduler/EventDrivenFaultTolerantQueryScheduler.java
Outdated
Show resolved
Hide resolved
...-main/src/main/java/io/trino/execution/scheduler/EventDrivenFaultTolerantQueryScheduler.java
Outdated
Show resolved
Hide resolved
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 % comments.
I am not a big fan of readyForScheduling
field name. It can flip flop true/false/true/false - and name suggest that when it becomes false the partition is no longer ready for scheduling - which is not true.
No good idea for rename though :P
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 % comments.
I am not a big fan of readyForScheduling
field name. It can flip flop true/false/true/false - and name suggest that when it becomes false the partition is no longer ready for scheduling - which is not true.
No good idea for rename though :P
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 % comments.
I am not a big fan of readyForScheduling
field name. It can flip flop true/false/true/false - and name suggest that when it becomes false the partition is no longer ready for scheduling - which is not true.
No good idea for rename though :P
8b3fdfc
to
4a518f0
Compare
Description
Now we create all tasks upfront and we schedule them immediately. This PR changes to only schedule a task when it has actually received non-replicated splits
Additional context and related issues
#17596
Release notes
(x) 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.
( ) Release notes are required, with the following suggested text: