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.

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

Fix TrinoOutputFile.create javadoc #20867

Conversation

findepi
Copy link
Member

@findepi findepi commented Feb 28, 2024

No description provided.

@findepi findepi added the no-release-notes This pull request does not require release notes entry label Feb 28, 2024
@cla-bot cla-bot bot added the cla-signed label Feb 28, 2024
@@ -53,7 +53,7 @@ default void createExclusive(Slice content)
}

/**
* Create file exclusively, failing if the file already exists. For file systems which do not
* Create file exclusively, failing or overwriting f the file already exists. For file systems which do not
Copy link
Contributor

Choose a reason for hiding this comment

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

dangling f

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks @wendigo , fixed

@oneonestar
Copy link
Member

The semantics are slightly different. This won't affect how caller use create() because currently we don't have a way to check is the underlying FS support it or not. However, this will affect how someone who tried to implement a FS in the future.

// Before
if (FS support create file exclusively and atomically) {
    create file exclusively and atomically
}
else {
    createOrOverwrite()
}

// After
if (FS support create file exclusively and atomically) {
    create file exclusively, could fail or overwriting an existing file
}
else {
    createOrOverwrite()
}

I went thought all the implementations and they all follow the former semantic. I don't think it is necessary to loose the restriction. I don't recommend this change.

HDFS: atomic (https://hadoop.apache.org/docs/stable/hadoop-project-dist/hadoop-common/filesystem/introduction.html#Core_Expectations_of_a_Hadoop_Compatible_FileSystem)
LocalFS: atomic, backed by Files.newOutputStream(StandardOpenOption.CREATE_NEW) 
MemoryFS: atomic, backed by ConcurrentHashMap.putIfAbsent()
Azure: atomic, backed by setIfNoneMatch(HeaderConstants.ETAG_WILDCARD))
GCS: atomic, backed by BlobTargetOption.doesNotExist()
S3: Non-atomic

@findepi
Copy link
Member Author

findepi commented Mar 1, 2024

@oneonestar i think i am correcting the first sentence of the javadoc only.
First sentence should be correct, currently it's not, which is apparent when all javadoc is read.

@findepi findepi force-pushed the findepi/fix-trinooutputfile-create-javadoc-9b6985 branch from ac36a4f to f12f582 Compare March 1, 2024 10:48
@findepi findepi requested a review from wendigo March 1, 2024 10:48
@findepi findepi merged commit 57871d5 into trinodb:master Mar 1, 2024
64 checks passed
@findepi findepi deleted the findepi/fix-trinooutputfile-create-javadoc-9b6985 branch March 1, 2024 15:54
@github-actions github-actions bot added this to the 440 milestone Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed no-release-notes This pull request does not require release notes entry
Development

Successfully merging this pull request may close these issues.

3 participants