-
Notifications
You must be signed in to change notification settings - Fork 3k
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 intelligent tiering storage class #3032
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.
LGTM. A few nits
@@ -109,6 +111,7 @@ public void testExplicitPropertyMappings() | |||
.setS3PathStyleAccess(true) | |||
.setS3UseInstanceCredentials(false) | |||
.setS3IamRole("roleArn") | |||
.setS3StorageClass(PrestoS3StorageClass.INTELLIGENT_TIERING) |
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 guess we can import the enum directly
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.
@Praveen2112 Importing Enum directly might look off since PrestoS3SignerType
and PrestoS3AclType
are used without importing Enum directly.
*/ | ||
package io.prestosql.plugin.hive.s3; | ||
|
||
import com.amazonaws.services.s3.model.StorageClass; |
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.
import com.amazonaws.services.s3.model.StorageClass; | |
import com.amazonaws.services.s3.model.StorageClass.Standard; | |
import com.amazonaws.services.s3.model.StorageClass.IntelligentTiering; |
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.
@Praveen2112 These statement below might look confusing if I import the storage classes Standard
and IntelligentTiering
directly
STANDARD(Standard),
INTELLIGENT_TIERING(IntelligentTiering);
Should I still change this?
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.
#2686 (comment) -- ie do not import them
@@ -35,6 +35,7 @@ | |||
private String s3AwsAccessKey; | |||
private String s3AwsSecretKey; | |||
private String s3Endpoint; | |||
private PrestoS3StorageClass s3StorageClass = PrestoS3StorageClass.STANDARD; |
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.
Can we directly import the enum ?
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.
@Praveen2112 Importing Enum directly might look off since PrestoS3SignerType and PrestoS3AclType are used without importing Enum directly.
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.
if static import removes readability, it's should not be used;
here it is OK to static import, except it would be inconsistent. Please keep as is (inconsistency decreases readability)
Merged, thanks! |
Cherry pick of trinodb/trino#3032 Co-authored-by Arpit Solanki <[email protected]>
Cherry pick of trinodb/trino#3032 Co-authored-by Arpit Solanki <[email protected]>
Info on S3 Storage classes: https://aws.amazon.com/s3/storage-classes/
Cleaned up the PR #2686