Skip to content

Make createOrOverwrite atomic for Azure FS#22858

Merged
wendigo merged 1 commit intotrinodb:masterfrom
nineinchnick:azure-fs-create-overwrite-atomic
Jul 30, 2024
Merged

Make createOrOverwrite atomic for Azure FS#22858
wendigo merged 1 commit intotrinodb:masterfrom
nineinchnick:azure-fs-create-overwrite-atomic

Conversation

@nineinchnick
Copy link
Copy Markdown
Member

@nineinchnick nineinchnick commented Jul 29, 2024

Description

Before this change, if an exception happens after opening the stream in createOrOverwrite, the stream will be properly closed, leaving behind an empty or partially written file.

I only tested the old behavior manually, I'm not sure how to invoke an exception when calling blobClient.upload().

Additional context and related issues

Release notes

(x) 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:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

There's a version of this method that takes a timeout. We do configure write timeouts in the HTTP client, and I don't want to add another config option, so I didn't use it here.

Copy link
Copy Markdown
Contributor

@mayankvadariya mayankvadariya left a comment

Choose a reason for hiding this comment

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

Sounds like similar changes may be required for other implementation of createOrOverwrite as well?

@electrum
Copy link
Copy Markdown
Member

I’m not sure the release notes are accurate, as this method is only used in one or a few places.

Before this change, if an exception happens after opening the stream in
`createOrOverwrite`, the stream will be properly closed, leaving behind
an empty or partially written file.
@nineinchnick nineinchnick force-pushed the azure-fs-create-overwrite-atomic branch from 4893f0d to 3b1ee24 Compare July 30, 2024 08:03
Copy link
Copy Markdown
Contributor

@SemionPar SemionPar left a comment

Choose a reason for hiding this comment

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

LGTM

@wendigo wendigo merged commit 1073b08 into trinodb:master Jul 30, 2024
@github-actions github-actions bot added this to the 454 milestone Jul 30, 2024
@ebyhr
Copy link
Copy Markdown
Member

ebyhr commented Jul 31, 2024

@nineinchnick @wendigo Please check CI failure on master https://github.com/trinodb/trino/actions/runs/10170139606/job/28128741695

Error:    TestAzureFileSystemGen2Flat>AbstractTestAzureFileSystem.testPaths:209->AbstractTestTrinoFileSystem.testPathHierarchical:698 
Expecting actual throwable to be an instance of any of the following types:
  [java.io.IOException, java.lang.IllegalArgumentException]
but was:
  com.azure.storage.blob.models.BlobStorageException: Status code 404, "<?xml version="1.0" encoding="utf-8"?><Error><Code>ContainerNotFound</Code><Message>The specified container does not exist.
RequestId:94bedc82-901e-003d-2acb-e210c1000000
Time:2024-07-30T22:00:23.5863752Z</Message></Error>"
	at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:733)
	at com.azure.core.implementation.MethodHandleReflectiveInvoker.invokeWithArguments(MethodHandleReflectiveInvoker.java:39)
	at com.azure.core.implementation.http.rest.ResponseExceptionConstructorCache.invoke(ResponseExceptionConstructorCache.java:53)
	...(25 remaining lines not displayed - this can be changed with Assertions.setMaxStackTraceElementsDisplayed)
Error:    TestAzureFileSystemGen2Hierarchical>AbstractTestAzureFileSystem.testPaths:209->AbstractTestTrinoFileSystem.testPathHierarchical:698 
Expecting actual throwable to be an instance of any of the following types:
  [java.io.IOException, java.lang.IllegalArgumentException]
but was:
  com.azure.storage.blob.models.BlobStorageException: Status code 404, "<?xml version="1.0" encoding="utf-8"?><Error><Code>ContainerNotFound</Code><Message>The specified container does not exist.
RequestId:cd691f1b-601e-00bf-4acb-e25e8b000000
Time:2024-07-30T22:00:14.9804852Z</Message></Error>"
	at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:733)
	at com.azure.core.implementation.MethodHandleReflectiveInvoker.invokeWithArguments(MethodHandleReflectiveInvoker.java:39)
	at com.azure.core.implementation.http.rest.ResponseExceptionConstructorCache.invoke(ResponseExceptionConstructorCache.java:53)
	...(25 remaining lines not displayed - this can be changed with Assertions.setMaxStackTraceElementsDisplayed)
Error:    TestAzureFileSystemOAuthGen2Flat>AbstractTestAzureFileSystem.testPaths:209->AbstractTestTrinoFileSystem.testPathHierarchical:698 
Expecting actual throwable to be an instance of any of the following types:
  [java.io.IOException, java.lang.IllegalArgumentException]
but was:
  com.azure.storage.blob.models.BlobStorageException: Status code 404, "<?xml version="1.0" encoding="utf-8"?><Error><Code>ContainerNotFound</Code><Message>The specified container does not exist.
RequestId:3197e210-f01e-000f-6acb-e248[1100](https://github.com/trinodb/trino/actions/runs/10170139606/job/28128741695#step:10:1101)0000
Time:2024-07-30T22:00:01.7589474Z</Message></Error>"
	at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:733)
	at com.azure.core.implementation.MethodHandleReflectiveInvoker.invokeWithArguments(MethodHandleReflectiveInvoker.java:39)
	at com.azure.core.implementation.http.rest.ResponseExceptionConstructorCache.invoke(ResponseExceptionConstructorCache.java:53)
	...(28 remaining lines not displayed - this can be changed with Assertions.setMaxStackTraceElementsDisplayed)
Error:    TestAzureFileSystemOAuthGen2Hierarchical>AbstractTestAzureFileSystem.testPaths:209->AbstractTestTrinoFileSystem.testPathHierarchical:698 
Expecting actual throwable to be an instance of any of the following types:
  [java.io.IOException, java.lang.IllegalArgumentException]
but was:
  com.azure.storage.blob.models.BlobStorageException: Status code 404, "<?xml version="1.0" encoding="utf-8"?><Error><Code>ContainerNotFound</Code><Message>The specified container does not exist.
RequestId:1203ebd2-e01e-009e-16cb-e27af0000000
Time:2024-07-30T22:00:36.9147252Z</Message></Error>"
	at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:733)
	at com.azure.core.implementation.MethodHandleReflectiveInvoker.invokeWithArguments(MethodHandleReflectiveInvoker.java:39)
	at com.azure.core.implementation.http.rest.ResponseExceptionConstructorCache.invoke(ResponseExceptionConstructorCache.java:53)
	...(28 remaining lines not displayed - this can be changed with Assertions.setMaxStackTraceElementsDisplayed)

@nineinchnick nineinchnick deleted the azure-fs-create-overwrite-atomic branch July 31, 2024 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

6 participants