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/03 12:10:59 UTC
[GitHub] [iceberg] rajarshisarkar opened a new pull request #4259: AWS: Add S3 Tagging
rajarshisarkar opened a new pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259
Draft PR to add [S3 Tags](https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-tagging.html).
_PR status: Work in progress_
--
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 #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r825025513
##########
File path: core/src/main/java/org/apache/iceberg/util/PropertyUtil.java
##########
@@ -70,4 +73,26 @@ public static String propertyAsString(Map<String, String> properties,
}
return defaultValue;
}
+
+ /**
+ * Returns subset of provided map with keys matching the provided prefix. Matching is case-sensitive and the matching
+ * prefix is removed from the keys in returned map.
+ *
+ * @param properties input map
+ * @param prefix prefix to choose keys from input map
+ * @return subset of input map with keys starting with provided prefix and prefix trimmed out
+ */
+ public static Map<String, String> propertiesWithPrefix(Map<String, String> properties,
Review comment:
prefer method arguments all on next lines of it has to span multiple lines, e.g.
```java
propertiesWithPrefix(
Map<String, String> properties,
String prefix) {
```
##########
File path: core/src/main/java/org/apache/iceberg/util/PropertyUtil.java
##########
@@ -70,4 +73,26 @@ public static String propertyAsString(Map<String, String> properties,
}
return defaultValue;
}
+
+ /**
+ * Returns subset of provided map with keys matching the provided prefix. Matching is case-sensitive and the matching
+ * prefix is removed from the keys in returned map.
+ *
+ * @param properties input map
+ * @param prefix prefix to choose keys from input map
+ * @return subset of input map with keys starting with provided prefix and prefix trimmed out
+ */
+ public static Map<String, String> propertiesWithPrefix(Map<String, String> properties,
Review comment:
nit: prefer method arguments all on next lines of it has to span multiple lines, e.g.
```java
propertiesWithPrefix(
Map<String, String> properties,
String prefix) {
```
--
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 edited a comment on pull request #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
rajarshisarkar edited a comment on pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#issuecomment-1062707574
I took the idea of parsing the properties beginning with a prefix from https://github.com/apache/iceberg/pull/4011
Refer method name: `PropertyUtil.propertiesWithPrefix`
--
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 #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r825081305
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
##########
@@ -110,6 +115,8 @@ private S3Client client() {
@Override
public void initialize(Map<String, String> properties) {
this.awsProperties = new AwsProperties(properties);
+ this.writeTags = toTags(
Review comment:
nit: I think we can combine this line into the helper method, that makes the purpose of the helper method clearer, like `this.writeTags = toTags(properties)`, and make the tag map extraction a part of the helper.
--
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 edited a comment on pull request #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
rajarshisarkar edited a comment on pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#issuecomment-1062707574
I took the idea of parsing the properties beginning with a prefix from https://github.com/apache/iceberg/pull/4011 (method name: `PropertyUtil.propertiesWithPrefix`)
--
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 #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r825082533
##########
File path: aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java
##########
@@ -215,6 +216,14 @@
*/
public static final String CLIENT_ASSUME_ROLE_REGION = "client.assume-role.region";
+ /**
+ * Used by {@link S3FileIO} to tag objects when writing. To set, we can pass a catalog property.
+ * Example in Spark: --conf spark.sql.catalog.my_catalog.s3.write.tags.my_key=my_val
Review comment:
nit: example should also be below <p> not above
--
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 #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
rajarshisarkar commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r823565098
##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java
##########
@@ -162,7 +173,28 @@ private String cleanWarehousePath(String path) {
@Override
protected TableOperations newTableOps(TableIdentifier tableIdentifier) {
- return new GlueTableOperations(glue, lockManager, catalogName, awsProperties, fileIO, tableIdentifier);
+ if (isS3FileIOConfigured()) {
+ // Create different S3FileIO to use different credentials in future
Review comment:
We have moved away from this approach in the current implementation.
##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java
##########
@@ -162,7 +173,28 @@ private String cleanWarehousePath(String path) {
@Override
protected TableOperations newTableOps(TableIdentifier tableIdentifier) {
- return new GlueTableOperations(glue, lockManager, catalogName, awsProperties, fileIO, tableIdentifier);
+ if (isS3FileIOConfigured()) {
+ // Create different S3FileIO to use different credentials in future
Review comment:
We have moved away from this approach in the current implementation.
--
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 #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r821275302
##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java
##########
@@ -162,7 +173,28 @@ private String cleanWarehousePath(String path) {
@Override
protected TableOperations newTableOps(TableIdentifier tableIdentifier) {
- return new GlueTableOperations(glue, lockManager, catalogName, awsProperties, fileIO, tableIdentifier);
+ if (isS3FileIOConfigured()) {
+ // Create different S3FileIO to use different credentials in future
+ FileIO s3FileIO = new S3FileIO(toTags(tableIdentifier));
+ s3FileIO.initialize(this.catalogProperties);
+ closeableGroup.addCloseable(s3FileIO);
+ return new GlueTableOperations(glue, lockManager, catalogName, awsProperties, s3FileIO, tableIdentifier);
+ } else {
+ return new GlueTableOperations(glue, lockManager, catalogName, awsProperties, fileIO, tableIdentifier);
+ }
+ }
+
+ private boolean isS3FileIOConfigured() {
Review comment:
having a specific case for S3FileIO should not be needed in a generic solution. `GlueCatalog` can just pass in the properties list to S3FileIO, and S3FileIO tries to parse any properties begin with `s3.write.tags` to gather all the tags needed.
In the future, we might want to initialize a different S3FileIO for each `newTableOps` to have table-specific tags, but I think that's an enhancement we can do later, it would be much cleaner if we first offer the generic solution.
--
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 #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
rajarshisarkar commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r819443189
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3Tag.java
##########
@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.aws.s3;
+
+import java.util.Collection;
+import java.util.Map;
+import org.apache.iceberg.CatalogProperties;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import software.amazon.awssdk.services.s3.model.Tag;
+
+public class S3Tag {
Review comment:
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] rajarshisarkar edited a comment on pull request #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
rajarshisarkar edited a comment on pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#issuecomment-1062707574
I took the idea of parsing the properties beginning with a prefix from https://github.com/apache/iceberg/pull/4011 (`PropertyUtil.propertiesWithPrefix`)
--
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 #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r825087229
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3OutputFile.java
##########
@@ -21,22 +21,24 @@
import java.io.IOException;
import java.io.UncheckedIOException;
+import java.util.Set;
import org.apache.iceberg.aws.AwsProperties;
import org.apache.iceberg.exceptions.AlreadyExistsException;
import org.apache.iceberg.io.InputFile;
import org.apache.iceberg.io.OutputFile;
import org.apache.iceberg.io.PositionOutputStream;
import org.apache.iceberg.metrics.MetricsContext;
import software.amazon.awssdk.services.s3.S3Client;
+import software.amazon.awssdk.services.s3.model.Tag;
public class S3OutputFile extends BaseS3File implements OutputFile {
public static S3OutputFile fromLocation(String location, S3Client client, AwsProperties awsProperties,
- MetricsContext metrics) {
- return new S3OutputFile(client, new S3URI(location), awsProperties, metrics);
+ MetricsContext metrics, Set<Tag> writeTags) {
Review comment:
public method signature should be be changed, we need to add a separated method
--
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 #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
rajarshisarkar commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r823570975
##########
File path: aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java
##########
@@ -85,6 +87,8 @@
private final Random random = new Random(1);
private final Path tmpDir = Files.createTempDirectory("s3fileio-test-");
private final String newTmpDirectory = "/tmp/newStagingDirectory";
+ private final List<Tag> tags = ImmutableList.of(
+ Tag.builder().key("abc").value("123").build());
Review comment:
Request will be rejected in case we have duplicate keys: https://docs.aws.amazon.com/awsaccountbilling/latest/aboutv2/allocation-tag-restrictions.html
Though, we are not supposed to have duplicate keys as the catalog `properties` map maintains uniqueness. So, if the user passes:
```
--conf spark.sql.catalog.my_catalog.s3.write.tags.my_key=my_val \
--conf spark.sql.catalog.my_catalog.s3.write.tags.my_key=my_val2
```
then the `properties` map would store only `s3.write.tags.my_key=my_val2` which is later parsed to extract `my_key=my_val2` and then converted to `writeTags`.
--
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 #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
rajarshisarkar commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r819442847
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3Tag.java
##########
@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.aws.s3;
+
+import java.util.Collection;
+import java.util.Map;
+import org.apache.iceberg.CatalogProperties;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import software.amazon.awssdk.services.s3.model.Tag;
+
+public class S3Tag {
+
+ private static final String S3_FILE_IO_IMPL = "org.apache.iceberg.aws.s3.S3FileIO";
+
+ public S3Tag() {
+ }
+
+ public boolean isS3FileIOConfigured(Map<String, String> catalogProperties) {
+ String fileIOImpl = catalogProperties.get(CatalogProperties.FILE_IO_IMPL);
+ return fileIOImpl == null || fileIOImpl.equals(S3_FILE_IO_IMPL);
+ }
+
+ public Collection<Tag> toTags(TableIdentifier tableIdentifier) {
Review comment:
Sounds good.
--
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 #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
rajarshisarkar commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r819443039
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
##########
@@ -50,6 +52,7 @@
private transient S3Client client;
private MetricsContext metrics = MetricsContext.nullMetrics();
private final AtomicBoolean isResourceClosed = new AtomicBoolean(false);
+ private Collection<Tag> s3Tags;
Review comment:
Changed it to `List<Tag>`.
--
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 pull request #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
rajarshisarkar commented on pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#issuecomment-1062707574
I took the idea of parsing the properties beginning with a prefix from https://github.com/apache/iceberg/pull/4011
--
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 #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r825087229
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3OutputFile.java
##########
@@ -21,22 +21,24 @@
import java.io.IOException;
import java.io.UncheckedIOException;
+import java.util.Set;
import org.apache.iceberg.aws.AwsProperties;
import org.apache.iceberg.exceptions.AlreadyExistsException;
import org.apache.iceberg.io.InputFile;
import org.apache.iceberg.io.OutputFile;
import org.apache.iceberg.io.PositionOutputStream;
import org.apache.iceberg.metrics.MetricsContext;
import software.amazon.awssdk.services.s3.S3Client;
+import software.amazon.awssdk.services.s3.model.Tag;
public class S3OutputFile extends BaseS3File implements OutputFile {
public static S3OutputFile fromLocation(String location, S3Client client, AwsProperties awsProperties,
- MetricsContext metrics) {
- return new S3OutputFile(client, new S3URI(location), awsProperties, metrics);
+ MetricsContext metrics, Set<Tag> writeTags) {
Review comment:
public method should be be changed, we need to add a separated method
--
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 #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r825079374
##########
File path: aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java
##########
@@ -116,7 +122,8 @@ public void testWrite() {
public void testAbortAfterFailedPartUpload() {
doThrow(new RuntimeException()).when(s3mock).uploadPart((UploadPartRequest) any(), (RequestBody) any());
- try (S3OutputStream stream = new S3OutputStream(s3mock, randomURI(), properties, nullMetrics())) {
+ try (S3OutputStream stream = new S3OutputStream(s3mock, randomURI(), properties, nullMetrics(),
Review comment:
nit: prefer method arguments all on next lines of it has to span multiple lines, e.g.
```
try (S3OutputStream stream = new S3OutputStream(
s3mock, randomURI(), properties, nullMetrics(), tags)) {
```
--
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 #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
jackye1995 merged pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259
--
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] amogh-jahagirdar commented on a change in pull request #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
amogh-jahagirdar commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r820920157
##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java
##########
@@ -162,7 +173,28 @@ private String cleanWarehousePath(String path) {
@Override
protected TableOperations newTableOps(TableIdentifier tableIdentifier) {
- return new GlueTableOperations(glue, lockManager, catalogName, awsProperties, fileIO, tableIdentifier);
+ if (isS3FileIOConfigured()) {
+ // Create different S3FileIO to use different credentials in future
+ FileIO s3FileIO = new S3FileIO(toTags(tableIdentifier));
+ s3FileIO.initialize(this.catalogProperties);
+ closeableGroup.addCloseable(s3FileIO);
+ return new GlueTableOperations(glue, lockManager, catalogName, awsProperties, s3FileIO, tableIdentifier);
+ } else {
+ return new GlueTableOperations(glue, lockManager, catalogName, awsProperties, fileIO, tableIdentifier);
+ }
+ }
+
+ private boolean isS3FileIOConfigured() {
+ String fileIOImpl = this.catalogProperties.get(CatalogProperties.FILE_IO_IMPL);
Review comment:
Nit:"this" is not needed.
--
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 #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r825087229
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3OutputFile.java
##########
@@ -21,22 +21,24 @@
import java.io.IOException;
import java.io.UncheckedIOException;
+import java.util.Set;
import org.apache.iceberg.aws.AwsProperties;
import org.apache.iceberg.exceptions.AlreadyExistsException;
import org.apache.iceberg.io.InputFile;
import org.apache.iceberg.io.OutputFile;
import org.apache.iceberg.io.PositionOutputStream;
import org.apache.iceberg.metrics.MetricsContext;
import software.amazon.awssdk.services.s3.S3Client;
+import software.amazon.awssdk.services.s3.model.Tag;
public class S3OutputFile extends BaseS3File implements OutputFile {
public static S3OutputFile fromLocation(String location, S3Client client, AwsProperties awsProperties,
- MetricsContext metrics) {
- return new S3OutputFile(client, new S3URI(location), awsProperties, metrics);
+ MetricsContext metrics, Set<Tag> writeTags) {
Review comment:
public method signature should not be changed, we need to add a separated method
--
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 #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
rajarshisarkar commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r823564732
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/BaseS3File.java
##########
@@ -19,20 +19,23 @@
package org.apache.iceberg.aws.s3;
+import java.util.List;
import org.apache.iceberg.aws.AwsProperties;
import org.apache.iceberg.metrics.MetricsContext;
import software.amazon.awssdk.http.HttpStatusCode;
import software.amazon.awssdk.services.s3.S3Client;
import software.amazon.awssdk.services.s3.model.HeadObjectRequest;
import software.amazon.awssdk.services.s3.model.HeadObjectResponse;
import software.amazon.awssdk.services.s3.model.S3Exception;
+import software.amazon.awssdk.services.s3.model.Tag;
abstract class BaseS3File {
private final S3Client client;
private final S3URI uri;
private final AwsProperties awsProperties;
private HeadObjectResponse metadata;
private final MetricsContext metrics;
+ private List<Tag> tags;
Review comment:
I have renamed to `writeTags`.
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/BaseS3File.java
##########
@@ -19,20 +19,23 @@
package org.apache.iceberg.aws.s3;
+import java.util.List;
import org.apache.iceberg.aws.AwsProperties;
import org.apache.iceberg.metrics.MetricsContext;
import software.amazon.awssdk.http.HttpStatusCode;
import software.amazon.awssdk.services.s3.S3Client;
import software.amazon.awssdk.services.s3.model.HeadObjectRequest;
import software.amazon.awssdk.services.s3.model.HeadObjectResponse;
import software.amazon.awssdk.services.s3.model.S3Exception;
+import software.amazon.awssdk.services.s3.model.Tag;
abstract class BaseS3File {
private final S3Client client;
private final S3URI uri;
private final AwsProperties awsProperties;
private HeadObjectResponse metadata;
private final MetricsContext metrics;
+ private List<Tag> tags;
Review comment:
I have renamed to `writeTags`.
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
##########
@@ -50,6 +54,7 @@
private transient S3Client client;
private MetricsContext metrics = MetricsContext.nullMetrics();
private final AtomicBoolean isResourceClosed = new AtomicBoolean(false);
+ private List<Tag> tags;
Review comment:
Used a `Set`. Yes, object hashcode is implemented. Also, I have renamed to `writeTags`.
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
##########
@@ -50,6 +54,7 @@
private transient S3Client client;
private MetricsContext metrics = MetricsContext.nullMetrics();
private final AtomicBoolean isResourceClosed = new AtomicBoolean(false);
+ private List<Tag> tags;
Review comment:
Used a `Set`. Yes, object hashcode is implemented. Also, I have renamed to `writeTags`.
--
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 #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r823021154
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/BaseS3File.java
##########
@@ -19,20 +19,23 @@
package org.apache.iceberg.aws.s3;
+import java.util.List;
import org.apache.iceberg.aws.AwsProperties;
import org.apache.iceberg.metrics.MetricsContext;
import software.amazon.awssdk.http.HttpStatusCode;
import software.amazon.awssdk.services.s3.S3Client;
import software.amazon.awssdk.services.s3.model.HeadObjectRequest;
import software.amazon.awssdk.services.s3.model.HeadObjectResponse;
import software.amazon.awssdk.services.s3.model.S3Exception;
+import software.amazon.awssdk.services.s3.model.Tag;
abstract class BaseS3File {
private final S3Client client;
private final S3URI uri;
private final AwsProperties awsProperties;
private HeadObjectResponse metadata;
private final MetricsContext metrics;
+ private List<Tag> tags;
Review comment:
private final, also should be named `writeTags`
##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java
##########
@@ -124,8 +125,8 @@ private LockManager initializeLockManager(Map<String, String> properties) {
private FileIO initializeFileIO(Map<String, String> properties) {
String fileIOImpl = properties.get(CatalogProperties.FILE_IO_IMPL);
- if (fileIOImpl == null) {
- FileIO io = new S3FileIO();
+ if (fileIOImpl == null || fileIOImpl.equals(S3_FILE_IO_IMPL)) {
+ FileIO io = new S3FileIO(properties);
Review comment:
I don't think you need to do this, properties will be passed in during `io.initialize(properties);` at next line anyway
##########
File path: aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java
##########
@@ -215,6 +216,11 @@
*/
public static final String CLIENT_ASSUME_ROLE_REGION = "client.assume-role.region";
+ /**
+ * S3 Tag prefix used by {@link S3FileIO}
Review comment:
can be more descriptive, we need to at least say this is used to tag objects when writing. Also would be great to provide:
1. an example of how to set write tags using prefix
2. a link to S3 tagging documentation
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
##########
@@ -50,6 +54,7 @@
private transient S3Client client;
private MetricsContext metrics = MetricsContext.nullMetrics();
private final AtomicBoolean isResourceClosed = new AtomicBoolean(false);
+ private List<Tag> tags;
Review comment:
should this be a set instead? Could you check if the object hashcode is implemented?
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
##########
@@ -81,14 +86,38 @@ public S3FileIO(SerializableSupplier<S3Client> s3, AwsProperties awsProperties)
this.awsProperties = awsProperties;
}
+ /**
+ * Constructor with custom s3 supplier and s3 tags
+ * <p>
+ * Calling {@link S3FileIO#initialize(Map)} will overwrite information set in this constructor.
+ * param s3 s3 supplier
+ * @param properties catalog properties
+ */
+ public S3FileIO(SerializableSupplier<S3Client> s3, Map<String, String> properties) {
Review comment:
the logic should be moved to `initialize`
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
##########
@@ -50,6 +54,7 @@
private transient S3Client client;
private MetricsContext metrics = MetricsContext.nullMetrics();
private final AtomicBoolean isResourceClosed = new AtomicBoolean(false);
+ private List<Tag> tags;
Review comment:
also similar comment, should be named `writeTags`
--
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 #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r819257065
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
##########
@@ -50,6 +52,7 @@
private transient S3Client client;
private MetricsContext metrics = MetricsContext.nullMetrics();
private final AtomicBoolean isResourceClosed = new AtomicBoolean(false);
+ private Collection<Tag> s3Tags;
Review comment:
things in S3FileIO must be serializable with stable serial ID, can you check if this is? If not, prefer to use simple data structure like `Map<String, String>` for this
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3Tag.java
##########
@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.aws.s3;
+
+import java.util.Collection;
+import java.util.Map;
+import org.apache.iceberg.CatalogProperties;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import software.amazon.awssdk.services.s3.model.Tag;
+
+public class S3Tag {
+
+ private static final String S3_FILE_IO_IMPL = "org.apache.iceberg.aws.s3.S3FileIO";
+
+ public S3Tag() {
+ }
+
+ public boolean isS3FileIOConfigured(Map<String, String> catalogProperties) {
+ String fileIOImpl = catalogProperties.get(CatalogProperties.FILE_IO_IMPL);
+ return fileIOImpl == null || fileIOImpl.equals(S3_FILE_IO_IMPL);
+ }
+
+ public Collection<Tag> toTags(TableIdentifier tableIdentifier) {
Review comment:
I think we can hold on this feature later, let's first add `s3.tags.<key>=<val>` as a generic way to specify s3 tags, and in a subsequent PR we can add `s3.table-tags-enabled`
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
##########
@@ -50,6 +52,7 @@
private transient S3Client client;
private MetricsContext metrics = MetricsContext.nullMetrics();
private final AtomicBoolean isResourceClosed = new AtomicBoolean(false);
+ private Collection<Tag> s3Tags;
Review comment:
variable name can just be `tags`
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3Tag.java
##########
@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.aws.s3;
+
+import java.util.Collection;
+import java.util.Map;
+import org.apache.iceberg.CatalogProperties;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import software.amazon.awssdk.services.s3.model.Tag;
+
+public class S3Tag {
Review comment:
the logic here can just be moved to Glue catalog, I don' t think it's worth adding a new class for it
--
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 #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
singhpk234 commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r820853651
##########
File path: aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java
##########
@@ -85,6 +87,8 @@
private final Random random = new Random(1);
private final Path tmpDir = Files.createTempDirectory("s3fileio-test-");
private final String newTmpDirectory = "/tmp/newStagingDirectory";
+ private final List<Tag> tags = ImmutableList.of(
+ Tag.builder().key("abc").value("123").build());
Review comment:
[question] what would be the behaviour when
```
private final List<Tag> tags = ImmutableList.of(
Tag.builder().key("abc").value("123").build(),
Tag.builder().key("abc").value("xyz").build());
```
- does for tag abc we will have xyz as value or 123 or the request will be rejected ?
- Do we require to maintain uniqueness at Tag Key level if yes then List<Tag> might not be a good DS.
Are these scenarios handled via :
```java
if (!tags.isEmpty()) {
requestBuilder.tagging(Tagging.builder().tagSet(tags).build());
}
```
--
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 #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
rajarshisarkar commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r823622682
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
##########
@@ -81,14 +86,38 @@ public S3FileIO(SerializableSupplier<S3Client> s3, AwsProperties awsProperties)
this.awsProperties = awsProperties;
}
+ /**
+ * Constructor with custom s3 supplier and s3 tags
+ * <p>
+ * Calling {@link S3FileIO#initialize(Map)} will overwrite information set in this constructor.
+ * param s3 s3 supplier
+ * @param properties catalog properties
+ */
+ public S3FileIO(SerializableSupplier<S3Client> s3, Map<String, String> properties) {
Review comment:
Thanks, 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] jackye1995 commented on a change in pull request #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r821273557
##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java
##########
@@ -73,11 +74,17 @@
import software.amazon.awssdk.services.glue.model.Table;
import software.amazon.awssdk.services.glue.model.TableInput;
import software.amazon.awssdk.services.glue.model.UpdateDatabaseRequest;
+import software.amazon.awssdk.services.s3.model.Tag;
public class GlueCatalog extends BaseMetastoreCatalog
implements Closeable, SupportsNamespaces, Configurable<Configuration> {
private static final Logger LOG = LoggerFactory.getLogger(GlueCatalog.class);
+ private static final String S3_FILE_IO_IMPL = "org.apache.iceberg.aws.s3.S3FileIO";
+ private static final String S3_TAGS_FORMAT = "s3.tags.format";
Review comment:
Let's first provide a generic solution, before adding specific tag keys for users. I think it's better if in `AwsProperties` we introduce the property prefix `s3.write.tags`, so that users can define any free-form prefixes, for example:
```
spark-sql --packages $DEPENDENCIES \
--conf spark.sql.catalog.my_catalog=org.apache.iceberg.spark.SparkCatalog \
--conf spark.sql.catalog.my_catalog.warehouse=s3://my-bucket/my/key/prefix \
--conf spark.sql.catalog.my_catalog.catalog-impl=org.apache.iceberg.aws.glue.GlueCatalog \
--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
```
This can be passed to `GlueCatalog.initialize`, and we can extract the tags `my_key->my_val` and `my_key2->my_val2`, and stored in S3FileIO to be used during a write.
`write` in `s3.write.tags` is useful in case we would like to introduce other tagging, such as `s3.delete.tags`.
--
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] amogh-jahagirdar commented on a change in pull request #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
amogh-jahagirdar commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r821029815
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
##########
@@ -81,14 +84,36 @@ public S3FileIO(SerializableSupplier<S3Client> s3, AwsProperties awsProperties)
this.awsProperties = awsProperties;
}
+ /**
+ * Constructor with custom s3 supplier and s3 tags
+ * <p>
+ * Calling {@link S3FileIO#initialize(Map)} will overwrite information set in this constructor.
+ * param s3 s3 supplier
+ * @param tags s3 tags
+ */
+ public S3FileIO(SerializableSupplier<S3Client> s3, List<Tag> tags) {
+ this(s3, new AwsProperties());
+ this.tags = tags;
+ }
+
+ /**
+ * Constructor with custom s3 tags
+ * <p>
+ * Calling {@link S3FileIO#initialize(Map)} will overwrite information set in this constructor.
+ * @param tags s3 tags
+ */
+ public S3FileIO(List<Tag> tags) {
+ this.tags = tags;
+ }
+
Review comment:
Instead of this would it make sense to have 1 constructor with 3 args: s3, awsProperties, and tags? In the existing constructors we would pass an empty list for the tags.
--
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] amogh-jahagirdar commented on a change in pull request #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
amogh-jahagirdar commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r820919594
##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java
##########
@@ -162,7 +173,28 @@ private String cleanWarehousePath(String path) {
@Override
protected TableOperations newTableOps(TableIdentifier tableIdentifier) {
- return new GlueTableOperations(glue, lockManager, catalogName, awsProperties, fileIO, tableIdentifier);
+ if (isS3FileIOConfigured()) {
+ // Create different S3FileIO to use different credentials in future
+ FileIO s3FileIO = new S3FileIO(toTags(tableIdentifier));
+ s3FileIO.initialize(this.catalogProperties);
Review comment:
Nit: I think the practice in ICeberg is to only use this when actually mutating the field, not needed when reading.
--
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 #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
singhpk234 commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r820844515
##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java
##########
@@ -162,7 +173,28 @@ private String cleanWarehousePath(String path) {
@Override
protected TableOperations newTableOps(TableIdentifier tableIdentifier) {
- return new GlueTableOperations(glue, lockManager, catalogName, awsProperties, fileIO, tableIdentifier);
+ if (isS3FileIOConfigured()) {
+ // Create different S3FileIO to use different credentials in future
Review comment:
[minor] can we elaborate more as to why we need to create new S3FileO obj . (As you stated this would help in picking up most up to date creds)
##########
File path: aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java
##########
@@ -85,6 +87,8 @@
private final Random random = new Random(1);
private final Path tmpDir = Files.createTempDirectory("s3fileio-test-");
private final String newTmpDirectory = "/tmp/newStagingDirectory";
+ private final List<Tag> tags = ImmutableList.of(
+ Tag.builder().key("abc").value("123").build());
Review comment:
[question] what would be the behaviour when
```
private final List<Tag> tags = ImmutableList.of(
Tag.builder().key("abc").value("123").build(),
Tag.builder().key("abc").value("xyz").build());
```
- does for tag abc we will have xyz as value or 123 or the request will be rejected ?
- Do we require to maintain uniqueness at Tag Key level if yes then List<Tag> might not be a good DS.
##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java
##########
@@ -162,7 +173,28 @@ private String cleanWarehousePath(String path) {
@Override
protected TableOperations newTableOps(TableIdentifier tableIdentifier) {
- return new GlueTableOperations(glue, lockManager, catalogName, awsProperties, fileIO, tableIdentifier);
+ if (isS3FileIOConfigured()) {
+ // Create different S3FileIO to use different credentials in future
+ FileIO s3FileIO = new S3FileIO(toTags(tableIdentifier));
+ s3FileIO.initialize(this.catalogProperties);
+ closeableGroup.addCloseable(s3FileIO);
+ return new GlueTableOperations(glue, lockManager, catalogName, awsProperties, s3FileIO, tableIdentifier);
+ } else {
+ return new GlueTableOperations(glue, lockManager, catalogName, awsProperties, fileIO, tableIdentifier);
+ }
+ }
+
+ private boolean isS3FileIOConfigured() {
+ String fileIOImpl = this.catalogProperties.get(CatalogProperties.FILE_IO_IMPL);
+ return fileIOImpl == null || fileIOImpl.equals(S3_FILE_IO_IMPL);
Review comment:
[question] can we unify this logic with FileIO initialization ? my rationale is what if we decide to update the behaviour when fileIOImpl is null and we don't use S3FileIOImp instead.
--
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 #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
rajarshisarkar commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r823563782
##########
File path: aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java
##########
@@ -215,6 +216,11 @@
*/
public static final String CLIENT_ASSUME_ROLE_REGION = "client.assume-role.region";
+ /**
+ * S3 Tag prefix used by {@link S3FileIO}
Review comment:
Thanks, I have made the changes.
##########
File path: aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java
##########
@@ -215,6 +216,11 @@
*/
public static final String CLIENT_ASSUME_ROLE_REGION = "client.assume-role.region";
+ /**
+ * S3 Tag prefix used by {@link S3FileIO}
Review comment:
Thanks, 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] jackye1995 commented on a change in pull request #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r825024834
##########
File path: core/src/main/java/org/apache/iceberg/util/PropertyUtil.java
##########
@@ -70,4 +73,26 @@ public static String propertyAsString(Map<String, String> properties,
}
return defaultValue;
}
+
+ /**
+ * Returns subset of provided map with keys matching the provided prefix. Matching is case-sensitive and the matching
+ * prefix is removed from the keys in returned map.
+ *
+ * @param properties input map
+ * @param prefix prefix to choose keys from input map
+ * @return subset of input map with keys starting with provided prefix and prefix trimmed out
+ */
+ public static Map<String, String> propertiesWithPrefix(Map<String, String> properties,
+ String prefix) {
+ if (properties == null || properties.isEmpty()) {
+ return ImmutableMap.of();
+ }
+
+ Preconditions.checkState(prefix != null, "prefix can't be null.");
Review comment:
nit: prefer to use `checkArgument`
--
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 #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
rajarshisarkar commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r822419462
##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java
##########
@@ -73,11 +74,17 @@
import software.amazon.awssdk.services.glue.model.Table;
import software.amazon.awssdk.services.glue.model.TableInput;
import software.amazon.awssdk.services.glue.model.UpdateDatabaseRequest;
+import software.amazon.awssdk.services.s3.model.Tag;
public class GlueCatalog extends BaseMetastoreCatalog
implements Closeable, SupportsNamespaces, Configurable<Configuration> {
private static final Logger LOG = LoggerFactory.getLogger(GlueCatalog.class);
+ private static final String S3_FILE_IO_IMPL = "org.apache.iceberg.aws.s3.S3FileIO";
+ private static final String S3_TAGS_FORMAT = "s3.tags.format";
+ private static final String S3_TAGS_NAMESPACE = "s3.tags.namespace";
+ private static final String S3_TAGS_TABLE = "s3.tags.table";
Review comment:
Agreed, 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] rajarshisarkar commented on a change in pull request #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
rajarshisarkar commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r822419222
##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java
##########
@@ -73,11 +74,17 @@
import software.amazon.awssdk.services.glue.model.Table;
import software.amazon.awssdk.services.glue.model.TableInput;
import software.amazon.awssdk.services.glue.model.UpdateDatabaseRequest;
+import software.amazon.awssdk.services.s3.model.Tag;
public class GlueCatalog extends BaseMetastoreCatalog
implements Closeable, SupportsNamespaces, Configurable<Configuration> {
private static final Logger LOG = LoggerFactory.getLogger(GlueCatalog.class);
+ private static final String S3_FILE_IO_IMPL = "org.apache.iceberg.aws.s3.S3FileIO";
+ private static final String S3_TAGS_FORMAT = "s3.tags.format";
Review comment:
This makes sense, I have made the changes to allow users to define any free-form prefixes.
--
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 #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r821274134
##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java
##########
@@ -73,11 +74,17 @@
import software.amazon.awssdk.services.glue.model.Table;
import software.amazon.awssdk.services.glue.model.TableInput;
import software.amazon.awssdk.services.glue.model.UpdateDatabaseRequest;
+import software.amazon.awssdk.services.s3.model.Tag;
public class GlueCatalog extends BaseMetastoreCatalog
implements Closeable, SupportsNamespaces, Configurable<Configuration> {
private static final Logger LOG = LoggerFactory.getLogger(GlueCatalog.class);
+ private static final String S3_FILE_IO_IMPL = "org.apache.iceberg.aws.s3.S3FileIO";
+ private static final String S3_TAGS_FORMAT = "s3.tags.format";
+ private static final String S3_TAGS_NAMESPACE = "s3.tags.namespace";
+ private static final String S3_TAGS_TABLE = "s3.tags.table";
Review comment:
also adding tags for table and namespace names might not be ideal, because people can rename table and these information could change. With the generic tagging solution described above, we can in the future add tags using some stable Iceberg table properties, such as the Iceberg table UUID.
--
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 #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
rajarshisarkar commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r823564732
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/BaseS3File.java
##########
@@ -19,20 +19,23 @@
package org.apache.iceberg.aws.s3;
+import java.util.List;
import org.apache.iceberg.aws.AwsProperties;
import org.apache.iceberg.metrics.MetricsContext;
import software.amazon.awssdk.http.HttpStatusCode;
import software.amazon.awssdk.services.s3.S3Client;
import software.amazon.awssdk.services.s3.model.HeadObjectRequest;
import software.amazon.awssdk.services.s3.model.HeadObjectResponse;
import software.amazon.awssdk.services.s3.model.S3Exception;
+import software.amazon.awssdk.services.s3.model.Tag;
abstract class BaseS3File {
private final S3Client client;
private final S3URI uri;
private final AwsProperties awsProperties;
private HeadObjectResponse metadata;
private final MetricsContext metrics;
+ private List<Tag> tags;
Review comment:
I have made it private final and renamed it to `writeTags`.
--
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 #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r825079302
##########
File path: aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java
##########
@@ -145,7 +153,8 @@ public void testMultipleClose() throws IOException {
public void testStagingDirectoryCreation() throws IOException {
AwsProperties newStagingDirectoryAwsProperties = new AwsProperties(ImmutableMap.of(
AwsProperties.S3FILEIO_STAGING_DIRECTORY, newTmpDirectory));
- S3OutputStream stream = new S3OutputStream(s3, randomURI(), newStagingDirectoryAwsProperties, nullMetrics());
+ S3OutputStream stream = new S3OutputStream(s3, randomURI(), newStagingDirectoryAwsProperties,
Review comment:
nit: prefer method arguments all on next lines of it has to span multiple lines
##########
File path: aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java
##########
@@ -116,7 +122,8 @@ public void testWrite() {
public void testAbortAfterFailedPartUpload() {
doThrow(new RuntimeException()).when(s3mock).uploadPart((UploadPartRequest) any(), (RequestBody) any());
- try (S3OutputStream stream = new S3OutputStream(s3mock, randomURI(), properties, nullMetrics())) {
+ try (S3OutputStream stream = new S3OutputStream(s3mock, randomURI(), properties, nullMetrics(),
Review comment:
nit: prefer method arguments all on next lines of it has to span multiple lines
##########
File path: aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java
##########
@@ -127,7 +134,8 @@ public void testAbortAfterFailedPartUpload() {
public void testAbortMultipart() {
doThrow(new RuntimeException()).when(s3mock).completeMultipartUpload((CompleteMultipartUploadRequest) any());
- try (S3OutputStream stream = new S3OutputStream(s3mock, randomURI(), properties, nullMetrics())) {
+ try (S3OutputStream stream = new S3OutputStream(s3mock, randomURI(), properties, nullMetrics(),
Review comment:
nit: prefer method arguments all on next lines of it has to span multiple lines
--
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 #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
rajarshisarkar commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r825617163
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3OutputFile.java
##########
@@ -21,22 +21,24 @@
import java.io.IOException;
import java.io.UncheckedIOException;
+import java.util.Set;
import org.apache.iceberg.aws.AwsProperties;
import org.apache.iceberg.exceptions.AlreadyExistsException;
import org.apache.iceberg.io.InputFile;
import org.apache.iceberg.io.OutputFile;
import org.apache.iceberg.io.PositionOutputStream;
import org.apache.iceberg.metrics.MetricsContext;
import software.amazon.awssdk.services.s3.S3Client;
+import software.amazon.awssdk.services.s3.model.Tag;
public class S3OutputFile extends BaseS3File implements OutputFile {
public static S3OutputFile fromLocation(String location, S3Client client, AwsProperties awsProperties,
- MetricsContext metrics) {
- return new S3OutputFile(client, new S3URI(location), awsProperties, metrics);
+ MetricsContext metrics, Set<Tag> writeTags) {
Review comment:
Separated the new method.
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3OutputFile.java
##########
@@ -21,22 +21,24 @@
import java.io.IOException;
import java.io.UncheckedIOException;
+import java.util.Set;
import org.apache.iceberg.aws.AwsProperties;
import org.apache.iceberg.exceptions.AlreadyExistsException;
import org.apache.iceberg.io.InputFile;
import org.apache.iceberg.io.OutputFile;
import org.apache.iceberg.io.PositionOutputStream;
import org.apache.iceberg.metrics.MetricsContext;
import software.amazon.awssdk.services.s3.S3Client;
+import software.amazon.awssdk.services.s3.model.Tag;
public class S3OutputFile extends BaseS3File implements OutputFile {
public static S3OutputFile fromLocation(String location, S3Client client, AwsProperties awsProperties,
- MetricsContext metrics) {
- return new S3OutputFile(client, new S3URI(location), awsProperties, metrics);
+ MetricsContext metrics, Set<Tag> writeTags) {
Review comment:
Separated as new method.
--
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 #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
rajarshisarkar commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r822431372
##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java
##########
@@ -162,7 +173,28 @@ private String cleanWarehousePath(String path) {
@Override
protected TableOperations newTableOps(TableIdentifier tableIdentifier) {
- return new GlueTableOperations(glue, lockManager, catalogName, awsProperties, fileIO, tableIdentifier);
+ if (isS3FileIOConfigured()) {
+ // Create different S3FileIO to use different credentials in future
+ FileIO s3FileIO = new S3FileIO(toTags(tableIdentifier));
+ s3FileIO.initialize(this.catalogProperties);
+ closeableGroup.addCloseable(s3FileIO);
+ return new GlueTableOperations(glue, lockManager, catalogName, awsProperties, s3FileIO, tableIdentifier);
+ } else {
+ return new GlueTableOperations(glue, lockManager, catalogName, awsProperties, fileIO, tableIdentifier);
+ }
+ }
+
+ private boolean isS3FileIOConfigured() {
Review comment:
Sure, I have made the changes accordingly.
--
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 #4259: AWS: Add S3 Tagging
Posted by GitBox <gi...@apache.org>.
rajarshisarkar commented on a change in pull request #4259:
URL: https://github.com/apache/iceberg/pull/4259#discussion_r822418708
##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java
##########
@@ -162,7 +173,28 @@ private String cleanWarehousePath(String path) {
@Override
protected TableOperations newTableOps(TableIdentifier tableIdentifier) {
- return new GlueTableOperations(glue, lockManager, catalogName, awsProperties, fileIO, tableIdentifier);
+ if (isS3FileIOConfigured()) {
+ // Create different S3FileIO to use different credentials in future
+ FileIO s3FileIO = new S3FileIO(toTags(tableIdentifier));
+ s3FileIO.initialize(this.catalogProperties);
Review comment:
Thanks, 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