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 storage classes config support #24934

Merged
merged 1 commit into from
Feb 15, 2025

Conversation

EdenKik
Copy link
Contributor

@EdenKik EdenKik commented Feb 6, 2025

Description

This PR enhances Trino's compatibility with S3 storage classes and adds support for configuring the storage class of objects written to S3. By introducing the s3.storage-class property, users can now specify how objects should be stored, enabling better control over storage costs and retrieval performance.

Closes issue #24698

Release notes

## Hive connector
* Add support for configuring s3.storage-class when writing objects to S3. ({issue}24698)

## Iceberg connector
* Add support for configuring s3.storage-class when writing objects to S3. ({issue}24698)

Copy link

cla-bot bot commented Feb 6, 2025

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: EdenKik.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@github-actions github-actions bot added the docs label Feb 6, 2025
@EdenKik EdenKik force-pushed the s3-storage-classes-config-support branch from 8da6edb to e541e2c Compare February 6, 2025 16:04
@cla-bot cla-bot bot added the cla-signed label Feb 6, 2025
@wendigo wendigo requested a review from anusudarsan February 7, 2025 16:39
Copy link
Member

@anusudarsan anusudarsan left a comment

Choose a reason for hiding this comment

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

you can squash all commits

@EdenKik EdenKik force-pushed the s3-storage-classes-config-support branch 2 times, most recently from 39c8191 to 5d02c5f Compare February 10, 2025 19:40
@mosabua mosabua requested a review from electrum February 10, 2025 21:37
@EdenKik EdenKik force-pushed the s3-storage-classes-config-support branch from 5d02c5f to 218a639 Compare February 13, 2025 06:11
@findinpath findinpath requested a review from wendigo February 14, 2025 14:22
STANDARD_IA,
INTELLIGENT_TIERING;

public static StorageClass getStorageClass(S3FileSystemConfig.StorageClassType storageClass)
Copy link
Contributor

@wendigo wendigo Feb 14, 2025

Choose a reason for hiding this comment

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

toStorageClass()

import StorageClassType

}

@Config("s3.storage-class")
@ConfigDescription("The S3 storage class to use when writing the data")
Copy link
Contributor

Choose a reason for hiding this comment

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

S3 storage class used while writing data

@@ -49,6 +50,7 @@ public void testDefaults()
.setExternalId(null)
.setStsEndpoint(null)
.setStsRegion(null)
.setStorageClass(StorageClassType.STANDARD)
Copy link
Contributor

Choose a reason for hiding this comment

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

static import STANDARD

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we cannot use it in this case because both RetryMode and StorageClassType have a STANDARD constant. This causes ambiguity in the code, as the compiler cannot distinguish between the two without explicitly mentioning their parent classes.

@@ -124,6 +127,7 @@ public void testExplicitPropertyMappings()
.setExternalId("myid")
.setStsEndpoint("sts.example.com")
.setStsRegion("us-west-2")
.setStorageClass(StorageClassType.STANDARD_IA)
Copy link
Contributor

Choose a reason for hiding this comment

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

static import STANDARD_IA

Copy link
Contributor

@wendigo wendigo left a comment

Choose a reason for hiding this comment

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

LGTM % couple of comments

@@ -30,6 +30,8 @@ support:
- Required region name for S3.
* - `s3.path-style-access`
- Use path-style access for all requests to S3
* - `s3.storage-class`
- Set the S3 storage class to use when writing the data, defaults to STANDARD
Copy link
Member

Choose a reason for hiding this comment

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

S3 storage class to use while writing data. Defaults to STANDARD.

Add s3.storage-class property integration in S3 operations

Modify configuration tests to use storage class property

Update Trino documentation to describe s3.storage-class configuration

Implement whitelist for supported storage classes and map to SDK options
@EdenKik EdenKik force-pushed the s3-storage-classes-config-support branch from 218a639 to 93e8ec6 Compare February 15, 2025 05:47
@wendigo wendigo merged commit 88df818 into trinodb:master Feb 15, 2025
66 checks passed
@github-actions github-actions bot added this to the 471 milestone Feb 15, 2025
@wendigo
Copy link
Contributor

wendigo commented Feb 15, 2025

Thanks @EdenKik

@mosabua
Copy link
Member

mosabua commented Feb 18, 2025

Added to release notes for all four connectors.

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

Successfully merging this pull request may close these issues.

5 participants