Add connector SPI for scale writers options by gaurav8297 · Pull Request #18561 · trinodb/trino

gaurav8297 · 2023-08-07T00:40:06Z

Description

Additional context and related issues

Release notes

( ) 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:

# Section
* Fix some things. ({issue}`issuenumber`)

core/trino-main/src/main/java/io/trino/metadata/Metadata.java

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

hashhar · 2023-08-08T06:57:26Z

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

taskScalingMaxWriterCount - is this per task or per table writer node?

It is per task

We also have another method getMaxWriterTasks in the SPI which helps to control the maximum number of tasks. How about if we put it in the WriterScalingOptions itself? cc @raunaqmorarka

I'm not clear on why getMaxWriterTasks was introduced. If it was to limit the maximum amount of write parallelism to an external data source, then the presence of per-task scaling already breaks that.
Per task writer scaling should probably be disabled when getMaxWriterTasks returns something non-empty, as it appears that jdbc connectors call this "write_parallelism".
We should probably deal with that separately to avoid getting blocked on that.

getMaxWriterTasks was introduced to limit number of writing tasks. From what I know jdbc connectors need that and use that - I know it from talks with different people - I did not confirm that so I am not sure.
It does not interfere with parallelism within task because it limit statically number of tasks.

If getMaxWriterTasks is not empty we could disable "local" scaling or "respect" this limit in local scaling.
@gaurav8297 , is it hard to disable / respect this number?

then the presence of per-task scaling already breaks that.

Yes, but after this PR we won't have writer scaling enabled for JDBC connectors.

As @radek-starburst mentioned, getMaxWriterTasks is there to limit the number of writing tasks. It doesn't have any control over what happens within a task. If a connector wants to control parallelism with scaling, then they ideally should set the value of both maxWriterTasks and perTaskMaxWriterCount.

Or, maybe instead of having two properties, we can have just one property maxWriters which controls the total number of writers in the cluster. We can look into this in future.

Also, I think this property works even when you've scaling disabled which is a case where we assign writing tasks to all the available workers.

I can see that getMaxWriterTasks limits number of writing tasks. The question is what underlying problem were we trying to solve by doing that ? If this was about limiting parallelism, then we need to change getMaxWriterTasks to getMaxWriterParallelism and implementing that should disable within task writer scaling. We can do it as a follow-up since landing this PR will side step the problem for now, but we should fix the API.

We can do it as a follow-up since landing this PR will side step the problem for now, but we should fix the API.

Yes, we should to it.

findepi · 2023-08-08T10:51:05Z

Can you please move Remove supportsReportingWrittenBytes from SPI to separate PR?

raunaqmorarka

lgtm
Needs eyes from other reviewers as well

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

raunaqmorarka · 2023-08-09T05:15:37Z

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

I'm not clear on why getMaxWriterTasks was introduced. If it was to limit the maximum amount of write parallelism to an external data source, then the presence of per-task scaling already breaks that.
Per task writer scaling should probably be disabled when getMaxWriterTasks returns something non-empty, as it appears that jdbc connectors call this "write_parallelism".
We should probably deal with that separately to avoid getting blocked on that.

findepi · 2023-08-09T10:31:48Z

the CI seems red

sopel39

lgtm % comments. I agree with @raunaqmorarka on 2ac76ee#r1287956305

testing/trino-testing/src/main/java/io/trino/testing/TestingConnectorBehavior.java

core/trino-main/src/main/java/io/trino/metadata/MetadataManager.java

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

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

sopel39 · 2023-08-09T10:57:06Z

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

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

findepi · 2023-08-09T11:37:46Z

Can you please move Remove supportsReportingWrittenBytes from SPI to separate PR?

thanks for thumbs up. feel free to link the PR here once it's created.

sopel39

mind test failures

core/trino-main/src/main/java/io/trino/metadata/MetadataManager.java

core/trino-main/src/main/java/io/trino/sql/planner/sanity/ValidateScaledWritersUsage.java

sopel39 · 2023-08-10T10:08:53Z

core/trino-main/src/main/java/io/trino/sql/planner/sanity/ValidateScaledWritersUsage.java

nit: that has quadratic cost (copying lists over and over)

sopel39 · 2023-08-10T10:09:39Z

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

@gaurav8297 @raunaqmorarka do we want to keep perTaskMaxScaledWriterCount as it seems redundant per previous discussion?

#18561 (comment)

I think we should keep it for now since it helps to limit parallelism. In future, we can look into combining both maxWriterTasks and this property into a single one.

Using WriterScalingOptions connector can control scaling by providing the following configurations. 1. isWriterTasksScalingEnabled 2. isPerTaskWriterScalingEnabled 3. perTaskMaxScaledWriterCount Additionally, for now scaling is only enabled for hive, iceberg and delta connector.

sopel39 · 2023-08-17T11:38:24Z

thx!

cla-bot bot added the cla-signed label Aug 7, 2023

github-actions bot added tests:hive iceberg Iceberg connector delta-lake Delta Lake connector hive Hive connector labels Aug 7, 2023

gaurav8297 marked this pull request as draft August 7, 2023 00:40

raunaqmorarka reviewed Aug 7, 2023

View reviewed changes

raunaqmorarka requested a review from sopel39 August 7, 2023 04:41

gaurav8297 force-pushed the scale_writer_flag branch from 7fee7ec to 3b4bbc8 Compare August 8, 2023 06:41

gaurav8297 marked this pull request as ready for review August 8, 2023 06:41

raunaqmorarka requested review from electrum, findepi, hashhar and wendigo August 8, 2023 06:44

gaurav8297 force-pushed the scale_writer_flag branch from 3b4bbc8 to 85793a9 Compare August 8, 2023 06:51

gaurav8297 requested a review from raunaqmorarka August 8, 2023 06:52

gaurav8297 force-pushed the scale_writer_flag branch from 85793a9 to 2ac76ee Compare August 8, 2023 06:56

hashhar reviewed Aug 8, 2023

View reviewed changes

findepi mentioned this pull request Aug 8, 2023

Improve scaling speed and some cleanup #18005

Merged

raunaqmorarka reviewed Aug 9, 2023

View reviewed changes

raunaqmorarka requested a review from radek-kondziolka August 9, 2023 05:40

sopel39 reviewed Aug 9, 2023

View reviewed changes

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java Outdated Show resolved Hide resolved

gaurav8297 mentioned this pull request Aug 10, 2023

Remove supportsReportingWrittenBytes from SPI #18617

Merged

gaurav8297 force-pushed the scale_writer_flag branch 2 times, most recently from 6164754 to 32525ce Compare August 10, 2023 02:56

gaurav8297 requested a review from raunaqmorarka August 10, 2023 03:18

gaurav8297 requested a review from sopel39 August 10, 2023 03:18

gaurav8297 force-pushed the scale_writer_flag branch from 32525ce to 95fb70b Compare August 10, 2023 03:24

sopel39 approved these changes Aug 10, 2023

View reviewed changes

gaurav8297 force-pushed the scale_writer_flag branch from 95fb70b to fa5eb8e Compare August 13, 2023 22:47

raunaqmorarka approved these changes Aug 16, 2023

View reviewed changes

sopel39 merged commit 996171c into trinodb:master Aug 17, 2023

sopel39 mentioned this pull request Aug 17, 2023

Release notes for 424 #18638

Closed

github-actions bot added this to the 424 milestone Aug 17, 2023

colebow mentioned this pull request Aug 17, 2023

Add Trino 424 release notes #18704

Merged

Conversation

gaurav8297 commented Aug 7, 2023

Description

Additional context and related issues

Release notes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

radek-kondziolka Aug 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaurav8297 Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

findepi commented Aug 8, 2023

Uh oh!

raunaqmorarka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findepi commented Aug 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sopel39 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

findepi commented Aug 9, 2023

Uh oh!

sopel39 left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sopel39 commented Aug 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

radek-kondziolka Aug 9, 2023 •

edited

Loading

gaurav8297 Aug 10, 2023 •

edited

Loading

findepi commented Aug 9, 2023 •

edited

Loading