Skip to content

Improve exchange interfaces to support speculative execution#14448

Merged
arhimondr merged 7 commits intotrinodb:masterfrom
arhimondr:exchange-output-selector
Oct 7, 2022
Merged

Improve exchange interfaces to support speculative execution#14448
arhimondr merged 7 commits intotrinodb:masterfrom
arhimondr:exchange-output-selector

Conversation

@arhimondr
Copy link
Copy Markdown
Contributor

@arhimondr arhimondr commented Oct 3, 2022

Description

Non-technical explanation

N/A

Release notes

(X) This is not user-visible or 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`)

@cla-bot cla-bot bot added the cla-signed label Oct 3, 2022
losipiuk
losipiuk reviewed Oct 4, 2022
View reviewed changes
core/trino-spi/src/main/java/io/trino/spi/exchange/ExchangeSourceOutputSelector.java Outdated
Copy link
Copy Markdown
Member

@losipiuk losipiuk Oct 4, 2022

Choose a reason for hiding this comment

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

Is there a contract that we limit task to have at most 127 attempts. Feels reasonable but it should probably be enforced somewhere. In TaskId constructor?

Copy link
Copy Markdown
Contributor Author

@arhimondr arhimondr Oct 4, 2022

Choose a reason for hiding this comment

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

Yeah, that makes sense. Let's address it as a separate PR: #14459

losipiuk
losipiuk reviewed Oct 4, 2022
View reviewed changes
core/trino-spi/src/main/java/io/trino/spi/exchange/ExchangeSourceOutputSelector.java Outdated
Copy link
Copy Markdown
Member

@losipiuk losipiuk Oct 4, 2022

Choose a reason for hiding this comment

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

unknown

Also you should probably provide totalNumberOfPartitions and verify that partitionCount is equal to that if you are building final selector (or do you validate it somewhere?)

Copy link
Copy Markdown
Contributor Author

@arhimondr arhimondr Oct 4, 2022

Choose a reason for hiding this comment

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

Actually yeah, i think it's a good idea to make the builder more restrictive and apply more validations. Improved.

losipiuk
losipiuk reviewed Oct 4, 2022
View reviewed changes
Copy link
Copy Markdown
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.

LGTM (I did not read super deeply)

@arhimondr arhimondr force-pushed the exchange-output-selector branch from 489fe40 to 3edb9db Compare October 4, 2022 19:18
@arhimondr
Copy link
Copy Markdown
Contributor Author

arhimondr commented Oct 4, 2022

@losipiuk Updated

linzebing
linzebing reviewed Oct 5, 2022
View reviewed changes
core/trino-spi/src/main/java/io/trino/spi/exchange/ExchangeSourceOutputSelector.java Outdated
Copy link
Copy Markdown
Member

@linzebing linzebing Oct 5, 2022

Choose a reason for hiding this comment

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

We may need to enforce upper bound to be Short.MAX_VALUE. Or if you think we may exceed this limit, then we need to change it to Integer https://github.com/starburstdata/trino-buffer-service/blob/cdbbc6c08a11f5e472951e6b53ff10c7dbc25b83/data-server/src/main/java/io/starburst/stargate/buffer/data/execution/Partition.java#L152

Copy link
Copy Markdown
Contributor Author

@arhimondr arhimondr Oct 5, 2022

Choose a reason for hiding this comment

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

Currently it's an integer everywhere in Trino. We can change it if needed, but probably as a follow up PR.

core/trino-spi/src/main/java/io/trino/spi/exchange/ExchangeSourceOutputSelector.java Outdated
Copy link
Copy Markdown
Member

@linzebing linzebing Oct 5, 2022

Choose a reason for hiding this comment

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

When will we use exclude?

Copy link
Copy Markdown
Contributor Author

@arhimondr arhimondr Oct 5, 2022

Choose a reason for hiding this comment

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

When a certain partition has to be split (e.g.: due to high memory pressure). In such scenario the original partition id (all attempts) must be excluded and new partition ids (created during split) must be included.

core/trino-spi/src/main/java/io/trino/spi/exchange/ExchangeSourceOutputSelector.java Outdated
Copy link
Copy Markdown
Member

@linzebing linzebing Oct 5, 2022

Choose a reason for hiding this comment

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

It's not super clear to me on when we need such a class instead of constructing a final map which maps taskId to attemptId. For memory efficiency?

Copy link
Copy Markdown
Contributor Author

@arhimondr arhimondr Oct 5, 2022

Choose a reason for hiding this comment

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

Memory efficiency + validations

core/trino-spi/src/main/java/io/trino/spi/exchange/ExchangeSourceOutputSelector.java Outdated
Copy link
Copy Markdown
Member

@linzebing linzebing Oct 5, 2022

Choose a reason for hiding this comment

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

I wonder if we allow transition from a final selector to another final selector

Copy link
Copy Markdown
Contributor Author

@arhimondr arhimondr Oct 5, 2022

Choose a reason for hiding this comment

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

Good point. I don't think we should. Let me forbid it.

@arhimondr arhimondr force-pushed the exchange-output-selector branch from 3edb9db to fd13dfe Compare October 5, 2022 21:21
@arhimondr
Copy link
Copy Markdown
Contributor Author

arhimondr commented Oct 5, 2022

Updated

@arhimondr arhimondr force-pushed the exchange-output-selector branch from fd13dfe to b45ab72 Compare October 6, 2022 10:07
losipiuk
losipiuk reviewed Oct 6, 2022
View reviewed changes
...xchange-filesystem/src/main/java/io/trino/plugin/exchange/filesystem/FileSystemExchange.java Outdated
Copy link
Copy Markdown
Member

@losipiuk losipiuk Oct 6, 2022

Choose a reason for hiding this comment

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

at this commit this code is weird. Why do we have outer loop as we are always exiting from it in frist iteration. I clearly miss sth

Copy link
Copy Markdown
Member

@losipiuk losipiuk Oct 6, 2022

Choose a reason for hiding this comment

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

Oh - it used to be findFirst. Maybe drop for in this commit and only add it in the next one?

Copy link
Copy Markdown
Contributor Author

@arhimondr arhimondr Oct 6, 2022

Choose a reason for hiding this comment

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

Good point, changed

losipiuk
losipiuk reviewed Oct 6, 2022
View reviewed changes
...system/src/main/java/io/trino/plugin/exchange/filesystem/FileSystemExchangeSourceHandle.java Outdated
Copy link
Copy Markdown
Member

@losipiuk losipiuk Oct 6, 2022

Choose a reason for hiding this comment

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

can this be record? Or you worried about getRetainedSizeInBytes?

Copy link
Copy Markdown
Contributor Author

@arhimondr arhimondr Oct 6, 2022

Choose a reason for hiding this comment

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

Yeah, I didn't want to implement getRetainedSizeInBytes myself.

losipiuk
losipiuk reviewed Oct 6, 2022
View reviewed changes
...xchange-filesystem/src/main/java/io/trino/plugin/exchange/filesystem/FileSystemExchange.java Outdated
Copy link
Copy Markdown
Member

@losipiuk losipiuk Oct 6, 2022

Choose a reason for hiding this comment

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

why? Some sinks may still be written, right?

Copy link
Copy Markdown
Contributor Author

@arhimondr arhimondr Oct 6, 2022

Choose a reason for hiding this comment

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

Currently we don't split tasks and don't "forget" entire partitions. Once we have this functionally we will remove this check. For now I wanted to keep it in place for verification purposes.

losipiuk
losipiuk approved these changes Oct 6, 2022
View reviewed changes
Copy link
Copy Markdown
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.

LGTM % questions

@arhimondr
Simplify updateInputsForQueryResults logic in FaultTolerantQuerySched…
417e3fe 
…uler
@arhimondr
Simplify naming in FileSystemExchangeSource
4d9c0bb 
Rename blockedOnSourceHandles to blockedOnFiles
@arhimondr
Include source task/attempt for each file in FileSystemExchangeSource…
b4a6333 
…Handle

To allow post filtering at the FileSystemExchangeSource level
@arhimondr
Select committed attempt provided by the engine
2404832
@arhimondr
Provide list of included attempts explicitly by the engine
45625f0
@arhimondr
Add allRequiredSinksFinished method to Exchange interface
c400991 
The engine may decide that not all the sinks are necessary. It should be
up to the engine to provide a signal to the exchange when all useful
data has already been written.
@arhimondr arhimondr force-pushed the exchange-output-selector branch from b45ab72 to 80b5c99 Compare October 6, 2022 17:25
@arhimondr
Copy link
Copy Markdown
Contributor Author

arhimondr commented Oct 6, 2022

Updated

@arhimondr arhimondr merged commit dd8dee5 into trinodb:master Oct 7, 2022
@arhimondr arhimondr deleted the exchange-output-selector branch October 7, 2022 05:39
@github-actions github-actions bot added this to the 400 milestone Oct 7, 2022
@colebow colebow mentioned this pull request Oct 11, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@losipiuk losipiuk losipiuk approved these changes

+1 more reviewer

@linzebing linzebing linzebing approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed

Milestone

400

Development

Successfully merging this pull request may close these issues.

3 participants

@arhimondr @losipiuk @linzebing