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

AWS SDK 2.X migration for source connector [KCON-84] #374

Merged

Conversation

aindriu-aiven
Copy link
Contributor

@aindriu-aiven aindriu-aiven commented Dec 17, 2024

The AWS 1.X sdk is in maintenance mode and will be out of support by December 2025.

Key differences are

  • Use of the builder pattern when creating objects
  • get and set removed from getters and setters e.g. getKey(), setKey(newKey) -> key(), key(newKey)
  • S3Client is immutable
  • different package names
  • Additional built in functionality removing some of the work from the connector implementation and having the existing library handle it.

SDK 1.X still in use by sink connector but that will be required to be updated as well in the future, but this means the s3-commons code has both the 1.x and 2.x jars.

@aindriu-aiven aindriu-aiven changed the base branch from main to s3-source-release December 17, 2024 13:50
@aindriu-aiven aindriu-aiven force-pushed the aindriu-aiven/aws-sdk-2X branch from 6da63c0 to 7c4aea9 Compare December 17, 2024 14:45
@aindriu-aiven aindriu-aiven marked this pull request as ready for review December 17, 2024 14:48
@aindriu-aiven aindriu-aiven requested review from a team as code owners December 17, 2024 14:48
Copy link
Contributor

@RyanSkraba RyanSkraba left a comment

Choose a reason for hiding this comment

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

The new method names are a bit inconsistent:

  • getAwsXyx -> getAwsV2Xyz
  • getStsXyx -> getStsV2Xyz
  • getCustomXyx -> getV2CustomXyz

I'd suggest using V2 as a strict suffix to the original method name as opposed to trying to fit it someplace in-between?

@@ -18,10 +18,13 @@ plugins { id("aiven-apache-kafka-connectors-all.java-conventions") }

val amazonS3Version by extra("1.12.777")
val amazonSTSVersion by extra("1.12.777")
val amazonV2Version by extra("2.29.34")
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be updated to 2.29.36 😆 Please try to keep up!!

(Just kidding, but we should keep in mind to bump it to the most recent patch version before release!)

if (Objects.isNull(config.getAwsS3EndPoint())) {
return null;
return S3Client.builder()
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice clean-up for readability here!

new PredefinedBackoffStrategies.FullJitterBackoffStrategy(
Math.toIntExact(config.getS3RetryBackoffDelayMs()),
Math.toIntExact(config.getS3RetryBackoffMaxDelayMs())),
config.getS3RetryBackoffMaxRetries(), false));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you dropped this maxRetries! You can add it later to the r -> r .backoffStrategy(backoffStrategy).maxAttempts(config.getS3RetryBackoffMaxRetries()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated and also updated the new method names conventions.

final ListObjectsV2Request request = ListObjectsV2Request.builder()
.bucket(bucketName)
.maxKeys(s3SourceConfig.getS3ConfigFragment().getFetchPageSize() * PAGE_SIZE_FACTOR)
.prefix(optionalKey(s3SourceConfig.getAwsS3Prefix()))
Copy link
Contributor

Choose a reason for hiding this comment

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

I checked, this looks like the correct way to set/unset a configuration (null/not-null)

return s3Client.listObjectsV2(
new ListObjectsV2Request().withContinuationToken(response.getNextContinuationToken()));
return s3Client.listObjectsV2(ListObjectsV2Request.builder()
.maxKeys(s3SourceConfig.getS3ConfigFragment().getFetchPageSize() * PAGE_SIZE_FACTOR)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to have maxKeys here, just for my understanding, as we don't have prefix check.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the maxkeys is still required here but I would probably need to test it to see if it remembers the original max keys setting as I could not find that info in the documentation.

Copy link
Contributor

@muralibasani muralibasani left a comment

Choose a reason for hiding this comment

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

Looks good in general.

…long term support.

Signed-off-by: Aindriu Lavelle <aindriu.lavelle@aiven.io>
Signed-off-by: Aindriu Lavelle <aindriu.lavelle@aiven.io>
Signed-off-by: Aindriu Lavelle <aindriu.lavelle@aiven.io>
…ved from the source connector

Signed-off-by: Aindriu Lavelle <aindriu.lavelle@aiven.io>
…retry

Signed-off-by: Aindriu Lavelle <aindriu.lavelle@aiven.io>
@RyanSkraba RyanSkraba force-pushed the aindriu-aiven/aws-sdk-2X branch from ea261ae to ec1ccdf Compare December 27, 2024 09:30
@RyanSkraba RyanSkraba changed the title aws sdk 2.X migration for source connector AWS SDK 2.X migration for source connector [KCON-84] Dec 30, 2024
@RyanSkraba RyanSkraba merged commit b4475c9 into Aiven-Open:s3-source-release Dec 30, 2024
8 checks 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.

3 participants