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.

Sign up for GitHub

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

Jump to bottom

Initial addition of common functionality between S3 sink and source connectors #312

Closed
wants to merge 7 commits into from
Closed

Initial addition of common functionality between S3 sink and source connectors #312

wants to merge 7 commits into from

Conversation

aindriu-aiven
Copy link
Contributor

@aindriu-aiven aindriu-aiven commented Oct 10, 2024 •
edited
Loading

  • Addition of a new module/project to allow the sharing of common code between S3 Sink and Source
  • Handles Creation of S3 Aws client
  • Refactors commons AivenCommonConfig and splits into a common config and a Sink and Source config, deprecating the AivenCommonConfig
  • Has common Configuration in S3SinkBaseConfig and S3SourceBaseConfig
    • Includes some config loader methods that are common to both for AWS STS and S3 Retry Policies
  • Updates the S3 Sink Connector to use the s3-common lib
  • Added deprecated config & a separate method to load into config so S3 Sink can decide when to remove deprecated config

@aindriu-aiven aindriu-aiven requested review from a team as code owners October 10, 2024 08:57
@aindriu-aiven aindriu-aiven changed the title Initial addition of common functionaity between S3 sink and source connectors Initial addition of common functionality between S3 sink and source connectors Oct 10, 2024
@aindriu-aiven aindriu-aiven force-pushed the aindriu-aiven/split-common-s3-connector-config branch 4 times, most recently from d41fe78 to 2df807f Compare October 10, 2024 10:06
Claudenw
Claudenw requested changes Oct 10, 2024
View reviewed changes
s3-connectors-common/src/main/java/io/aiven/kafka/connect/iam/AwsAccessSecret.java Outdated Show resolved Hide resolved
s3-connectors-common/src/main/java/io/aiven/kafka/connect/s3/S3BaseConfig.java Outdated Show resolved Hide resolved
s3-connectors-common/src/main/java/io/aiven/kafka/connect/iam/AwsCredentialProviderFactory.java Outdated Show resolved Hide resolved
s3-connectors-common/src/main/java/io/aiven/kafka/connect/iam/AwsStsEndpointConfig.java Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Like AwsAccessSecret this too can be removed.

s3-connectors-common/src/main/java/io/aiven/kafka/connect/iam/AwsCredentialProviderFactory.java Outdated
Comment on lines 55 to 60
final AwsStsEndpointConfig endpointConfig = config.getStsEndpointConfig();
final AwsClientBuilder.EndpointConfiguration stsConfig = new AwsClientBuilder.EndpointConfiguration(
endpointConfig.getServiceEndpoint(), endpointConfig.getSigningRegion());
final AWSSecurityTokenServiceClientBuilder stsBuilder = AWSSecurityTokenServiceClientBuilder.standard();
stsBuilder.setEndpointConfiguration(stsConfig);
return stsBuilder.build();
Copy link
Contributor

Choose a reason for hiding this comment

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

Config should return the AwcClientBuilder.EndpointConfiguration object if the config has the properties. No point in making a bean to carry values when the actual object can be created in the config.

Config could have hasX() methods for the 3 ways AWS lets you build AWSCredential providers.

s3-connectors-common/src/main/java/io/aiven/kafka/connect/iam/AwsStsRole.java Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Another candidate for removal.

s3-connectors-common/src/main/java/io/aiven/kafka/connect/s3/S3BaseConfig.java Outdated Show resolved Hide resolved
s3-connectors-common/src/main/java/io/aiven/kafka/connect/s3/S3BaseConfig.java Outdated
ConfigDef.Width.NONE, AWS_S3_REGION_CONFIG);
}

protected static void addS3SinkConfig(final ConfigDef configDef) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this one should remain in the S3SinkConfig class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I split it out here on purpose so it wouldn't be forgotten about as it is contained in the sink class'
addAwsConfigGroup but not in the source addAwsConfigGroup and I didn't want it forgotten about, by someone doing a straight swap,

Would adding a TODO addAwsConfigGroup on this suffice?

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, ad the TODO plz

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh actually @AnatolyPopov asked me to make the changes to the S3 sink connector in this PR as well, I just need to make time for that at some point today as I am switching between multiple things, however, I will try to include in this PR as well.

s3-connectors-common/src/main/java/io/aiven/kafka/connect/s3/S3ClientUtils.java Outdated Show resolved Hide resolved
@aindriu-aiven aindriu-aiven force-pushed the aindriu-aiven/split-common-s3-connector-config branch from 2dbcfb1 to 21d5761 Compare October 10, 2024 15:34
muralibasani
muralibasani reviewed Oct 11, 2024
View reviewed changes
settings.gradle.kts Outdated Show resolved Hide resolved
@aindriu-aiven aindriu-aiven force-pushed the aindriu-aiven/split-common-s3-connector-config branch from 21d5761 to 7a3073f Compare October 11, 2024 08:14
Claudenw
Claudenw reviewed Oct 16, 2024
View reviewed changes
Copy link
Contributor

@Claudenw Claudenw left a comment

Choose a reason for hiding this comment

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

The code in this change is copied/merged from the source and sink projects or just from source?

Claudenw
Claudenw previously approved these changes Oct 16, 2024
View reviewed changes
@aindriu-aiven
Copy link
Contributor Author

The code in this change is copied/merged from the source and sink projects or just from source?

Actually copied from Sink as the source of truth as Source is a moving target at the moment.

@aindriu-aiven aindriu-aiven force-pushed the aindriu-aiven/split-common-s3-connector-config branch from 8b1b349 to c6638b7 Compare October 21, 2024 08:36
@aindriu-aiven aindriu-aiven mentioned this pull request Nov 4, 2024
Draft
Claudenw
Claudenw previously approved these changes Nov 4, 2024
View reviewed changes
Copy link
Contributor

@Claudenw Claudenw left a comment

Choose a reason for hiding this comment

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

LTGM. Seems that these are mostly copied from elsewhere with slight renames.

I think that, in light of comments made by Greg we should reconsider how we build configs in general. However, that is a largish piece of work and can build upon the framework presented here.

muralibasani
muralibasani requested changes Nov 5, 2024
View reviewed changes
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.

Overall looks good. Have a few comments on refactoring

@aindriu-aiven aindriu-aiven force-pushed the aindriu-aiven/split-common-s3-connector-config branch from 721a367 to 99e925e Compare November 5, 2024 13:49
@aindriu-aiven
Split common Config for source and sink connectors, have S3 sink conn…
46436c0 
…ector use the sink source for configuration

Signed-off-by: Aindriu Lavelle <[email protected]>
@aindriu-aiven aindriu-aiven force-pushed the aindriu-aiven/split-common-s3-connector-config branch from 99e925e to 46436c0 Compare November 6, 2024 13:10
Claudenw
Claudenw previously approved these changes Nov 7, 2024
View reviewed changes
Copy link
Contributor

@Claudenw Claudenw left a comment •
edited
Loading

Choose a reason for hiding this comment

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

There are some bits that I think we need to resolve as architecture decisions. However, I think this change is sufficient and correct enough for merging.

@muralibasani
Copy link
Contributor

There are some bits that I think we need to resolve as architecture decisions. However, I think this change is sufficient and correct enough for merging.

@Claudenw I prefer to wait until these duplicated methods mentioned above are fixed in this pr.

@aindriu-aiven
Copy link
Contributor Author

Going to close this, and replace with a new PR which also introduces the Config Fragments, to the config, this allows us to re-open this PR if required to split the config split into multiple parts if required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@muralibasani muralibasani muralibasani requested changes

@Claudenw Claudenw Claudenw left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aindriu-aiven @muralibasani @Claudenw