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

chore(S3BasicFileAttributes): Do IO in methods that throw IOException #342

Merged
merged 2 commits into from
Dec 28, 2023

Conversation

guicamest
Copy link
Contributor

@guicamest guicamest commented Dec 25, 2023

Description of changes:

The implementation of S3BasicFileAttributes does IO whenever size(), lastModified() or fileKey() are invoked. When there are network problems, RuntimeExceptions are thrown, since the BasicFileAttributes nio interface does not declare any expected exceptions to be thrown when calling these methods.

In order to conform better with the contract specified by the nio interfaces, the IO done in size(), lastModified() and fileKey() methods is now done before constructing the attributes: when readAttributes from the provider or the attributeview is called. readAttributes in those interfaces specify that it can throw IOException and therefore seems a better place where to do the IO to get the object metadata.

As a side effect, requests to S3 are reduced whenever a user of a library needs more than 1 attribute of a file; for example size and lastModified.

References:

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

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! Thanks

@markjschreiber markjschreiber merged commit 99db624 into awslabs:main Dec 28, 2023
1 check passed
@guicamest guicamest deleted the ioInReadAttributes branch December 29, 2023 09:25
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