Use write api #20800

elonazoulay · 2024-02-22T02:49:56Z

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`)

ebyhr · 2024-02-22T03:22:16Z

/test-with-secrets sha=456142157c288d25d917965279ad2627008d7c43

github-actions · 2024-02-22T03:26:03Z

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/7999002596

ebyhr · 2024-02-22T03:36:21Z

/test-with-secrets sha=d8f54d89f31deee461f75758672ec579d0d9333c

github-actions · 2024-02-22T03:40:47Z

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/7999104709

elonazoulay · 2024-02-22T06:09:36Z

/test-with-secrets sha=db78942fd7aa85ae23b51c3521f3eb5d82f66e7e

electrum · 2024-02-22T06:19:32Z

Can you update the file system tests to verify that the file doesn’t exist until the output stream is closed?

lib/trino-filesystem-gcs/src/test/java/io/trino/filesystem/gcs/TestGcsFileSystemConfig.java

lib/trino-filesystem-gcs/src/main/java/io/trino/filesystem/gcs/GcsStorageFactory.java

lib/trino-filesystem-gcs/src/test/java/io/trino/filesystem/gcs/TestGcsFileSystemGcs.java

findepi · 2024-02-22T08:39:53Z

/test-with-secrets sha=bf9f124989286f046fece43bad73613d3d38f788

github-actions · 2024-02-22T09:14:51Z

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/8001732829

findepi · 2024-02-23T12:20:39Z

/test-with-secrets sha=7e7d44c1a71902875866fab97ae1bc27c87bac69

github-actions · 2024-02-23T12:21:35Z

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/8019094596

findepi · 2024-02-23T12:22:03Z

lib/trino-filesystem/src/test/java/io/trino/filesystem/AbstractTestTrinoFileSystem.java

+    }
+
+    @Test
+    @Disabled("This test takes 10 minutes to run locally")


writing 32M takes 10 minutes?

Yep (!) writing and reading the file every 8k bytes to verify that it's not changed until the outputstream is closed.

would it suffice to verify it once just before writing last 8k,
or after writing last 8k and before doing close (= committing the write)?

Good idea! Trying that out and will push

and now it takes 18 seconds:) removing from @Disabled. Thanks for that!

14 seconds for the other test, enabled that one as well.

Now down to about 9.7 - 10 seconds

lib/trino-filesystem/src/test/java/io/trino/filesystem/AbstractTestTrinoFileSystem.java

lib/trino-filesystem-gcs/src/main/java/io/trino/filesystem/gcs/GcsOutputStream.java

findinpath · 2024-02-27T12:05:27Z

lib/trino-filesystem/src/test/java/io/trino/filesystem/AbstractTestTrinoFileSystem.java

+        for (int i = 0; i < bytes.length / 4; i++) {
+            //noinspection NumericCastThatLosesPrecision
+            bytes[i] = (byte) i;
+        }


I don't follow what is happening here.
Why are we filling only a quarter of the array?

This snippet is used in another method below as well - extract in a helper method.

Maybe the hint from #20800 (comment) was not correctly applied?

Seems like the hint was correct: the contents don't matter as long as they differ from the original contents - +1 on extracting, will do. lmk if you have any concern w any of this.

I don't follow what is happening here. Why are we filling only a quarter of the array?

This snippet is used in another method below as well - extract in a helper method.

Update: misread this. This was a bug, thanks for catching! Also extracted to a helper method.

findinpath · 2024-02-27T12:08:17Z

lib/trino-filesystem/src/test/java/io/trino/filesystem/AbstractTestTrinoFileSystem.java

+        }
+        try (OutputStream outputStream = getFileSystem().newOutputFile(location).createOrOverwrite()) {
+            // write a 32 mb file
+            for (int i = 0; i < divide(32 * 1024 * 1024, bytes.length, UNNECESSARY); i++) {


Is 32 MB really necessary ? We do control the size of array being written (8K). Is there local caching that you're trying to avoid ?
There may be some issues on the CI when testing from a Github runner which is far from the region of the GCS bucket.

These were originally to test gcs filesystem: the write buffer under the hood is 16mb, so wanted to choose a size that would ensure we were exercising the case where the buffer flushes to storage. I can do 20mb - currently the 2 tests take 18 seconds and 14 seconds respectively, lmk if you'd like the size lowered.

We could make it 17MB or 18MB

findinpath · 2024-02-27T12:10:47Z

lib/trino-filesystem/src/test/java/io/trino/filesystem/AbstractTestTrinoFileSystem.java

+    }
+
+    @Test
+    public void testLargeFileDoesNotOverwriteUntilClosed()


testLargeFileDoesNotOverwriteUntilClosed -> testLargeFileDoesIsNotOverwrittenUntilClosed

what about combining the 2? :) testLargeFileIsNotOverwrittenUntilClosed?

lib/trino-filesystem-gcs/src/main/java/io/trino/filesystem/gcs/GcsOutputStream.java

electrum · 2024-02-27T19:51:58Z

lib/trino-filesystem/src/test/java/io/trino/filesystem/AbstractTestTrinoFileSystem.java

+        getFileSystem().deleteFile(location);
+        byte[] bytes = new byte[8192];
+        for (int i = 0; i < bytes.length / 4; i++) {
+            //noinspection NumericCastThatLosesPrecision


Make this @SuppressWarnings

electrum · 2024-02-27T19:54:14Z

lib/trino-filesystem/src/test/java/io/trino/filesystem/AbstractTestTrinoFileSystem.java

+    void testFileDoesNotExistUntilClosed()
+            throws Exception
+    {
+        Location location = getRootLocation().appendPath("testFile2");


Let's give these better names

electrum · 2024-02-27T19:54:17Z

lib/trino-filesystem/src/test/java/io/trino/filesystem/AbstractTestTrinoFileSystem.java

+    public void testLargeFileDoesNotExistUntilClosed()
+            throws IOException
+    {
+        Location location = getRootLocation().appendPath("testFile3");


electrum · 2024-02-27T19:54:50Z

lib/trino-filesystem/src/test/java/io/trino/filesystem/AbstractTestTrinoFileSystem.java

+            bytes[i] = (byte) i;
+        }
+        try (OutputStream outputStream = getFileSystem().newOutputFile(location).create()) {
+            // write a 32 mb file


Nit: use uppercase MB to match other usages

What buffer size do we care about here? If this is for a 16MB buffer, we should use 17MB, or 33MB for a 32MB buffer, to ensure we have written more than the buffer flush size.

16mb, so will try 17mb. But that only applies to gcs.

electrum · 2024-02-27T19:55:48Z

lib/trino-filesystem/src/test/java/io/trino/filesystem/AbstractTestTrinoFileSystem.java

+        Location location = getRootLocation().appendPath("testFile3");
+        getFileSystem().deleteFile(location);
+        byte[] bytes = new byte[8192];
+        for (int i = 0; i < bytes.length / 4; i++) {


This shouldn't be / 4

thanks! didn't catch that when I changed the function.

electrum · 2024-02-27T19:56:44Z

lib/trino-filesystem/src/test/java/io/trino/filesystem/AbstractTestTrinoFileSystem.java

+        }
+        try (OutputStream outputStream = getFileSystem().newOutputFile(location).create()) {
+            // write a 32 mb file
+            for (int i = 0; i < divide(32 * 1024 * 1024, bytes.length, UNNECESSARY); i++) {


I don't understand what this divide() call does here

It was from a previous review suggestion above - to ensure that the size is divisible by the bytes.length.

electrum · 2024-02-27T20:05:25Z

lib/trino-filesystem/src/test/java/io/trino/filesystem/AbstractTestTrinoFileSystem.java

+        }
+        try (OutputStream outputStream = getFileSystem().newOutputFile(location).create()) {
+            // write a 32 mb file
+            for (int i = 0; i < divide(32 * 1024 * 1024, bytes.length, UNNECESSARY); i++) {


How about we do

assertThat(MEGABYTE % bytes.length).isEqualTo(0); // write a 33 MB file int target = 33 * MEGABYTE; int count = 0; while (count < target) { outputStream.write(bytes); count += bytes.length; if (count % MEGABYTE == 0) { assertFalse(fileExistsInListing(location)); assertFalse(fileExists(location)); } }

Nice! They both take about 10-11seconds now.

supercool. do we need to check every megabyte (33 times)?
would we lose anything if we asserted only before final flush/close?
ie do we believe it is possible for a file to first show up, then disappear, and then show up again while we're writing?

I don't think so - updating now, will save this version just in case.

electrum · 2024-02-27T20:05:48Z

lib/trino-filesystem/src/test/java/io/trino/filesystem/AbstractTestTrinoFileSystem.java

+    public void testLargeFileDoesNotOverwriteUntilClosed()
+            throws IOException
+    {
+        Location location = getRootLocation().appendPath("testFile4");


electrum · 2024-02-27T20:07:14Z

lib/trino-filesystem/src/test/java/io/trino/filesystem/AbstractTestTrinoFileSystem.java

+    private boolean fileExistsInListing(Location location)
+            throws IOException
+    {
+        for (FileIterator fileIterator = getFileSystem().listFiles(getRootLocation()); fileIterator.hasNext(); ) {


Nit: this reads better as

FileIterator fileIterator = getFileSystem().listFiles(getRootLocation()); while (fileIterator.hasNext()) {

findepi · 2024-02-28T09:49:16Z

lib/trino-filesystem/src/test/java/io/trino/filesystem/AbstractTestTrinoFileSystem.java

+            throws IOException
+    {
+        if (!supportsIncompleteWriteNoClobber()) {
+            return;


abort("...") to mark it skipped

findepi · 2024-02-28T09:49:19Z

lib/trino-filesystem/src/test/java/io/trino/filesystem/AbstractTestTrinoFileSystem.java

+            throws IOException
+    {
+        if (!supportsIncompleteWriteNoClobber()) {
+            return;


abort("...") to mark it skipped

findepi · 2024-02-28T09:50:17Z

lib/trino-filesystem/src/test/java/io/trino/filesystem/AbstractTestTrinoFileSystem.java

+    }
+
+    @Test
+    @SuppressWarnings("NumericCastThatLosesPrecision")


Do not @SuppressWarnings on whole method (unless it's a very short method).
use either @SuppressWarnings on "a variable declaration with assignment" or use comment-based suppression

findepi · 2024-02-28T09:51:27Z

lib/trino-filesystem/src/test/java/io/trino/filesystem/AbstractTestTrinoFileSystem.java

+            // write a 17 MB file
+            int target = 17 * MEGABYTE;


Code-comment why 17 and not some other number. (The idea was to write more than assumed buffer size)

(same above)

findepi · 2024-02-28T09:52:06Z

lib/trino-filesystem/src/test/java/io/trino/filesystem/AbstractTestTrinoFileSystem.java

+                if (count % MEGABYTE == 0) {
+                    assertEquals(fileSize, readFile(inputFile, fileBytes));
+                    assertEquals("test", new String(fileBytes, UTF_8));


it would be enough to do this once, before existing from try (OutputStream outputStream = getFileSystem().newOutputFile(location).createOrOverwrite()) { block

findepi · 2024-02-28T09:52:38Z

lib/trino-filesystem/src/test/java/io/trino/filesystem/AbstractTestTrinoFileSystem.java

+                if (count % MEGABYTE == 0) {
+                    assertFalse(fileExistsInListing(location));
+                    assertFalse(fileExists(location));
+                }


it would be enough to do this once, before existing from try (OutputStream outputStream = getFileSystem().newOutputFile(location).createOrOverwrite()) { block

findepi · 2024-02-29T09:35:57Z

there are (related) failures (eg in TestHdfsFileSystemLocal)

elonazoulay requested a review from electrum February 22, 2024 02:50

cla-bot bot added the cla-signed label Feb 22, 2024

elonazoulay requested a review from findinpath February 22, 2024 02:50

github-actions bot added the tests:hive label Feb 22, 2024

elonazoulay requested a review from findepi February 22, 2024 02:51

elonazoulay force-pushed the use_write_api branch from 4561421 to d8f54d8 Compare February 22, 2024 03:24

elonazoulay requested a review from wendigo February 22, 2024 05:39

elonazoulay force-pushed the use_write_api branch from db78942 to bfb3b1f Compare February 22, 2024 06:50

findinpath requested a review from pajaks February 22, 2024 07:46

elonazoulay force-pushed the use_write_api branch from bfb3b1f to bf9f124 Compare February 22, 2024 08:27

findepi reviewed Feb 22, 2024

View reviewed changes

lib/trino-filesystem-gcs/src/test/java/io/trino/filesystem/gcs/TestGcsFileSystemConfig.java Outdated Show resolved Hide resolved

findepi reviewed Feb 22, 2024

View reviewed changes

lib/trino-filesystem-gcs/src/main/java/io/trino/filesystem/gcs/GcsStorageFactory.java Outdated Show resolved Hide resolved

findepi reviewed Feb 22, 2024

View reviewed changes

lib/trino-filesystem-gcs/src/test/java/io/trino/filesystem/gcs/TestGcsFileSystemGcs.java Outdated Show resolved Hide resolved

lib/trino-filesystem-gcs/src/test/java/io/trino/filesystem/gcs/TestGcsFileSystemGcs.java Outdated Show resolved Hide resolved

elonazoulay force-pushed the use_write_api branch from bf9f124 to 4f02b29 Compare February 22, 2024 08:57

elonazoulay force-pushed the use_write_api branch from 4f02b29 to 7e7d44c Compare February 23, 2024 11:08

elonazoulay requested a review from findepi February 23, 2024 11:14

findepi reviewed Feb 23, 2024

View reviewed changes

lib/trino-filesystem/src/test/java/io/trino/filesystem/AbstractTestTrinoFileSystem.java Outdated Show resolved Hide resolved

findepi reviewed Feb 23, 2024

View reviewed changes

lib/trino-filesystem/src/test/java/io/trino/filesystem/AbstractTestTrinoFileSystem.java Outdated Show resolved Hide resolved

findepi reviewed Feb 23, 2024

View reviewed changes

elonazoulay force-pushed the use_write_api branch 3 times, most recently from efdaffa to c4e3a3a Compare February 27, 2024 05:43

findinpath reviewed Feb 27, 2024

View reviewed changes

findinpath assigned elonazoulay Feb 27, 2024

electrum reviewed Feb 27, 2024

View reviewed changes

elonazoulay force-pushed the use_write_api branch 2 times, most recently from 5759144 to 4520de2 Compare February 28, 2024 09:41

findepi approved these changes Feb 28, 2024

View reviewed changes

findepi mentioned this pull request Feb 28, 2024

Trino FS Write API #20869

Closed

elonazoulay force-pushed the use_write_api branch from 4520de2 to 340b057 Compare February 29, 2024 09:20

elonazoulay force-pushed the use_write_api branch from 340b057 to c638adc Compare February 29, 2024 10:24

Use write instead of create for GcsOutputStream

78d370f

elonazoulay force-pushed the use_write_api branch from c638adc to 78d370f Compare March 2, 2024 07:59

electrum approved these changes Mar 4, 2024

View reviewed changes

electrum merged commit 59db1f7 into trinodb:master Mar 4, 2024
62 checks passed

github-actions bot added this to the 440 milestone Mar 4, 2024

colebow mentioned this pull request Mar 6, 2024

Add Trino 440 release notes #20797

Merged

Use write api #20800

Use write api #20800

Conversation

elonazoulay commented Feb 22, 2024

Description

Additional context and related issues

Release notes

ebyhr commented Feb 22, 2024

github-actions bot commented Feb 22, 2024 • edited Loading

ebyhr commented Feb 22, 2024

github-actions bot commented Feb 22, 2024 • edited Loading

elonazoulay commented Feb 22, 2024

electrum commented Feb 22, 2024

findepi commented Feb 22, 2024

github-actions bot commented Feb 22, 2024 • edited Loading

findepi commented Feb 23, 2024

github-actions bot commented Feb 23, 2024 • 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

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

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

findinpath Feb 27, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elonazoulay Feb 28, 2024 • 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

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

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

Choose a reason for hiding this comment

findepi commented Feb 29, 2024

github-actions bot commented Feb 22, 2024 •

edited

Loading

github-actions bot commented Feb 22, 2024 •

edited

Loading

github-actions bot commented Feb 22, 2024 •

edited

Loading

github-actions bot commented Feb 23, 2024 •

edited

Loading

findinpath Feb 27, 2024 •

edited

Loading

elonazoulay Feb 28, 2024 •

edited

Loading