Support exchange spooling on Azure Blob Storage #12211

linzebing · 2022-05-02T18:49:19Z

Description

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

New feature

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

trino-exchange-plugin

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

This enables users to set up exchange spooling on Azure Blob Storage.

Related issues, pull requests, and links

Documentation

#12467
( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
(x) 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
* Added support for exchange spooling on Azure Blob Storage.

plugin/trino-delta-lake/pom.xml

...-filesystem/src/main/java/io/trino/plugin/exchange/filesystem/azure/ExchangeAzureConfig.java

.../src/main/java/io/trino/plugin/exchange/filesystem/local/LocalFileSystemExchangeStorage.java

...system/src/main/java/io/trino/plugin/exchange/filesystem/s3/S3FileSystemExchangeStorage.java

...-filesystem/src/main/java/io/trino/plugin/exchange/filesystem/azure/ExchangeAzureConfig.java

.../main/java/io/trino/plugin/exchange/filesystem/azure/AzureBlobFileSystemExchangeStorage.java

losipiuk · 2022-05-04T11:26:44Z

.../main/java/io/trino/plugin/exchange/filesystem/azure/AzureBlobFileSystemExchangeStorage.java

+        }
+
+        @Override
+        public ListenableFuture<Void> finish()


would that make sense to mark Writer as finishing so we throw an error if finish is called twice (or we can return previous future for second call).

(can be a followup - if to be done, same applies for s3 implementation)

Actually in ExchangeSink's protocol, for finish() This method is guaranteed not be called more than once..

losipiuk · 2022-05-04T11:28:23Z

.../main/java/io/trino/plugin/exchange/filesystem/azure/AzureBlobFileSystemExchangeStorage.java

+        @Override
+        public long getRetainedSize()
+        {
+            return INSTANCE_SIZE;


nit: there are two lists also - but I guess it is fine to skip those as size should be negligible.

Good catch. Added accounting for blockIds, but not sure how to get estimated size of a ListenableFuture<Void>

.../main/java/io/trino/plugin/exchange/filesystem/azure/AzureBlobFileSystemExchangeStorage.java

losipiuk · 2022-05-04T11:44:46Z

.../main/java/io/trino/plugin/exchange/filesystem/azure/AzureBlobFileSystemExchangeStorage.java

+                if (secretKey.isPresent()) {
+                    blockBlobAsyncClient = blockBlobAsyncClient.getCustomerProvidedKeyAsyncClient(new CustomerProvidedKey(secretKey.get().getEncoded()));
+                }
+                for (int i = 0; i < readableBlocks && fileOffset < fileSize; ++i) {


Do we need && fileOffset < fileSize here? Isn't how readableBlocks is computed enough to not go over file size?

We need. readableBlocks is calculated based on the remaining space in the buffer, but it's possible that the remaining file content is even shorter

losipiuk · 2022-05-04T11:48:19Z

.../main/java/io/trino/plugin/exchange/filesystem/azure/AzureBlobFileSystemExchangeStorage.java

+            if (sliceSize < 0) {
+                sliceSize = sliceInput.readInt();
+            }
+            Slice data = sliceInput.readSlice(sliceSize);


It is not obvious that sliceInput will have at least sliceSize bytes at this point (after single call to fillBuffer) worth a comment.

Edit: well maybe it is ok - as fillBuffer literally ensures buffer is full AND we know a single row will always fit in the buffer. Still, a sentence which states just that would be helpful IMO.

I agree, but I find it a little awkward to add comment here as the code seems to explain itself better.

losipiuk

LGTM - some nitpicks.

arhimondr · 2022-05-06T19:44:35Z

.../main/java/io/trino/plugin/exchange/filesystem/azure/AzureBlobFileSystemExchangeStorage.java

+    public boolean exists(URI file)
+            throws IOException
+    {
+        return blobServiceClient.getBlobContainerClient(getContainerName(file)).getBlobClient(getPath(file)).exists();


Is blobServiceClient.getBlobContainerClient cheap? Do we need to cache it?

It's purely a class wrapper, not sure it's worth it (because the cache will need to be concurrently accessed). Please advise.

If it's just a matter of creating a wrapper object than it's fine

.../main/java/io/trino/plugin/exchange/filesystem/azure/AzureBlobFileSystemExchangeStorage.java

arhimondr · 2022-05-06T19:48:47Z

.../main/java/io/trino/plugin/exchange/filesystem/azure/AzureBlobFileSystemExchangeStorage.java

+                            .transformAsync(response -> toListenableFuture(response.getValue().collectList().toFuture()), directExecutor())
+                            .transform(byteBuffers -> {
+                                int offset = finalBufferFill;
+                                for (ByteBuffer byteBuffer : byteBuffers) {


Is it possible to have the client directly write bytes into a destination buffer (to avoid double copying?)

I don't see an easy way, as Azure SDK doesn't provide any kind of callback/transformer interface to do so.

linzebing · 2022-05-09T18:24:51Z

CI: #12298

linzebing · 2022-05-10T01:32:31Z

CI: #12300

cla-bot bot added the cla-signed label May 2, 2022

linzebing requested review from losipiuk and arhimondr May 2, 2022 18:49

linzebing force-pushed the azure branch from 160c63b to f0bb81c Compare May 4, 2022 00:49

losipiuk reviewed May 4, 2022

View reviewed changes

plugin/trino-delta-lake/pom.xml Show resolved Hide resolved

losipiuk reviewed May 4, 2022

View reviewed changes

...-filesystem/src/main/java/io/trino/plugin/exchange/filesystem/azure/ExchangeAzureConfig.java Outdated Show resolved Hide resolved

losipiuk reviewed May 4, 2022

View reviewed changes

.../src/main/java/io/trino/plugin/exchange/filesystem/local/LocalFileSystemExchangeStorage.java Show resolved Hide resolved

losipiuk reviewed May 4, 2022

View reviewed changes

...system/src/main/java/io/trino/plugin/exchange/filesystem/s3/S3FileSystemExchangeStorage.java Show resolved Hide resolved

losipiuk reviewed May 4, 2022

View reviewed changes

...-filesystem/src/main/java/io/trino/plugin/exchange/filesystem/azure/ExchangeAzureConfig.java Outdated Show resolved Hide resolved

losipiuk reviewed May 4, 2022

View reviewed changes

.../main/java/io/trino/plugin/exchange/filesystem/azure/AzureBlobFileSystemExchangeStorage.java Outdated Show resolved Hide resolved

losipiuk reviewed May 4, 2022

View reviewed changes

.../main/java/io/trino/plugin/exchange/filesystem/azure/AzureBlobFileSystemExchangeStorage.java Outdated Show resolved Hide resolved

losipiuk reviewed May 4, 2022

View reviewed changes

linzebing force-pushed the azure branch from f0bb81c to ade3771 Compare May 4, 2022 19:20

arhimondr reviewed May 6, 2022

View reviewed changes

linzebing force-pushed the azure branch from ade3771 to b6eda6a Compare May 6, 2022 23:45

arhimondr approved these changes May 7, 2022

View reviewed changes

linzebing force-pushed the azure branch 2 times, most recently from 6beb139 to 7e82ec4 Compare May 9, 2022 16:22

linzebing force-pushed the azure branch from 7e82ec4 to edaa09a Compare May 9, 2022 20:10

linzebing added 4 commits May 9, 2022 16:17

Bump azure dependency versions on trino-delta-lake

a4f2638

Slightly adjust isDirectory check in S3FileSystemExchangeStorage

bd8727c

Add support for exchange spooling on Azure Blob Storage

3660928

Slightly refactor LocalFileSystemExchangeStorage

fcce19d

linzebing force-pushed the azure branch from edaa09a to fcce19d Compare May 9, 2022 23:17

losipiuk approved these changes May 10, 2022

View reviewed changes

arhimondr merged commit 260a244 into trinodb:master May 10, 2022

mosabua mentioned this pull request May 10, 2022

Add Trino 381 release notes #12277

Merged

github-actions bot added this to the 381 milestone May 10, 2022

linzebing mentioned this pull request May 18, 2022

Add documentation for exchange spooling on Azure and GCS #12467

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support exchange spooling on Azure Blob Storage #12211

Support exchange spooling on Azure Blob Storage #12211

linzebing commented May 2, 2022 •

edited

Loading

losipiuk May 4, 2022

linzebing May 4, 2022

losipiuk May 4, 2022

linzebing May 4, 2022

losipiuk May 4, 2022

linzebing May 4, 2022

losipiuk May 4, 2022 •

edited

Loading

linzebing May 4, 2022

losipiuk left a comment

arhimondr May 6, 2022

linzebing May 6, 2022 •

edited

Loading

arhimondr May 7, 2022

arhimondr May 6, 2022

linzebing May 6, 2022

linzebing commented May 9, 2022

linzebing commented May 10, 2022

Support exchange spooling on Azure Blob Storage #12211

Support exchange spooling on Azure Blob Storage #12211

Conversation

linzebing commented May 2, 2022 • edited Loading

Description

Related issues, pull requests, and links

Documentation

Release notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

losipiuk May 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

losipiuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linzebing May 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linzebing commented May 9, 2022

linzebing commented May 10, 2022

linzebing commented May 2, 2022 •

edited

Loading

losipiuk May 4, 2022 •

edited

Loading

linzebing May 6, 2022 •

edited

Loading