-
Notifications
You must be signed in to change notification settings - Fork 81
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
Add endpointOverride S3 support #5087
Conversation
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 really like this change, just one minor comment
extensions/s3/src/test/java/io/deephaven/extensions/s3/S3SeekableChannelTestBase.java
Outdated
Show resolved
Hide resolved
extensions/s3/src/main/java/io/deephaven/extensions/s3/AwsCredentials.java
Outdated
Show resolved
Hide resolved
extensions/s3/src/main/java/io/deephaven/extensions/s3/S3Instructions.java
Show resolved
Hide resolved
extensions/s3/src/main/java/io/deephaven/extensions/s3/S3Instructions.java
Outdated
Show resolved
Hide resolved
extensions/s3/src/test/java/io/deephaven/extensions/s3/SingletonContainers.java
Outdated
Show resolved
Hide resolved
extensions/s3/src/test/java/io/deephaven/extensions/s3/SingletonContainers.java
Show resolved
Hide resolved
extensions/s3/src/test/java/io/deephaven/extensions/s3/S3SeekableChannelTestBase.java
Outdated
Show resolved
Hide resolved
extensions/s3/src/test/java/io/deephaven/extensions/s3/S3SeekableChannelMinIOTest.java
Show resolved
Hide resolved
extensions/s3/src/main/java/io/deephaven/extensions/s3/BasicCredentials.java
Show resolved
Hide resolved
extensions/s3/src/main/java/io/deephaven/extensions/s3/S3Instructions.java
Outdated
Show resolved
Hide resolved
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 |
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 says "S3", while newer edits in other places carefully use "S3-compatible". Is this just S3, or should the docs be rephrased as compatible?
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.
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.
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.
.
…tegration-testing
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.
Python LGTM. I haven't reviewed the rest.
Labels indicate documentation is required. Issues for documentation have been opened: Community: https://github.com/deephaven/deephaven.io/issues/3686 |
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.