Fix #1402 Migrating AWS S3 SDK to v2 #1403
Conversation
seratch
added
enhancement
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1403 +/- ##
============================================
- Coverage 73.68% 73.59% -0.10%
+ Complexity 4389 4383 -6
============================================
Files 476 476
Lines 14139 14171 +32
Branches 1423 1430 +7
============================================
+ Hits 10419 10429 +10
- Misses 2882 2900 +18
- Partials 838 842 +4 ☔ View full report in Codecov by Sentry. |
| boolean bucketExists = false; | ||
| Exception error = null; | ||
| try (S3Client s3 = createS3Client()) { | ||
| bucketExists = s3.headBucket(HeadBucketRequest.builder().bucket(bucketName).build()) != null; |
There was a problem hiding this comment.
@acharyashashank Thanks for the suggestion!
I switched to headBucket because the AWS migration guide recommends using the API in version 2: https://docs.aws.amazon.com/sdk-for-java/latest/developer-guide/migration-service-changes.html. Do you think using getBucketAcl instead could bring benefits such as faster runtime performance?
Even if I use the code snippet provided there, I won't follow AwsServiceException error handling because the goal of this code is to verify if the client has permissions to fetch/modify S3 bucket data, rather than checking the bucket's existence.
| if (!bucketExists) { | ||
| throw new IllegalStateException("Failed to access the Amazon S3 bucket (name: " + bucketName + ")"); | ||
| String message = "Failed to access the Amazon S3 bucket (name: " + bucketName + ", error: " + error + ")"; |
There was a problem hiding this comment.
Will this print out only the error message or the message with the stack trace?
Do we want to have the message of this error contain the trace of another error 🤔
There was a problem hiding this comment.
This prints only this exception, but it depends on how toString method is implemented by an exception class. So, I've tweaked the code to have only the class name and message. Thanks for the suggestion!
seratch
force-pushed
the
issue-1402-aws-v2
branch
from
327a83e to
8651a88
Compare
This pull request resolves #1402
Category (place an
xin each of the[ ])Requirements
Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you agree to those rules.