-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Do not use SkewedPartitionRebalancer for remote exchange in FTE #21979
base: master
Are you sure you want to change the base?
Conversation
CI (re-running) |
core/trino-main/src/main/java/io/trino/sql/planner/LocalExecutionPlanner.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/sql/planner/LocalExecutionPlanner.java
Outdated
Show resolved
Hide resolved
@@ -581,7 +581,8 @@ public LocalExecutionPlan plan( | |||
PartitionFunction partitionFunction; | |||
Optional<SkewedPartitionRebalancer> skewedPartitionRebalancer = Optional.empty(); | |||
int taskCount = getTaskCount(partitioningScheme); | |||
if (checkCanScalePartitionsRemotely(taskContext.getSession(), taskCount, partitioningScheme.getPartitioning().getHandle(), nodePartitioningManager)) { | |||
if (SystemSessionProperties.getRetryPolicy(taskContext.getSession()) != RetryPolicy.TASK |
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: what happens if I change policy from TASK
to none via Session property?
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.
So NONE is no retries - and the we want to use skewedPartitionRebalancer
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.
I would guess using none on fte cluster has perf implications
@@ -581,7 +581,8 @@ public LocalExecutionPlan plan( | |||
PartitionFunction partitionFunction; |
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.
Is it somewhat testable in query plans or similar? Is it worth testing? I would assume there was regression due to scaling rebalancer in FTE
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.
I will check how much regression we get realistically - not tested - yet this code is not needed in FTE - so CPU cycles are wasted for sure.
81b873a
to
f8a6adb
Compare
The rebalancing logic from SkewedPartitionRebalancer is already covered by partitions splitting/merging in HashDistributionSplitAssigner. Also the way remote SkewedPartitionRebalancer is configured does not match FTE behaviour as there is assumption that data will be consumed by N tasks where N is number of cluster nodes. This is not true for FTE.
f8a6adb
to
5453fe9
Compare
The rebalancing logic from SkewedPartitionRebalancer is already covered by partitions splitting/merging in HashDistributionSplitAssigner. Also the way remote SkewedPartitionRebalancer is configured does not match FTE behaviour as there is assumption that data will be consumed by N tasks where N is number of cluster nodes. This is not true for FTE.
Release notes
(x) This is not user-visible or is 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: