You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/03/17 06:01:08 UTC
[GitHub] [iceberg] rajarshisarkar opened a new pull request #4350: AWS: Move S3 writeTags to AwsProperties
rajarshisarkar opened a new pull request #4350:
URL: https://github.com/apache/iceberg/pull/4350
This change moves S3 `writeTags` to `AwsProperties`. This provides a cleaner solution as `AwsProperties` was designed to be the extension point for new configs without changing constructors.
Users can pass their custom tags as part of the catalog properties.
Spark SQL launch command:
```
sh spark-sql --conf spark.sql.catalog.my_catalog=org.apache.iceberg.spark.SparkCatalog \
--conf spark.sql.catalog.my_catalog.warehouse=s3://iceberg-warehouse/s3-tagging \
--conf spark.sql.catalog.my_catalog.catalog-impl=org.apache.iceberg.aws.glue.GlueCatalog \
--conf spark.sql.catalog.my_catalog.io-impl=org.apache.iceberg.aws.s3.S3FileIO \
--conf spark.sql.catalog.my_catalog.s3.write.tags.my_key=my_val \
--conf spark.sql.catalog.my_catalog.s3.write.tags.my_key2=my_val2
```
Tags added in S3:
```
aws s3api get-object-tagging --bucket iceberg-warehouse --key s3-tagging/data/00000-0-7eb83d01-ad82-41ca-814b-c7686d9d9211-00001.parquet
{
"TagSet": [
{
"Key": "my_key2",
"Value": "my_val2"
},
{
"Key": "my_key",
"Value": "my_val"
}
]
}
```
---
cc: @jackye1995 @arminnajafi @singhpk234 @amogh-jahagirdar @xiaoxuandev @yyanyy
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 merged pull request #4350: AWS: Move S3 writeTags to AwsProperties
Posted by GitBox <gi...@apache.org>.
jackye1995 merged pull request #4350:
URL: https://github.com/apache/iceberg/pull/4350
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4350: AWS: Move S3 writeTags to AwsProperties
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4350:
URL: https://github.com/apache/iceberg/pull/4350#discussion_r829188029
##########
File path: aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java
##########
@@ -267,6 +271,8 @@
private boolean isS3ChecksumEnabled;
+ private final Set<Tag> s3WriteTags;
Review comment:
nit: for easier categorization of the feature of each service, can you also move up `s3WriteTags` and `isS3ChecksumEnabled` up so that all the s3 related feature variables are together?
##########
File path: aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java
##########
@@ -339,6 +344,8 @@ public AwsProperties(Map<String, String> properties) {
this.dynamoDbTableName = PropertyUtil.propertyAsString(properties, DYNAMODB_TABLE_NAME,
DYNAMODB_TABLE_NAME_DEFAULT);
+
+ this.s3WriteTags = toTags(properties, S3_WRITE_TAGS_PREFIX);
Review comment:
Nice catch for this! I think we still need to check null, because the object is serialized, and it might serialize an old version of the object that does not have the `writeTags` fields, which means the object will be deserialized with the value equal to null.
Also nit: similar to the above comments, could you move the assignment to L347?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rajarshisarkar commented on a change in pull request #4350: AWS: Move S3 writeTags to AwsProperties
Posted by GitBox <gi...@apache.org>.
rajarshisarkar commented on a change in pull request #4350:
URL: https://github.com/apache/iceberg/pull/4350#discussion_r828892115
##########
File path: aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java
##########
@@ -339,6 +344,8 @@ public AwsProperties(Map<String, String> properties) {
this.dynamoDbTableName = PropertyUtil.propertyAsString(properties, DYNAMODB_TABLE_NAME,
DYNAMODB_TABLE_NAME_DEFAULT);
+
+ this.s3WriteTags = toTags(properties, S3_WRITE_TAGS_PREFIX);
Review comment:
I agree, empty set would be a cleaner solution. I have made the changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] singhpk234 commented on a change in pull request #4350: AWS: Move S3 writeTags to AwsProperties
Posted by GitBox <gi...@apache.org>.
singhpk234 commented on a change in pull request #4350:
URL: https://github.com/apache/iceberg/pull/4350#discussion_r828814860
##########
File path: aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java
##########
@@ -339,6 +344,8 @@ public AwsProperties(Map<String, String> properties) {
this.dynamoDbTableName = PropertyUtil.propertyAsString(properties, DYNAMODB_TABLE_NAME,
DYNAMODB_TABLE_NAME_DEFAULT);
+
+ this.s3WriteTags = toTags(properties, S3_WRITE_TAGS_PREFIX);
Review comment:
Ahh I see you made a check of tags != null below then it might not be required, but I think still empty set as default is good as it avoids us checking the null checks when it's being used. WDYT ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] singhpk234 commented on a change in pull request #4350: AWS: Move S3 writeTags to AwsProperties
Posted by GitBox <gi...@apache.org>.
singhpk234 commented on a change in pull request #4350:
URL: https://github.com/apache/iceberg/pull/4350#discussion_r828814042
##########
File path: aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java
##########
@@ -444,4 +451,15 @@ public boolean isS3ChecksumEnabled() {
public void setS3ChecksumEnabled(boolean eTagCheckEnabled) {
this.isS3ChecksumEnabled = eTagCheckEnabled;
}
+
+ public Set<Tag> getS3WriteTags() {
+ return s3WriteTags;
+ }
+
+ public Set<Tag> toTags(Map<String, String> properties, String prefix) {
+ return PropertyUtil.propertiesWithPrefix(properties, prefix)
+ .entrySet().stream()
+ .map(e -> Tag.builder().key(e.getKey()).value(e.getValue()).build())
+ .collect(Collectors.toSet());
+ }
Review comment:
[question] any reason why we made it public, as earlier it was private
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4350: AWS: Move S3 writeTags to AwsProperties
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4350:
URL: https://github.com/apache/iceberg/pull/4350#discussion_r828791860
##########
File path: aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java
##########
@@ -267,6 +270,8 @@
private boolean isS3ChecksumEnabled;
+ private Set<Tag> writeTags;
Review comment:
prefer to prefix with service name, `s3WriteTags`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] singhpk234 commented on a change in pull request #4350: AWS: Move S3 writeTags to AwsProperties
Posted by GitBox <gi...@apache.org>.
singhpk234 commented on a change in pull request #4350:
URL: https://github.com/apache/iceberg/pull/4350#discussion_r828813436
##########
File path: aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java
##########
@@ -339,6 +344,8 @@ public AwsProperties(Map<String, String> properties) {
this.dynamoDbTableName = PropertyUtil.propertyAsString(properties, DYNAMODB_TABLE_NAME,
DYNAMODB_TABLE_NAME_DEFAULT);
+
+ this.s3WriteTags = toTags(properties, S3_WRITE_TAGS_PREFIX);
Review comment:
we should also initialize the s3WriteTags value in the constructor above :
https://github.com/apache/iceberg/pull/4350/files#diff-19af8af4e4204814f89c7e71633aae4b635229472dd20772d7651044d20cc900R291
```java
this.s3WriteTags = Sets.newHashSet();
```
otherwise the people using the constructor will get NPE.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 merged pull request #4350: AWS: Move S3 writeTags to AwsProperties
Posted by GitBox <gi...@apache.org>.
jackye1995 merged pull request #4350:
URL: https://github.com/apache/iceberg/pull/4350
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on pull request #4350: AWS: Move S3 writeTags to AwsProperties
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #4350:
URL: https://github.com/apache/iceberg/pull/4350#issuecomment-1070370448
Thanks, this was based on the discussion in https://github.com/apache/iceberg/pull/4334/files#r828411959 for some context
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rajarshisarkar commented on a change in pull request #4350: AWS: Move S3 writeTags to AwsProperties
Posted by GitBox <gi...@apache.org>.
rajarshisarkar commented on a change in pull request #4350:
URL: https://github.com/apache/iceberg/pull/4350#discussion_r828895200
##########
File path: aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java
##########
@@ -444,4 +451,15 @@ public boolean isS3ChecksumEnabled() {
public void setS3ChecksumEnabled(boolean eTagCheckEnabled) {
this.isS3ChecksumEnabled = eTagCheckEnabled;
}
+
+ public Set<Tag> getS3WriteTags() {
+ return s3WriteTags;
+ }
+
+ public Set<Tag> toTags(Map<String, String> properties, String prefix) {
+ return PropertyUtil.propertiesWithPrefix(properties, prefix)
+ .entrySet().stream()
+ .map(e -> Tag.builder().key(e.getKey()).value(e.getValue()).build())
+ .collect(Collectors.toSet());
+ }
Review comment:
It can be private here (unlike other methods of `AwsProperties` which are public). I don't see a scope of `toTags` method to be used by other classes. We can make it public later on, if required.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4350: AWS: Move S3 writeTags to AwsProperties
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4350:
URL: https://github.com/apache/iceberg/pull/4350#discussion_r829188029
##########
File path: aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java
##########
@@ -267,6 +271,8 @@
private boolean isS3ChecksumEnabled;
+ private final Set<Tag> s3WriteTags;
Review comment:
nit: for easier categorization of the feature of each service, can you also move up `s3WriteTags` and `isS3ChecksumEnabled` up so that all the s3 related feature variables are together?
##########
File path: aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java
##########
@@ -339,6 +344,8 @@ public AwsProperties(Map<String, String> properties) {
this.dynamoDbTableName = PropertyUtil.propertyAsString(properties, DYNAMODB_TABLE_NAME,
DYNAMODB_TABLE_NAME_DEFAULT);
+
+ this.s3WriteTags = toTags(properties, S3_WRITE_TAGS_PREFIX);
Review comment:
Nice catch for this! I think we still need to check null, because the object is serialized, and it might serialize an old version of the object that does not have the `writeTags` fields, which means the object will be deserialized with the value equal to null.
Also nit: similar to the above comments, could you move the assignment to L347?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org