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

Enable random access when opening a SeekableByteChannel or FileChannel #618

Merged
merged 1 commit into from
Mar 4, 2025

Conversation

arouel
Copy link
Contributor

@arouel arouel commented Feb 20, 2025

Description of changes:

  • Enable simultaneous READ and WRITE support when opening a SeekableByteChannel (or FileChannel).
  • Enable random access to the temporarily cached S3 file, so that we can seek to a particular position, and read from or write to that.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@markjschreiber
Copy link
Contributor

Thanks for this contribution. It looks like the CI check failed during integration testing with:

FileChannel$open* should read and write on S3 > open with READ and WRITE is supported FAILED
    java.nio.file.NoSuchFileException at FileChannelTest.java:41

FileChannel$open* should read and write on S3 > open with CREATE and WRITE is supported FAILED
    java.util.concurrent.CompletionException at FileChannelTest.java:67
        Caused by: software.amazon.awssdk.core.exception.SdkClientException at FileChannelTest.java:67

Can you take a look? Let me know if you need help.

@arouel
Copy link
Contributor Author

arouel commented Feb 20, 2025

The test failures are caused by missing AWS credentials while either downloading or uploading the temporary file. I wonder why, because the test setup works. I'm surprised that the test cases in FilesWriteTest work, but the newly added testes don't due to credentials.

@arouel
Copy link
Contributor Author

arouel commented Feb 20, 2025

@markjschreiber I found the problem and fixed it. Please have a look at line 838 to 840 in S3FileSystemProvider. Calling on the path toUri and then resolving the FileSystem didn't work. This can be avoided by resolving the file system directly from the incoming S3Path.

Copy link
Contributor

@markjschreiber markjschreiber left a comment

Choose a reason for hiding this comment

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

Not sure if you saw the comment on adding usnit tests for the new methods in S3WritableByteChannel? With those I believe we can merge this PR.

@markjschreiber
Copy link
Contributor

Updating this PR to merge in other changes. Hopefully this will make it easier to merge when you add unit tests

- Get size from write channel instead of fetching
- This is a workaround for the case where the file does not exist remotely but the caller is working with the open option `CREATE`.
- Add unit tests
@arouel
Copy link
Contributor Author

arouel commented Mar 4, 2025

I've added the unit tests as requested. Let me know if it needs some changes.

Copy link
Contributor

@markjschreiber markjschreiber left a comment

Choose a reason for hiding this comment

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

Nice addition! Thanks.

@markjschreiber markjschreiber merged commit 33e4046 into awslabs:main Mar 4, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants