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

Jump to bottom

Adjust scheduler assignment queue for node #15168

Merged
merged 2 commits into from
Dec 21, 2022
Merged

Adjust scheduler assignment queue for node #15168

merged 2 commits into from
Dec 21, 2022

Conversation

Dith3r
Copy link
Member

@Dith3r Dith3r commented Nov 23, 2022 •
edited
Loading

Description

Aim to fix #15146.

PR is splitting max-pending-task into the separated configuration variables and adds adjustment during scheduling of splits to increase assignments based on queue clearance. Also adding the possibility to exit assignment split loop if all nodes are filled with assignments.

select count(*) from hive.sf1000.lineitem_parquet_64_group;

default: 19,44 s
patch:   8,93 s
2000:    8,58 s

select count(orderkey),count(suppkey) from hive.sf1000.lineitem_parquet_64_group;

default: 36,16 s
patch:   26,23 s
2000:    25,01 s

select count(orderkey),count(suppkey), count(linenumber), count(quantity), count(extendedprice), count(discount) from hive.sf1000.lineitem_parquet_64_group;
																							
default: 58,84 s
patch:   53,88 s
2000:    55,22 s

Additional context and related issues

Allow scheduler to adjust size of assignments based on how fast nodes are processing splits.

Release notes

( ) This is not user-visible or docs only and no release notes are required.
(x) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Nov 23, 2022
@Dith3r Dith3r force-pushed the master branch 3 times, most recently from ea013ce to 3b5d156 Compare November 24, 2022 11:42
@Dith3r Dith3r force-pushed the master branch 5 times, most recently from 5c2871a to e6411d1 Compare November 28, 2022 13:18
@Dith3r Dith3r marked this pull request as ready for review November 28, 2022 14:20
@Dith3r Dith3r force-pushed the master branch 3 times, most recently from 08260d9 to bcdcbdf Compare November 28, 2022 15:08
@Dith3r Dith3r requested a review from sopel39 November 28, 2022 16:04
sopel39
sopel39 reviewed Nov 29, 2022
View reviewed changes
core/trino-main/src/main/java/io/trino/execution/scheduler/NodeScheduler.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/scheduler/UniformNodeSelector.java Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/scheduler/NodeScheduler.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/scheduler/UniformNodeSelector.java Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/scheduler/UniformNodeSelector.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/scheduler/UniformNodeSelector.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/scheduler/UniformNodeSelector.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/scheduler/UniformNodeSelector.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/scheduler/UniformNodeSelector.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/scheduler/UniformNodeSelector.java Outdated Show resolved Hide resolved
@Dith3r Dith3r force-pushed the master branch 5 times, most recently from 23e7cfc to bd32624 Compare December 2, 2022 14:48
sopel39
sopel39 reviewed Dec 15, 2022
View reviewed changes
core/trino-main/src/main/java/io/trino/execution/scheduler/NodeSchedulerConfig.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/scheduler/UniformNodeSelector.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/scheduler/UniformNodeSelector.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/scheduler/UniformNodeSelector.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/scheduler/UniformNodeSelector.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/scheduler/UniformNodeSelector.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/scheduler/UniformNodeSelector.java Show resolved Hide resolved
docs/src/main/sphinx/admin/properties-node-scheduler.rst Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/scheduler/TopologyAwareNodeSelector.java Outdated Show resolved Hide resolved
docs/src/main/sphinx/admin/properties-node-scheduler.rst Outdated Show resolved Hide resolved
radek-kondziolka
radek-kondziolka reviewed Dec 15, 2022
View reviewed changes
Copy link
Contributor

@radek-kondziolka radek-kondziolka left a comment

Choose a reason for hiding this comment

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

Please run throughput benchmarks

core/trino-main/src/main/java/io/trino/execution/scheduler/UniformNodeSelector.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/scheduler/UniformNodeSelector.java Outdated Show resolved Hide resolved
sopel39
sopel39 reviewed Dec 15, 2022
View reviewed changes
Copy link
Member

@sopel39 sopel39 left a comment

Choose a reason for hiding this comment

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

comments, but I think the approach generally looks good

core/trino-main/src/main/java/io/trino/execution/scheduler/NodeSchedulerConfig.java Show resolved Hide resolved
docs/src/main/sphinx/admin/properties-node-scheduler.rst Outdated Show resolved Hide resolved
@Dith3r Dith3r requested a review from sopel39 December 15, 2022 17:38
sopel39
sopel39 reviewed Dec 19, 2022
View reviewed changes
core/trino-main/src/main/java/io/trino/execution/scheduler/NodeSchedulerConfig.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/scheduler/NodeSchedulerConfig.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/scheduler/UniformNodeSelector.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/scheduler/UniformNodeSelectorFactory.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/scheduler/UniformNodeSelector.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/scheduler/UniformNodeSelector.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/scheduler/UniformNodeSelector.java Outdated Show resolved Hide resolved
sopel39
sopel39 approved these changes Dec 19, 2022
View reviewed changes
Copy link
Member

@sopel39 sopel39 left a comment

Choose a reason for hiding this comment

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

lgtm % comments

core/trino-main/src/main/java/io/trino/execution/scheduler/UniformNodeSelector.java Outdated Show resolved Hide resolved
core/trino-main/src/test/java/io/trino/execution/TestNodeScheduler.java Show resolved Hide resolved
docs/src/main/sphinx/admin/properties-node-scheduler.rst Outdated Show resolved Hide resolved
core/trino-main/src/test/java/io/trino/execution/TestNodeScheduler.java Outdated Show resolved Hide resolved
core/trino-main/src/test/java/io/trino/execution/TestNodeScheduler.java Outdated Show resolved Hide resolved
@Dith3r
Exit split assignment loop if all nodes exhausted
4880911 
Splits statistics are taken at the begining of computeAssignments method, breaking a loop of assingmnet allows more efficient scheduling without testing if exhausted nodes can accept another split. Sending assignments to nodes is done after exit from this method.
@Dith3r Dith3r force-pushed the master branch 3 times, most recently from bc1940f to 40dc472 Compare December 20, 2022 12:44
sopel39
sopel39 approved these changes Dec 20, 2022
View reviewed changes
core/trino-main/src/test/java/io/trino/execution/scheduler/TestUniformNodeSelector.java Outdated Show resolved Hide resolved
core/trino-main/src/test/java/io/trino/execution/scheduler/TestUniformNodeSelector.java Outdated Show resolved Hide resolved
core/trino-main/src/test/java/io/trino/execution/scheduler/TestUniformNodeSelector.java Outdated Show resolved Hide resolved
core/trino-main/src/test/java/io/trino/execution/scheduler/TestUniformNodeSelector.java Show resolved Hide resolved
core/trino-main/src/test/java/io/trino/execution/scheduler/TestUniformNodeSelector.java Show resolved Hide resolved
core/trino-main/src/test/java/io/trino/execution/scheduler/TestUniformNodeSelector.java Outdated Show resolved Hide resolved
docs/src/main/sphinx/admin/properties-node-scheduler.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/admin/properties-node-scheduler.rst Outdated Show resolved Hide resolved
@Dith3r
Adjust scheduler assignment queue for node
328958f 
Add splits assignment adjustment based on node statistics. If the node is able to process all splits in the queue,
the amount of max pending splits to be assigned is doubled in the next iteration of the computeAssignments method.
Scaling down is done after interval from previous scaling up.
Queue adjustment is limited to be in range of newly created `min-pending-splits-per-task` and changed `max-pending-splits-per-task` to `max-adjusted-pending-splits-per-task`.
sopel39
sopel39 approved these changes Dec 21, 2022
View reviewed changes
core/trino-main/src/test/java/io/trino/execution/scheduler/TestUniformNodeSelector.java

// contents of taskMap indicate the node-task map for the current stage
nodeScheduler = new NodeScheduler(new UniformNodeSelectorFactory(nodeManager, nodeSchedulerConfig, nodeTaskMap));
// contents of taskMap indicate the node-task map for the current stage
Copy link
Member

Choose a reason for hiding this comment

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

nit: duplicate comment

@sopel39 sopel39 merged commit ac18d5b into trinodb:master Dec 21, 2022
@sopel39 sopel39 mentioned this pull request Dec 21, 2022
Closed
@github-actions github-actions bot added this to the 404 milestone Dec 21, 2022
@colebow colebow mentioned this pull request Dec 21, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@radek-kondziolka radek-kondziolka radek-kondziolka left review comments

@sopel39 sopel39 sopel39 approved these changes

@lukasz-stec lukasz-stec Awaiting requested review from lukasz-stec

Assignees
No one assigned
Labels
cla-signed docs
Milestone
405
Development

Successfully merging this pull request may close these issues.

Inefficient scheduling simple and quick to process splits for workers
4 participants
@Dith3r @sopel39 @lukasz-stec @radek-kondziolka