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

S3 Ingestion from non-default endpoints #11798

Merged
merged 9 commits into from
Jul 15, 2022
Merged

Conversation

a2l007
Copy link
Contributor

@a2l007 a2l007 commented Oct 13, 2021

Description

S3InputSource presently provides an option to override the default S3 credentials which is useful to read from different buckets, but it has to be in the same endpoint. This PR adds functionality to specify a different endpoint and associated client properties as part of the S3InputSource spec. This can be helpful for operators looking to migrate data from one store to another. This is implemented only for S3 at the moment, but we can support other cloud providers as a followup.

Key changed/added classes in this PR
  • S3InputSource

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • been tested in a test Druid cluster.

},

"properties": {
"accessKeyId": "KLJ78979SDFdS2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace with xxxx or something ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for taking a look. These are fake credentials.

@stale
Copy link

stale bot commented Apr 19, 2022

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the [email protected] list. Thank you for your contributions.

@@ -98,16 +109,49 @@ public S3InputSource(
@JsonProperty("objects") @Nullable List<CloudObjectLocation> objects,
@JsonProperty("filter") @Nullable String filter,
@JsonProperty("properties") @Nullable S3InputSourceConfig s3InputSourceConfig,
@JsonProperty("proxyConfig") @Nullable AWSProxyConfig awsProxyConfig,
@JsonProperty("endpointConfig") @Nullable AWSEndpointConfig awsEndpointConfig,
@JsonProperty("clientConfig") @Nullable AWSClientConfig awsClientConfig,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reusing the Config objects here so that we don't duplicate the properties. We could push these under S3InputSourceConfig itself if needed.

@a2l007
Copy link
Contributor Author

a2l007 commented Jul 11, 2022

@maytasm Could you please take a look when you get a chance? Thanks!

Copy link
Contributor

@maytasm maytasm left a comment

Choose a reason for hiding this comment

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

Change LGTM

@maytasm
Copy link
Contributor

maytasm commented Jul 12, 2022

@a2l007 Approved. Looks like there is a test failure in intellij inspections.

@a2l007
Copy link
Contributor Author

a2l007 commented Jul 13, 2022

Thanks @maytasm for the review. I've fixed the build failure.
@cryptoe Did you have any other comments before this is merged?

"host='" + host + '\'' +
", port=" + port +
", username='" + username + '\'' +
", password='" + password + '\'' +
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit: Should we toString the password here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, removed it.

Copy link
Contributor

@cryptoe cryptoe left a comment

Choose a reason for hiding this comment

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

LGTM apart from minor nit!!

@a2l007
Copy link
Contributor Author

a2l007 commented Jul 15, 2022

Thanks for the review @cryptoe

@a2l007 a2l007 merged commit 7504597 into apache:master Jul 15, 2022
@abhishekagarwal87 abhishekagarwal87 added this to the 24.0.0 milestone Aug 26, 2022
abhishekagarwal87 added a commit to abhishekagarwal87/druid that referenced this pull request Sep 7, 2022
abhishekagarwal87 pushed a commit to abhishekagarwal87/druid that referenced this pull request Sep 7, 2022
* Add endpoint support for s3inputsource

* Changes to tests

* Fix docs

* Fix config

* Fix inspections

* Fix spelling

* Remove password from toString
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants