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

Add endpointOverride S3 support #5087

Merged

Conversation

devinrsmith
Copy link
Member

Adds the ability to set an endpoint override for S3Instructions. This allows Deephaven to connect to S3-compatible APIs (not just AWS S3) such as backblaze or minio.

In addition, this adds java S3 local integration testing using testcontainers-java, via a localstack container and a minio container.

In support of #5064.

Copy link
Contributor

@malhotrashivam malhotrashivam left a comment

Choose a reason for hiding this comment

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

I really like this change, just one minor comment

malhotrashivam
malhotrashivam previously approved these changes Jan 31, 2024
@devinrsmith devinrsmith requested a review from rcaudy February 5, 2024 17:33
used from software.amazon.awssdk.auth.credentials.DefaultCredentialsProvider.
access_key_id (str): the access key for reading parquet files stored in S3. Both access key and secret
access key must be provided to use static credentials, else default credentials will be used.
secret_access_key (str): the secret access key for reading parquet files stored in S3. Both access key and
Copy link
Member

Choose a reason for hiding this comment

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

This says "S3", while newer edits in other places carefully use "S3-compatible". Is this just S3, or should the docs be rephrased as compatible?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I think saying "S3" is a bit redundant given the module and class name. Also, I don't think we need / want to be explicitly calling out parquet files here, so I've gone ahead and done a light edit of some of the other pydocs.

Copy link
Member

@rcaudy rcaudy left a comment

Choose a reason for hiding this comment

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

.

chipkent
chipkent previously approved these changes Feb 6, 2024
Copy link
Member

@chipkent chipkent left a comment

Choose a reason for hiding this comment

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

Python LGTM. I haven't reviewed the rest.

rcaudy
rcaudy previously approved these changes Feb 6, 2024
@devinrsmith devinrsmith dismissed stale reviews from rcaudy and chipkent via 02d68c4 February 7, 2024 16:50
@devinrsmith devinrsmith enabled auto-merge (squash) February 7, 2024 17:04
@devinrsmith devinrsmith merged commit f77e85a into deephaven:main Feb 7, 2024
19 checks passed
@devinrsmith devinrsmith deleted the nightly/s3-local-integration-testing branch February 7, 2024 18:29
@deephaven-internal
Copy link
Contributor

Labels indicate documentation is required. Issues for documentation have been opened:

Community: https://github.com/deephaven/deephaven.io/issues/3686

@github-actions github-actions bot locked and limited conversation to collaborators Feb 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants