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

Make region optional for S3 instructions #5603

Merged
merged 3 commits into from
Jun 12, 2024

Conversation

malhotrashivam
Copy link
Contributor

Before this change, region was a mandatory parameter for reading parquet files from S3.
After this change, if region is not provided, it will be picked up by the AWS sdk using the following provider chain:

1. Check the 'aws.region' system property for the region.
2. Check the 'AWS_REGION' environment variable for the region.
3. Check the {user.home}/.aws/credentials and {user.home}/.aws/config files for the region.
4. If running in EC2, check the EC2 metadata service for the region.

Throw exception if not found in any of the locations above

Reference: https://sdk.amazonaws.com/java/api/2.0.0-preview-11/software/amazon/awssdk/awscore/client/builder/AwsClientBuilder.html

Also, since region was the only mandatory parameter in S3 instructions, after this change, reading from S3 will be allowed without passing any S3 instructions. For example, code like the following python example will work, as long as the region, access key and secret key are set properly using properties or config files:

from deephaven import parquet

test = parquet.read("s3://bucket-name/file-name.parquet")   # Note no s3 instructions

@malhotrashivam malhotrashivam added parquet Related to the Parquet integration DocumentationNeeded ReleaseNotesNeeded Release notes are needed s3 labels Jun 11, 2024
@malhotrashivam malhotrashivam added this to the 3. May 2024 milestone Jun 11, 2024
@malhotrashivam malhotrashivam self-assigned this Jun 11, 2024
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.

My comments are the same as Larry's.

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.

Caught up

Copy link
Contributor

@jmao-denver jmao-denver left a comment

Choose a reason for hiding this comment

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

Python changes LGTM

@malhotrashivam malhotrashivam merged commit e3bf07e into deephaven:main Jun 12, 2024
15 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jun 12, 2024
@deephaven-internal
Copy link
Contributor

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

Community: deephaven/deephaven-docs-community#234

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
DocumentationNeeded parquet Related to the Parquet integration ReleaseNotesNeeded Release notes are needed s3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants