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.

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

Improve sheduling of min drivers per task #10249

Merged

Conversation

sopel39
Copy link
Member

@sopel39 sopel39 commented Dec 9, 2021

Start more than single split in TaskExecutor#scheduleTaskIfNecessary.
Previously only single split was started. This caused
that less than task.min-drivers-per-task splits were
running for situations when buffer was underutlized
and no more splits were scheduled by coordinator.

@sopel39
Copy link
Member Author

sopel39 commented Dec 9, 2021

cc @dain

Copy link
Member

@losipiuk losipiuk left a comment

Choose a reason for hiding this comment

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

Code LGTM (for what I can tell :) )

Maybe make it more explicit in commit message that it is about target concurrency which is changed while execution. Maybe sht like:

Start more than a single split in TaskExecutor#scheduleTaskIfNecessary.
Previously only a single split was started. This caused
that less than task.min-drivers-per-task splits were
running for situations when targetConcurrency was increased in SplitConcurrencyController.

You mention buffer. What buffery you have in mind?

@sopel39 sopel39 force-pushed the ks/improve_min_drivers_per_task branch from baff049 to e37eb1e Compare December 9, 2021 16:14
@sopel39
Copy link
Member Author

sopel39 commented Dec 9, 2021

You mention buffer. What buffery you have in mind?

It's task output buffer. SplitConcurrencyController adjust concurrency so that buffer is neither empty or full

Copy link
Contributor

@arhimondr arhimondr left a comment

Choose a reason for hiding this comment

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

The change looks good to me. The description is a little confusing though.

My understanding is that this method is called when a split is added or when a split is finished. I guess the assumption was that it will always schedule a split if one exist. With this assumption the implementation would behave correctly.

However due to the adaptive concurrency implementation details the pollNextSplit method may not always return a split even if one exist, so the assumption breaks. Thus it is required to schedule as many splits as needed in one shot.

core/trino-main/src/main/java/io/trino/util/MoreMath.java Outdated Show resolved Hide resolved
@sopel39
Copy link
Member Author

sopel39 commented Dec 9, 2021

My understanding is that this method is called when a split is added or when a split is finished. I guess the assumption was that it will always schedule a split if one exist. With this assumption the implementation would behave correctly.

If worker gets N splits, but adaptive task concurrency = N/2 only N/2 splits will start (when all remaining driver slots are already taken by other tasks). In that case in current code when no new splits are received, at most N/2 splits would run until task finishes even if adaptive task concurrency is increased. Basically, I've observed that there happens to be a long tail when query is almost finished and query creeps forward by few splits at a time

Start more than a single split in TaskExecutor#scheduleTaskIfNecessary.
Previously only a single split was started. This caused
that less than task.min-drivers-per-task splits were
running for situations when targetConcurrency was increased in SplitConcurrencyController.
@sopel39 sopel39 force-pushed the ks/improve_min_drivers_per_task branch from e37eb1e to ce4dc86 Compare December 9, 2021 19:42
@sopel39 sopel39 merged commit 9503bf4 into trinodb:master Dec 10, 2021
@sopel39 sopel39 deleted the ks/improve_min_drivers_per_task branch December 10, 2021 09:44
@sopel39 sopel39 mentioned this pull request Dec 10, 2021
10 tasks
@github-actions github-actions bot added this to the 366 milestone Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants