You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "amogh-jahagirdar (via GitHub)" <gi...@apache.org> on 2024/07/19 01:57:03 UTC
[PR] API, AWS: Add SupportsRecoveryOperations mixin for FileIO and implementation for S3FileIO [iceberg]
amogh-jahagirdar opened a new pull request, #10721:
URL: https://github.com/apache/iceberg/pull/10721
(no comment)
--
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
Re: [PR] AWS: Implement SupportsRecoveryOperations for S3FileIO [iceberg]
Posted by "amoghjaha-db (via GitHub)" <gi...@apache.org>.
amoghjaha-db commented on code in PR #10721:
URL: https://github.com/apache/iceberg/pull/10721#discussion_r1710090471
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java:
##########
@@ -420,4 +427,40 @@ protected void finalize() throws Throwable {
}
}
}
+
+ @Override
+ public boolean recoverFile(String path) {
+ S3URI location = new S3URI(path, s3FileIOProperties.bucketToAccessPointMapping());
+ ListObjectVersionsIterable response =
+ client()
+ .listObjectVersionsPaginator(
+ ListObjectVersionsRequest.builder()
+ .bucket(location.bucket())
+ .prefix(location.key())
+ .build());
+
+ Optional<ObjectVersion> latestVersion =
+ response.versions().stream().max(Comparator.comparing(ObjectVersion::lastModified));
+ return latestVersion.map(version -> recoverObject(version, location.bucket())).orElse(false);
Review Comment:
Yeah that seems like a simple check we can do to avoid unnecessary copies, updated! I didn't go down the route of comparing the latest deletion marker with the version to recover to, because inverting the logic to just verify that the version to recover to is not the latest is simple; and we know that "version to recover" cannot represent a deletion marker due to the guarantees provided by the S3 api.
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java:
##########
@@ -420,4 +427,40 @@ protected void finalize() throws Throwable {
}
}
}
+
+ @Override
+ public boolean recoverFile(String path) {
+ S3URI location = new S3URI(path, s3FileIOProperties.bucketToAccessPointMapping());
+ ListObjectVersionsIterable response =
+ client()
+ .listObjectVersionsPaginator(
+ ListObjectVersionsRequest.builder()
+ .bucket(location.bucket())
+ .prefix(location.key())
+ .build());
+
+ Optional<ObjectVersion> latestVersion =
+ response.versions().stream().max(Comparator.comparing(ObjectVersion::lastModified));
+ return latestVersion.map(version -> recoverObject(version, location.bucket())).orElse(false);
Review Comment:
Yeah that seems like a simple check we can do to avoid unnecessary copies, updated! I didn't go down the route of comparing the latest deletion marker with the version to recover to, because inverting the logic to just verify that the version to recover to is not the latest is simple; and we know that "version to recover" cannot represent a deletion marker due to the guarantees provided by the S3 api.
--
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
Re: [PR] AWS: Implement SupportsRecoveryOperations for S3FileIO [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10721:
URL: https://github.com/apache/iceberg/pull/10721#discussion_r1709931564
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java:
##########
@@ -420,4 +427,40 @@ protected void finalize() throws Throwable {
}
}
}
+
+ @Override
+ public boolean recoverFile(String path) {
+ S3URI location = new S3URI(path, s3FileIOProperties.bucketToAccessPointMapping());
+ ListObjectVersionsIterable response =
+ client()
+ .listObjectVersionsPaginator(
+ ListObjectVersionsRequest.builder()
+ .bucket(location.bucket())
+ .prefix(location.key())
+ .build());
+
+ Optional<ObjectVersion> latestVersion =
+ response.versions().stream().max(Comparator.comparing(ObjectVersion::lastModified));
+ return latestVersion.map(version -> recoverObject(version, location.bucket())).orElse(false);
Review Comment:
I think this might be my poor naming, this latestVersion is the version we want to recover to.
For the "isLatest()" check S3 considers deletion markers .
isLatest will only be true for the deletion marker version in the recovery case.
The version that we want to recover to is not the latest version based on S3's consideration of deletion markers; it's the version just before that which the current check will do based on the last modified of the versions (deletions are not included in response.versions().
I'll call this variable `recoveryVersion` and add an inline comment.
--
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
Re: [PR] AWS: Implement SupportsRecoveryOperations for S3FileIO [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10721:
URL: https://github.com/apache/iceberg/pull/10721#discussion_r1706410889
##########
aws/src/integration/java/org/apache/iceberg/aws/s3/TestS3FileIOIntegration.java:
##########
@@ -106,6 +109,12 @@ public static void beforeClass() {
AwsIntegTestUtil.createAccessPoint(s3Control, accessPointName, bucketName);
AwsIntegTestUtil.createAccessPoint(
crossRegionS3Control, crossRegionAccessPointName, crossRegionBucketName);
+ s3.putBucketVersioning(
+ PutBucketVersioningRequest.builder()
+ .bucket(bucketName)
+ .versioningConfiguration(
+ VersioningConfiguration.builder().status(BucketVersioningStatus.ENABLED).build())
Review Comment:
The simplest way for integration tests seemed to be just to enable bucket versioning for the bucket and then on cleanup, ensure deletion of every object version to make sure no garbage is left over.
--
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
Re: [PR] AWS: Implement SupportsRecoveryOperations for S3FileIO [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #10721:
URL: https://github.com/apache/iceberg/pull/10721#discussion_r1709801097
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java:
##########
@@ -420,4 +427,40 @@ protected void finalize() throws Throwable {
}
}
}
+
+ @Override
+ public boolean recoverFile(String path) {
+ S3URI location = new S3URI(path, s3FileIOProperties.bucketToAccessPointMapping());
+ ListObjectVersionsIterable response =
+ client()
+ .listObjectVersionsPaginator(
+ ListObjectVersionsRequest.builder()
+ .bucket(location.bucket())
+ .prefix(location.key())
+ .build());
+
+ Optional<ObjectVersion> latestVersion =
+ response.versions().stream().max(Comparator.comparing(ObjectVersion::lastModified));
+ return latestVersion.map(version -> recoverObject(version, location.bucket())).orElse(false);
Review Comment:
You might want to put a check here for `version.isLatest()` just to protect against being called on an object this the latest version. It's unlikely, but it could actually recover and older version since there is no direct check here.
--
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
Re: [PR] AWS: Implement SupportsRecoveryOperations for S3FileIO [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #10721:
URL: https://github.com/apache/iceberg/pull/10721#discussion_r1710037881
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java:
##########
@@ -420,4 +427,40 @@ protected void finalize() throws Throwable {
}
}
}
+
+ @Override
+ public boolean recoverFile(String path) {
+ S3URI location = new S3URI(path, s3FileIOProperties.bucketToAccessPointMapping());
+ ListObjectVersionsIterable response =
+ client()
+ .listObjectVersionsPaginator(
+ ListObjectVersionsRequest.builder()
+ .bucket(location.bucket())
+ .prefix(location.key())
+ .build());
+
+ Optional<ObjectVersion> latestVersion =
+ response.versions().stream().max(Comparator.comparing(ObjectVersion::lastModified));
+ return latestVersion.map(version -> recoverObject(version, location.bucket())).orElse(false);
Review Comment:
I think we can skip the recovery (i.e copy) if recoveryVersion.isLatest() is true as well, this will help the case when we are trying to recover an object which is not deleted yet, so it will not have any deletionMarker
--
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
Re: [PR] AWS: Implement SupportsRecoveryOperations for S3FileIO [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #10721:
URL: https://github.com/apache/iceberg/pull/10721#discussion_r1708266721
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java:
##########
@@ -420,4 +427,38 @@ protected void finalize() throws Throwable {
}
}
}
+
+ @Override
+ public boolean recoverFile(String path) {
+ S3URI location = new S3URI(path, s3FileIOProperties.bucketToAccessPointMapping());
+ ListObjectVersionsIterable response =
+ client.listObjectVersionsPaginator(
+ ListObjectVersionsRequest.builder()
+ .bucket(location.bucket())
+ .prefix(location.key())
+ .build());
+
+ Optional<ObjectVersion> latestVersion =
+ response.versions().stream().max(Comparator.comparing(ObjectVersion::lastModified));
+ return latestVersion.map(version -> recoverObject(version, location.bucket())).orElse(false);
+ }
+
+ private boolean recoverObject(ObjectVersion version, String bucket) {
+ LOG.info("Attempting to recover object {}", version.key());
+ try {
+ client.copyObject(
Review Comment:
```suggestion
client().copyObject(
```
can we use `client()` instead of client, as it might be null when serde happens copy to all places
--
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
Re: [PR] AWS: Implement SupportsRecoveryOperations for S3FileIO [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #10721:
URL: https://github.com/apache/iceberg/pull/10721#discussion_r1710151755
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java:
##########
@@ -420,4 +426,46 @@ protected void finalize() throws Throwable {
}
}
}
+
+ @Override
+ public boolean recoverFile(String path) {
+ S3URI location = new S3URI(path, s3FileIOProperties.bucketToAccessPointMapping());
+ ListObjectVersionsIterable response =
+ client()
+ .listObjectVersionsPaginator(
+ builder -> builder.bucket(location.bucket()).prefix(location.key()));
+
+ // Recover to the last modified version, not isLatest,
+ // since isLatest is true for deletion markers.
+ Optional<ObjectVersion> recoverVersion =
+ response.versions().stream().max(Comparator.comparing(ObjectVersion::lastModified));
+
+ return recoverVersion.map(version -> recoverObject(version, location.bucket())).orElse(false);
+ }
+
+ private boolean recoverObject(ObjectVersion version, String bucket) {
+ if (version.isLatest()) {
+ return true;
+ }
Review Comment:
[optional] we can have an integ test trying to restore an existing object doesn't leads to creating a new version of obj
--
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
Re: [PR] AWS: Implement SupportsRecoveryOperations for S3FileIO [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #10721:
URL: https://github.com/apache/iceberg/pull/10721#discussion_r1709765272
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java:
##########
@@ -420,4 +427,40 @@ protected void finalize() throws Throwable {
}
}
}
+
+ @Override
+ public boolean recoverFile(String path) {
+ S3URI location = new S3URI(path, s3FileIOProperties.bucketToAccessPointMapping());
+ ListObjectVersionsIterable response =
+ client()
+ .listObjectVersionsPaginator(
+ ListObjectVersionsRequest.builder()
Review Comment:
nit: we're using a mixture of patterns (builder/request object). Could we just switch this to:
```java
builder -> builder.bucket(location.bucket()).prefix(location.key())
```
like we do below?
--
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
Re: [PR] AWS: Implement SupportsRecoveryOperations for S3FileIO [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #10721:
URL: https://github.com/apache/iceberg/pull/10721#discussion_r1708266721
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java:
##########
@@ -420,4 +427,38 @@ protected void finalize() throws Throwable {
}
}
}
+
+ @Override
+ public boolean recoverFile(String path) {
+ S3URI location = new S3URI(path, s3FileIOProperties.bucketToAccessPointMapping());
+ ListObjectVersionsIterable response =
+ client.listObjectVersionsPaginator(
+ ListObjectVersionsRequest.builder()
+ .bucket(location.bucket())
+ .prefix(location.key())
+ .build());
+
+ Optional<ObjectVersion> latestVersion =
+ response.versions().stream().max(Comparator.comparing(ObjectVersion::lastModified));
+ return latestVersion.map(version -> recoverObject(version, location.bucket())).orElse(false);
+ }
+
+ private boolean recoverObject(ObjectVersion version, String bucket) {
+ LOG.info("Attempting to recover object {}", version.key());
+ try {
+ client.copyObject(
Review Comment:
```suggestion
client().copyObject(
```
we should use `client()` instead of client as it might be null when serde happens copy to all places
--
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
Re: [PR] AWS: Implement SupportsRecoveryOperations for S3FileIO [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #10721:
URL: https://github.com/apache/iceberg/pull/10721#discussion_r1708266721
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java:
##########
@@ -420,4 +427,38 @@ protected void finalize() throws Throwable {
}
}
}
+
+ @Override
+ public boolean recoverFile(String path) {
+ S3URI location = new S3URI(path, s3FileIOProperties.bucketToAccessPointMapping());
+ ListObjectVersionsIterable response =
+ client.listObjectVersionsPaginator(
+ ListObjectVersionsRequest.builder()
+ .bucket(location.bucket())
+ .prefix(location.key())
+ .build());
+
+ Optional<ObjectVersion> latestVersion =
+ response.versions().stream().max(Comparator.comparing(ObjectVersion::lastModified));
+ return latestVersion.map(version -> recoverObject(version, location.bucket())).orElse(false);
+ }
+
+ private boolean recoverObject(ObjectVersion version, String bucket) {
+ LOG.info("Attempting to recover object {}", version.key());
+ try {
+ client.copyObject(
Review Comment:
```suggestion
client().copyObject(
```
can we use `client()` instead of client, as it might be null when serde happens, copy to all places
--
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
Re: [PR] AWS: Implement SupportsRecoveryOperations for S3FileIO [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10721:
URL: https://github.com/apache/iceberg/pull/10721#discussion_r1709931564
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java:
##########
@@ -420,4 +427,40 @@ protected void finalize() throws Throwable {
}
}
}
+
+ @Override
+ public boolean recoverFile(String path) {
+ S3URI location = new S3URI(path, s3FileIOProperties.bucketToAccessPointMapping());
+ ListObjectVersionsIterable response =
+ client()
+ .listObjectVersionsPaginator(
+ ListObjectVersionsRequest.builder()
+ .bucket(location.bucket())
+ .prefix(location.key())
+ .build());
+
+ Optional<ObjectVersion> latestVersion =
+ response.versions().stream().max(Comparator.comparing(ObjectVersion::lastModified));
+ return latestVersion.map(version -> recoverObject(version, location.bucket())).orElse(false);
Review Comment:
I think this might be my poor naming, this latestVersion is the version we want to recover to.
For the "isLatest()" check S3 considers deletion markers .
isLatest will only be true for the deletion marker version.
The version that we want to recover to is not the latest version based on S3's consideration of deletion markers; it's the version just before that which the current check will do based on the last modified of the versions (deletions are not included in response.versions().
I'll call this variable `recoveryVersion` and add an inline comment.
--
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
Re: [PR] AWS: Implement SupportsRecoveryOperations for S3FileIO [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10721:
URL: https://github.com/apache/iceberg/pull/10721#discussion_r1710328535
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java:
##########
@@ -420,4 +426,46 @@ protected void finalize() throws Throwable {
}
}
}
+
+ @Override
+ public boolean recoverFile(String path) {
+ S3URI location = new S3URI(path, s3FileIOProperties.bucketToAccessPointMapping());
+ ListObjectVersionsIterable response =
+ client()
+ .listObjectVersionsPaginator(
+ builder -> builder.bucket(location.bucket()).prefix(location.key()));
+
+ // Recover to the last modified version, not isLatest,
+ // since isLatest is true for deletion markers.
+ Optional<ObjectVersion> recoverVersion =
+ response.versions().stream().max(Comparator.comparing(ObjectVersion::lastModified));
+
+ return recoverVersion.map(version -> recoverObject(version, location.bucket())).orElse(false);
+ }
+
+ private boolean recoverObject(ObjectVersion version, String bucket) {
+ if (version.isLatest()) {
+ return true;
+ }
Review Comment:
Missed this, sure I can do a follow on to verify that we skip the recovery if the latest version is not a deletion marker.
--
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
Re: [PR] AWS: Implement SupportsRecoveryOperations for S3FileIO [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar merged PR #10721:
URL: https://github.com/apache/iceberg/pull/10721
--
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
Re: [PR] AWS: Implement SupportsRecoveryOperations for S3FileIO [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #10721:
URL: https://github.com/apache/iceberg/pull/10721#issuecomment-2272662631
cc @singhpk234 @geruh @rahil-c
--
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
Re: [PR] AWS: Implement SupportsRecoveryOperations for S3FileIO [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10721:
URL: https://github.com/apache/iceberg/pull/10721#discussion_r1708527555
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java:
##########
@@ -420,4 +427,38 @@ protected void finalize() throws Throwable {
}
}
}
+
+ @Override
+ public boolean recoverFile(String path) {
+ S3URI location = new S3URI(path, s3FileIOProperties.bucketToAccessPointMapping());
+ ListObjectVersionsIterable response =
+ client.listObjectVersionsPaginator(
+ ListObjectVersionsRequest.builder()
+ .bucket(location.bucket())
+ .prefix(location.key())
+ .build());
+
+ Optional<ObjectVersion> latestVersion =
+ response.versions().stream().max(Comparator.comparing(ObjectVersion::lastModified));
+ return latestVersion.map(version -> recoverObject(version, location.bucket())).orElse(false);
+ }
+
+ private boolean recoverObject(ObjectVersion version, String bucket) {
+ LOG.info("Attempting to recover object {}", version.key());
+ try {
+ client.copyObject(
Review Comment:
That's a great point, yeah we probably should be using `client()` for distributed use cases.
--
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
Re: [PR] AWS: Implement SupportsRecoveryOperations for S3FileIO [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10721:
URL: https://github.com/apache/iceberg/pull/10721#discussion_r1708529268
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java:
##########
@@ -420,4 +427,38 @@ protected void finalize() throws Throwable {
}
}
}
+
+ @Override
+ public boolean recoverFile(String path) {
+ S3URI location = new S3URI(path, s3FileIOProperties.bucketToAccessPointMapping());
+ ListObjectVersionsIterable response =
+ client.listObjectVersionsPaginator(
+ ListObjectVersionsRequest.builder()
+ .bucket(location.bucket())
+ .prefix(location.key())
+ .build());
+
+ Optional<ObjectVersion> latestVersion =
+ response.versions().stream().max(Comparator.comparing(ObjectVersion::lastModified));
+ return latestVersion.map(version -> recoverObject(version, location.bucket())).orElse(false);
+ }
+
+ private boolean recoverObject(ObjectVersion version, String bucket) {
+ LOG.info("Attempting to recover object {}", version.key());
+ try {
+ client.copyObject(
Review Comment:
Fixed!
--
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
Re: [PR] AWS: Implement SupportsRecoveryOperations for S3FileIO [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #10721:
URL: https://github.com/apache/iceberg/pull/10721#discussion_r1709774374
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java:
##########
@@ -420,4 +427,40 @@ protected void finalize() throws Throwable {
}
}
}
+
+ @Override
+ public boolean recoverFile(String path) {
+ S3URI location = new S3URI(path, s3FileIOProperties.bucketToAccessPointMapping());
+ ListObjectVersionsIterable response =
+ client()
+ .listObjectVersionsPaginator(
+ ListObjectVersionsRequest.builder()
+ .bucket(location.bucket())
+ .prefix(location.key())
+ .build());
+
+ Optional<ObjectVersion> latestVersion =
+ response.versions().stream().max(Comparator.comparing(ObjectVersion::lastModified));
+ return latestVersion.map(version -> recoverObject(version, location.bucket())).orElse(false);
Review Comment:
using `map` feels awkward here, can't we just use `ifPresent`?
Never mind, I see you're trying to return the boolean which is difficult the other way.
--
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
Re: [PR] AWS: Implement SupportsRecoveryOperations for S3FileIO [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #10721:
URL: https://github.com/apache/iceberg/pull/10721#discussion_r1709774374
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java:
##########
@@ -420,4 +427,40 @@ protected void finalize() throws Throwable {
}
}
}
+
+ @Override
+ public boolean recoverFile(String path) {
+ S3URI location = new S3URI(path, s3FileIOProperties.bucketToAccessPointMapping());
+ ListObjectVersionsIterable response =
+ client()
+ .listObjectVersionsPaginator(
+ ListObjectVersionsRequest.builder()
+ .bucket(location.bucket())
+ .prefix(location.key())
+ .build());
+
+ Optional<ObjectVersion> latestVersion =
+ response.versions().stream().max(Comparator.comparing(ObjectVersion::lastModified));
+ return latestVersion.map(version -> recoverObject(version, location.bucket())).orElse(false);
Review Comment:
using `map` feels awkward here, can't we just use `ifPresent`?
--
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
Re: [PR] AWS: Implement SupportsRecoveryOperations for S3FileIO [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10721:
URL: https://github.com/apache/iceberg/pull/10721#discussion_r1710093143
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java:
##########
@@ -420,4 +427,40 @@ protected void finalize() throws Throwable {
}
}
}
+
+ @Override
+ public boolean recoverFile(String path) {
+ S3URI location = new S3URI(path, s3FileIOProperties.bucketToAccessPointMapping());
+ ListObjectVersionsIterable response =
+ client()
+ .listObjectVersionsPaginator(
+ ListObjectVersionsRequest.builder()
+ .bucket(location.bucket())
+ .prefix(location.key())
+ .build());
+
+ Optional<ObjectVersion> latestVersion =
+ response.versions().stream().max(Comparator.comparing(ObjectVersion::lastModified));
+ return latestVersion.map(version -> recoverObject(version, location.bucket())).orElse(false);
Review Comment:
Yeah that seems like a simple check we can do to avoid unnecessary copies, updated! I didn't go down the route of comparing the latest deletion marker with the version to recover to, because inverting the logic to just verify that the version to recover to is not the latest is simple; and we know that "version to recover" cannot represent a deletion marker due to the guarantees provided by the S3 api.
--
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