-
Notifications
You must be signed in to change notification settings - Fork 294
Migrating to new S3 SDK #1124
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
Migrating to new S3 SDK #1124
Conversation
| implementation 'com.sun.jersey.contribs:jersey-guice:1.19.4' | ||
| implementation 'com.google.guava:guava:21.0' | ||
| implementation 'com.google.code.findbugs:jsr305:3.0.2' | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Added AWS SDK v2 dependencies (BOM platform, s3, sts)
- Kept AWS SDK v1 dependencies for EC2, AutoScaling, SNS, and core services
- Created hybrid SDK environment supporting both v1 and v2 simultaneously
| * | ||
| */ | ||
| package com.netflix.priam.aws; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Updated constructor to inject IS3Credential with @nAmed("awss3roleassumption")
- Changed S3 client initialization to use S3Client.builder() with explicit region
- Updated downloadFileImpl() to use RangeReadInputStream with SDK v2 client
- Updated uploadMultipart() to use CreateMultipartUploadRequest builder and CompletedPart list
- Updated uploadFileImpl() to use PutObjectRequest builder and RequestBody.fromBytes()
- Updated metadata handling to use SDK v2 patterns
| * limitations under the License. | ||
| */ | ||
| package com.netflix.priam.aws; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Changed AmazonS3 client to S3Client
- Updated getFileSize() to use HeadObjectRequest with builder pattern
- Updated doesRemoteFileExist() to use SDK v2 builders and exception types (NoSuchKeyException)
- Updated deleteFiles() to use ObjectIdentifier and Delete builders
| */ | ||
|
|
||
| package com.netflix.priam.aws; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Changed ObjectListing to ListObjectsResponse
- Changed client type from AmazonS3 to S3Client
- Updated initListing() to use ListObjectsRequest builder
- Changed S3ObjectSummary to S3Object
- Updated method calls (getObjectSummaries() → contents(), getKey() → key())
- Replaced listNextBatchOfObjects() with manual pagination using nextMarker()
| * | ||
| */ | ||
| package com.netflix.priam.aws; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Changed client type from AmazonS3 to S3Client
- Changed List to List
- Updated uploadPart() to use SDK v2 request builders and RequestBody
- Updated completeUpload() to use CompletedMultipartUpload builder
- Updated abortUpload() to use SDK v2 request builder
- Changed exception types from AmazonClientException to SdkException
| * | ||
| */ | ||
| package com.netflix.priam.backup; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Changed client type from AmazonS3 to S3Client
- Updated read() method to use GetObjectRequest builder
- Changed range format from .setRange(start, end) to .range("bytes=start-end")
- Changed response type from S3ObjectInputStream to ResponseInputStream
| * limitations under the License. | ||
| */ | ||
| package com.netflix.priam.aws.auth; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Implements IS3Credential for instance profile credentials
- Uses SDK v2's InstanceProfileCredentialsProvider
| * limitations under the License. | ||
| */ | ||
| package com.netflix.priam.aws.auth; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Implements IS3Credential for STS role assumption
- Uses SDK v2's StsClient and StsAssumeRoleCredentialsProvider
- Handles automatic credential refresh
- Added @nAmed("s3") annotation to constructor for dependency injection
| * limitations under the License. | ||
| */ | ||
| package com.netflix.priam.aws.auth; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Created new interface for S3-specific credentials using SDK v2
- Defines methods for getting AwsCredentials and AwsCredentialsProvider (SDK v2 types)
mattl-netflix
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not completely done here, but I wanted to unblock you to the extent possible.
| import org.apache.commons.io.IOUtils; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
| import software.amazon.awssdk.core.sync.RequestBody; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put imports in alphabetical order. Ctrl-Opt-o in intellij.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in alphabetical order right? software.* comes after org.*
| .bucket(prefix) | ||
| .key(remotePath); | ||
|
|
||
| // Add metadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two types of comments worth writing:
- Explaining why something was implemented in a way that isn't the simplest possible.
- Javadoc for automated documentation.
Anything explaining what the subsequent code does (redundant as it is obvious by inspection), any links (rot-prone), why something is present (because it is totally essential) should just be omitted as they make it harder to parse the code.
Please delete all comments you have added.
| .key(remotePath); | ||
|
|
||
| // Add metadata | ||
| long lastModified = localFile.lastModified(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please refactor this and the equivalent code in generatePut() into subroutine akin to getObjectMetadata() in the original code.
It looks like that might have been your intention with getObjectMetadataBuilder() all along.
priam/src/main/java/com/netflix/priam/aws/auth/IS3Credential.java
Outdated
Show resolved
Hide resolved
priam/src/main/java/com/netflix/priam/aws/auth/S3RoleAssumptionCredential.java
Outdated
Show resolved
Hide resolved
mattl-netflix
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just confirming explicitly my original review is complete. Let's just work on vetting it. We can meet tomorrow to check up on that and maybe parallelize some work if you like.
- AmazonS3Client.builder() → S3Client.builder()
- .withCredentials() → .credentialsProvider()
- .withRegion() → .region()
- Mutable objects → Immutable builders
- new PutObjectRequest(bucket, key) → PutObjectRequest.builder().bucket(bucket).key(key).build()
- .getXxx() → .xxx() (no "get" prefix in SDK v2)
- .setXxx() → Builder pattern
- AmazonClientException → SdkException
- AmazonServiceException → S3Exception
Test changes:
- Changed imports from SDK v1 to SDK v2
- Updated MockS3PartUploader to use CompletedPart instead of PartETag
- Updated MockAmazonS3Client to mock S3Client (SDK v2) instead of AmazonS3Client
- Updated all exception types and response builders
- Added binding for IS3Credential with @nAmed("s3") annotation to FakeS3Credential
- Kept the existing binding for role assumption
- Added @nAmed("s3") annotation to the constructor parameter
- Added missing import for javax.inject.Named