Skip to content

Implement exchange spooling#10823

Merged
losipiuk merged 3 commits intotrinodb:masterfrom
linzebing:exchange-spooling
Feb 15, 2022
Merged

Implement exchange spooling#10823
losipiuk merged 3 commits intotrinodb:masterfrom
linzebing:exchange-spooling

Conversation

@linzebing
Copy link
Copy Markdown
Member

@linzebing linzebing commented Jan 27, 2022 •
edited
Loading

See previous PR #10376 for discussions. I accidentally deleted the remote branch and the PR became orphaned so open this new one. I have addressed or responded to all the comments in the previous PR.

This PR resolves #9936

Overview

Supports both local file system and AWS S3. Parallel reading will be a separate PR.

Testing

Added MinIO testing to test S3-related codepaths. Tested TPC-H Q1-Q6 locally on a scale factor of 10.

@cla-bot cla-bot bot added the cla-signed label Jan 27, 2022
@linzebing linzebing changed the title Implement Exchange spooling Implement exchange spooling Jan 27, 2022
@linzebing linzebing force-pushed the exchange-spooling branch 3 times, most recently from 9cb13e3 to 2e04fae Compare January 28, 2022 02:59
@linzebing linzebing force-pushed the exchange-spooling branch 2 times, most recently from 7fe9ab9 to 54b8b7a Compare February 1, 2022 05:56
@linzebing linzebing mentioned this pull request Feb 1, 2022
Merged
@linzebing linzebing force-pushed the exchange-spooling branch 2 times, most recently from 820e6cb to 379c630 Compare February 2, 2022 00:34
arhimondr
arhimondr reviewed Feb 2, 2022
View reviewed changes
losipiuk
losipiuk reviewed Feb 9, 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.

Great job. Looks nice.

arhimondr
arhimondr reviewed Feb 10, 2022
View reviewed changes
plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveFileQueryFailureRecoveryTest.java Outdated
Copy link
Copy Markdown
Contributor

@arhimondr arhimondr Feb 9, 2022

Choose a reason for hiding this comment

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

@losipiuk Actually I'm thinking if we even need these tests. These tests take a lot of time to finish. I wonder if we should simply run with MinIO for both exchange and storage?

Copy link
Copy Markdown
Member Author

@linzebing linzebing Feb 10, 2022

Choose a reason for hiding this comment

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

Changed to both use MinIO. Indeed, these tests are really time-consuming.

plugin/trino-exchange/src/main/java/io/trino/plugin/exchange/FileSystemExchangeSink.java Outdated
Copy link
Copy Markdown
Contributor

@arhimondr arhimondr Feb 9, 2022

Choose a reason for hiding this comment

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

I'm afraid synchronization is still needed, as both abort and finish may run concurrently.

Copy link
Copy Markdown
Member Author

@linzebing linzebing Feb 11, 2022

Choose a reason for hiding this comment

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

If abort and finish run concurrently, per-partition locking will not help. Added null check for buffer and writers.

(however at the very end you suggested a restructure of the code organization which should simplify the concurrency reasoning here)

@linzebing linzebing force-pushed the exchange-spooling branch 5 times, most recently from 0c634b4 to 574b005 Compare February 14, 2022 02:16
losipiuk
losipiuk reviewed Feb 14, 2022
View reviewed changes
plugin/trino-exchange/src/main/java/io/trino/plugin/exchange/FileSystemExchangeSink.java Outdated
Copy link
Copy Markdown
Member

@losipiuk losipiuk Feb 14, 2022

Choose a reason for hiding this comment

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

why not synchronize this as a whole?

Copy link
Copy Markdown
Contributor

@arhimondr arhimondr Feb 14, 2022

Choose a reason for hiding this comment

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

I assume to avoid running completableFuture.complete(null); under a lock

Copy link
Copy Markdown
Member Author

@linzebing linzebing Feb 14, 2022

Choose a reason for hiding this comment

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

Yes, trying to complete the future outside of the lock here.

arhimondr
arhimondr approved these changes Feb 14, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@arhimondr arhimondr 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

plugin/trino-exchange/src/main/java/io/trino/plugin/exchange/FileSystemExchangeSink.java Outdated
Copy link
Copy Markdown
Contributor

@arhimondr arhimondr Feb 14, 2022

Choose a reason for hiding this comment

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

I assume to avoid running completableFuture.complete(null); under a lock

@linzebing linzebing force-pushed the exchange-spooling branch 2 times, most recently from c02b71c to fe8c17b Compare February 15, 2022 07:00
@losipiuk losipiuk merged commit 426e79a into trinodb:master Feb 15, 2022
@mosabua mosabua mentioned this pull request Feb 15, 2022
Merged
@github-actions github-actions bot added this to the 371 milestone Feb 15, 2022
@linzebing linzebing deleted the exchange-spooling branch February 16, 2022 23:28
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

@arhimondr arhimondr arhimondr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@linzebing linzebing

Labels

cla-signed

Milestone

371

Development

Successfully merging this pull request may close these issues.

Implement DFS based external exchange

3 participants

@linzebing @losipiuk @arhimondr