Skip to content
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.

Sign up for GitHub

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

Do not compare finished and all tasks sets eagerly #14635

Merged
merged 1 commit into from
Oct 17, 2022
Merged

Do not compare finished and all tasks sets eagerly #14635

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion core/trino-main/src/main/java/io/trino/execution/scheduler/PipelinedStageExecution.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
import java.util.stream.Stream;

import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static io.airlift.http.client.HttpUriBuilder.uriBuilderFrom;
Expand Down Expand Up @@ -395,7 +396,11 @@ private synchronized boolean isStageFlushing()

private synchronized boolean isStageFinished()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do similar optimization in isStageFlushing ?

Copy link
Member

@Dith3r Dith3r Oct 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isStageFlushing we check two different sets. There is a high chance that flushing size and finished size will be higher than allTask size (first we add to finished, and then we remove from flushing).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, if we can have a task id in both sets at some point in time, then we can't make similar optimization there.

{
return finishedTasks.containsAll(allTasks);
boolean finished = finishedTasks.size() == allTasks.size();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are 100% sure that finishedTasks will not contain any additional tasks which are not included in allTasks this is ok, but I would think of >= comparison here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are 100% sure that finishedTasks will not contain any additional tasks which are not included in allTasks

That would be a bug

if (finished) {
checkState(finishedTasks.containsAll(allTasks), "Finished tasks should contain all tasks");
}
return finished;
}

private boolean addFlushingTask(TaskId taskId)
Expand Down