Remove grouped execution #12916

skrzypo987 · 2022-06-21T07:24:05Z

Description

Removal of grouped execution as it can be easily replaced by Tardigrade.
The PR contains many small commits to help with the review process. Majority should be squashed/rearranged before the merge.
Most of the changes is just a mindless unused code removal. Some parts, however, required more robust refactorings. Since I am not an expert in many of the areas, some things has been done without full understanding.

Is this change a fix, improvement, new feature, refactoring, or other?

all of the above

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

all

How would you describe this change to a non-technical end user or system administrator?

Removal of grouped execution

Related issues, pull requests, and links

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
(x) Release notes entries required with the following suggested text:

# Section
* Removed grouped execution mechanism. `grouped-execution-enabled`, `dynamic-schedule-for-grouped-execution`, `concurrent-lifespans-per-task` config options and `grouped_execution`, `dynamic_schedule_for_grouped_execution` and `concurrent_lifespans_per_task` system properties are no longer available

skrzypo987 · 2022-06-21T08:24:19Z

The testSingleVersionBackwardCompatibility is failing. I don't really know how to handle that.
@kokosing You made this test, any thoughts?

kokosing · 2022-06-21T09:58:21Z

core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorSplitManager.java

Can you remove this deprecated method now?

My changes had nothing to do with that. The method is still used

kokosing · 2022-06-21T09:59:31Z

core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorSplitManager.java

To avoid issues with backward compatibility you would need to make things you remove deprecated first and add non deprecated methods.

kokosing · 2022-06-21T10:00:12Z

The testSingleVersionBackwardCompatibility is failing. I don't really know how to handle that.

The best way is to run the test in the IDE. The diff is presented much better there than in CI.

kokosing · 2022-06-21T10:01:37Z

2022-06-21T07:31:37.7682499Z but could not find the following element(s):
2022-06-21T07:31:37.7682783Z  <["Class: public static enum io.trino.spi.connector.ConnectorSplitManager$SplitSchedulingStrategy",
2022-06-21T07:31:37.7683229Z     "Method: public static io.trino.spi.connector.ConnectorSplitManager$SplitSchedulingStrategy[] io.trino.spi.connector.ConnectorSplitManager$SplitSchedulingStrategy.values()",
2022-06-21T07:31:37.7683703Z     "Method: public static io.trino.spi.connector.ConnectorSplitManager$SplitSchedulingStrategy io.trino.spi.connector.ConnectorSplitManager$SplitSchedulingStrategy.valueOf(java.lang.String)",
2022-06-21T07:31:37.7684233Z     "Field: public static io.trino.spi.connector.ConnectorSplitManager$SplitSchedulingStrategy io.trino.spi.connector.ConnectorSplitManager$SplitSchedulingStrategy.UNGROUPED_SCHEDULING",
2022-06-21T07:31:37.7684881Z     "Field: public static io.trino.spi.connector.ConnectorSplitManager$SplitSchedulingStrategy io.trino.spi.connector.ConnectorSplitManager$SplitSchedulingStrategy.GROUPED_SCHEDULING",
2022-06-21T07:31:37.7686488Z     "Method: public default io.trino.spi.connector.ConnectorSplitSource io.trino.spi.connector.ConnectorSplitManager.getSplits(io.trino.spi.connector.ConnectorTransactionHandle,io.trino.spi.connector.ConnectorSession,io.trino.spi.connector.ConnectorTableHandle,io.trino.spi.connector.ConnectorSplitManager$SplitSchedulingStrategy,io.trino.spi.connector.DynamicFilter,io.trino.spi.connector.Constraint)"]>

I think that is the exact list of things that you removed.

skrzypo987 · 2022-06-21T12:39:44Z

@kokosing I rolled back the deletion of the class and put deprecated annotations around (last commit). Is that the way to do it?

kokosing · 2022-06-21T14:46:07Z

I rolled back the deletion of the class and put deprecated annotations around (last commit). Is that the way to do it?

Almost.

Since deprecated method cannot be annotated with deprecated annotation any more I guess you should avoid changing it signature too. I think that next step should be to just remove it.
Your new deprecated method is actually not used so if a plugin is using that method still instead of new one it won't work. So your change is still backward incompatible.

losipiuk · 2022-06-21T15:55:04Z

plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseHiveConnectorTest.java

would that make sense to leave test assertion for notColocated? Or is that covered by other tests. I would assume the latter, but can you check please?

Maybe. I did not found tests that strictly mimic this one. The question remains about the name of the test since it has nothing to do with grouped execution anymore

losipiuk · 2022-06-21T16:02:29Z

plugin/trino-kudu/src/main/java/io/trino/plugin/kudu/KuduClientConfig.java

It looks to me that this property was misnamed and should have been called exposeTableRangePartitioning or sth. Not sure if that is applicable only for grouped exectution. Any Kudu experts on board? @MartinWeindel @grantatspothero ?

I'm not very familiar with ConnectorTableProperties, are tablePartitioning and streamPartitioningColumns used for anything besides grouped execution?

Right now these two fields are only populated behind the kudu.grouped-execution.enabled flag.

losipiuk · 2022-06-21T16:23:28Z

core/trino-main/src/test/java/io/trino/execution/TestSqlTaskExecution.java

does not look like specific to just GROUPED_EXECUTION

I left the testSimple method and removed the grouped case.

core/trino-main/src/main/java/io/trino/execution/SqlTaskExecution.java

core/trino-main/src/main/java/io/trino/operator/join/HashBuilderOperator.java

losipiuk · 2022-06-21T17:28:58Z

core/trino-main/src/main/java/io/trino/sql/planner/NodePartitioningManager.java

Can you do that? There are calls with preferDynamic set to true and false

I am afraid you are right. I dropped the last commit.

losipiuk

Looks OK.
@dain would you like to skim?

skrzypo987 · 2022-06-23T07:42:20Z

Addressed comments in existing commits. The Kudu question remains

skrzypo987 · 2022-06-24T11:49:09Z

Rebased and added another commit that removes another stuff

skrzypo987 · 2022-06-27T12:52:07Z

Another rebase and this time added a commit requested by @sopel39. Also addressed small comments received offline.
If tests are ok we should start squashing commits to a more logical ones and it will be good to merge.

This commit removes the feature of grouped execution. The code associated with this feature is, however, mostly still present in the codebase. The commit only removes config options and system session properties. The only additional changes are for the purpose of successful compilation. The dead code that is left in the project will be cleared in the following commits

The class is not useful after removal of grouped execution. Major parts of PlanFragmenter class has been removed in the process, as well as some tests strictly related to grouped execution

The class is not useful after removal of grouped execution.

After the removal of grouped execution it is impossible for this object to be anything else than the "taskWide" singleton. This object is at that point completely useless and will exist just until its last occurrence is removed, which will take place in the following commits.

The lifespan object is a useless singleton at this point so it is being removed from: -DriverContext -DriverFactory -DriverStats -Join related classes -Split and SplitAssignment -LocalExchange. LocalExchangeFactory class is not needed anymore. -SqlTaskExecution

It was always a `NOT_PARTITION` since the removal of grouped execution, so it can be hardcoded at this point.

The class is not actually used since the removal of grouped execution. The last usages removed in this commit are: -TaskStatus class -TaskContext class -operator factories -schedulers

It was always a `NOT_PARTITION` since the removal of grouped execution, so it can be hardcoded at this point. The ConnectorNodePartitioningProvider::listPartitionHandles method can also be deprecated at this point

cla-bot bot added the cla-signed label Jun 21, 2022

github-actions bot added docs tests:hive labels Jun 21, 2022

skrzypo987 marked this pull request as ready for review June 21, 2022 07:24

skrzypo987 requested review from losipiuk and sopel39 June 21, 2022 07:24

kokosing reviewed Jun 21, 2022

View reviewed changes

skrzypo987 force-pushed the skrzypo/073-remove-grouped-execution branch 3 times, most recently from a82dd9b to e47429f Compare June 21, 2022 12:38

losipiuk reviewed Jun 21, 2022

View reviewed changes

core/trino-main/src/main/java/io/trino/execution/SqlTaskExecution.java Outdated Show resolved Hide resolved

losipiuk reviewed Jun 21, 2022

View reviewed changes

core/trino-main/src/main/java/io/trino/operator/join/HashBuilderOperator.java Outdated Show resolved Hide resolved

losipiuk reviewed Jun 21, 2022

View reviewed changes

skrzypo987 force-pushed the skrzypo/073-remove-grouped-execution branch from c9932c3 to 86e7745 Compare June 23, 2022 07:41

skrzypo987 force-pushed the skrzypo/073-remove-grouped-execution branch from 86e7745 to 47399a5 Compare June 24, 2022 11:48

losipiuk approved these changes Jun 27, 2022

View reviewed changes

skrzypo987 force-pushed the skrzypo/073-remove-grouped-execution branch from 47399a5 to c52416d Compare June 27, 2022 12:50

skrzypo987 force-pushed the skrzypo/073-remove-grouped-execution branch from c52416d to 56439b4 Compare June 27, 2022 13:11

skrzypo987 added 10 commits June 28, 2022 10:11

Remove grouped execution config options from Kudu

249ec5f

Remove StageExecutionDescriptor class

ceef49d

The class is not useful after removal of grouped execution. Major parts of PlanFragmenter class has been removed in the process, as well as some tests strictly related to grouped execution

Remove PipelineExecutionStrategy class

8eb9b54

The class is not useful after removal of grouped execution.

Deprecate SplitSchedulingStrategy class

5a316a2

The class is not useful after removal of grouped execution.

Remove partitionHandle argument from SplitSource::getNextBatch method

7e61354

It was always a `NOT_PARTITION` since the removal of grouped execution, so it can be hardcoded at this point.

Remove Lifespan class

aa583a1

The class is not actually used since the removal of grouped execution. The last usages removed in this commit are: -TaskStatus class -TaskContext class -operator factories -schedulers

Deprecate partitionHandle argument on ConnectorSplitSource::getNextBatch

a5c5c21

It was always a `NOT_PARTITION` since the removal of grouped execution, so it can be hardcoded at this point. The ConnectorNodePartitioningProvider::listPartitionHandles method can also be deprecated at this point

skrzypo987 force-pushed the skrzypo/073-remove-grouped-execution branch from 56439b4 to a5c5c21 Compare June 28, 2022 08:42

sopel39 approved these changes Jun 28, 2022

View reviewed changes

losipiuk merged commit 9bb5231 into trinodb:master Jun 28, 2022

sopel39 mentioned this pull request Jun 28, 2022

Release notes for 388 #12945

Closed

github-actions bot added this to the 388 milestone Jun 28, 2022

colebow mentioned this pull request Jun 28, 2022

Add Trino 388 release notes #13004

Merged

ans76 mentioned this pull request Aug 12, 2022

Union query may blocked after remove grouped execution #13652

Closed

raunaqmorarka mentioned this pull request Sep 24, 2022

Deprecate and remove grouped execution #10698

Closed

Remove grouped execution #12916

Remove grouped execution #12916

Uh oh!

Conversation

skrzypo987 commented Jun 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues, pull requests, and links

Documentation

Release notes

Uh oh!

skrzypo987 commented Jun 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kokosing commented Jun 21, 2022

Uh oh!

kokosing commented Jun 21, 2022

Uh oh!

skrzypo987 commented Jun 21, 2022

Uh oh!

kokosing commented Jun 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

losipiuk left a comment

Choose a reason for hiding this comment

Uh oh!

skrzypo987 commented Jun 23, 2022

Uh oh!

skrzypo987 commented Jun 24, 2022

Uh oh!

skrzypo987 commented Jun 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

skrzypo987 commented Jun 21, 2022 •

edited

Loading