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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
