Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Batch delete requests for exchange spooling on S3 and Azure #12511

Merged
merged 3 commits into from
May 31, 2022
Merged

Batch delete requests for exchange spooling on S3 and Azure #12511

merged 3 commits into from
May 31, 2022

Conversation

linzebing
Copy link
Member

Description

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

Improvement.

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

trino-filesystem-exchange

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

This will help reduce the number of delete request sent to S3/Azure by batching them.

Related issues, pull requests, and links

Depend on #12360

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

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

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

@cla-bot cla-bot bot added the cla-signed label May 23, 2022
@linzebing linzebing requested review from arhimondr and losipiuk May 23, 2022 05:59
@losipiuk
Copy link
Member

errorprone:

Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.9.0:compile (default-compile) on project trino-exchange-filesystem: Compilation failure
Error:  /home/runner/work/trino/trino/plugin/trino-exchange-filesystem/src/main/java/io/trino/plugin/exchange/filesystem/azure/AzureBlobFileSystemExchangeStorage.java:[270,63] [UnusedVariable] The parameter 'containerName' is never read.
Error:      (see https://errorprone.info/bugpattern/UnusedVariable)
Error:    Did you mean 'return deleteObjects(blobUrls.build());'?
Error:  -> [Help 1]

losipiuk
losipiuk approved these changes May 23, 2022
View reviewed changes
Copy link
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

arhimondr
arhimondr reviewed May 25, 2022
View reviewed changes
...nge-filesystem/src/main/java/io/trino/plugin/exchange/filesystem/FileSystemExchangeUtil.java Outdated
private FileSystemExchangeUtil() {}

// Helper function that translates exception and transform future type to avoid abstraction leak
public static ListenableFuture<Void> transformFuture(ListenableFuture<?> listenableFuture)
Copy link
Contributor

@arhimondr arhimondr May 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if it is possible to give this method a better name. How about calling the method translateFailures? (I know that it also transforms the future into a ListenableFuture<Void> future, but that's probably a less important aspect of it that can be safely omitted without significantly decreasing readability )

...nge-filesystem/src/main/java/io/trino/plugin/exchange/filesystem/FileSystemExchangeUtil.java Outdated
import static com.google.common.util.concurrent.MoreExecutors.directExecutor;
import static io.airlift.concurrent.MoreFutures.asVoid;

public final class FileSystemExchangeUtil
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Generally we are trying to avoid *Util classes as they tend to end up being a dump site of a bunch of loosely related hardly reusable code. I would recommend calling it FileSystemExchangeFutures. It will make it more obvious of what methods this class may contain and potentially increase the likelihood of the methods being used / reused.

@linzebing linzebing force-pushed the batch-delete branch 2 times, most recently from a4685c0 to a679c0b Compare May 25, 2022 22:07
@linzebing
Copy link
Member Author

CI: #12535

losipiuk
losipiuk reviewed May 27, 2022
View reviewed changes
...-filesystem/src/main/java/io/trino/plugin/exchange/filesystem/FileSystemExchangeFutures.java
@@ -0,0 +1,41 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

commit message typo sppoling

@arhimondr arhimondr merged commit 97dce6a into trinodb:master May 31, 2022
@github-actions github-actions bot added this to the 383 milestone May 31, 2022
@colebow colebow mentioned this pull request May 31, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arhimondr arhimondr arhimondr approved these changes

@losipiuk losipiuk losipiuk approved these changes

Assignees
No one assigned
Labels
cla-signed
Milestone
383
Development

Successfully merging this pull request may close these issues.
3 participants
@linzebing @losipiuk @arhimondr