GCP: Implement SupportsRecoveryOperations for GCSFileIO #11565
Conversation
gcp/src/test/java/org/apache/iceberg/gcp/gcs/GCSFileIOTest.java
Outdated
Show resolved
Hide resolved
gcp/src/test/java/org/apache/iceberg/gcp/gcs/GCSFileIOTest.java
Outdated
Show resolved
Hide resolved
gcp/src/test/java/org/apache/iceberg/gcp/gcs/GCSFileIOTest.java
Outdated
Show resolved
Hide resolved
amogh-jahagirdar
left a comment
amogh-jahagirdar
left a comment
There was a problem hiding this comment.
Thanks @mrcnc , I think this is pretty close, just some comments on adding more details to the logs!
| LOG.info("Latest deleted version was restored for {}", blob.getBlobId()); | ||
| return true; | ||
| } | ||
| LOG.warn("No latest deleted version was found"); |
There was a problem hiding this comment.
Could we include the blobID in the log?
| LOG.warn("No latest deleted version was found"); | ||
|
|
||
| } catch (StorageException e) { | ||
| LOG.warn("Failed to restore latest deleted version", e); |
| LOG.warn("No soft deleted object was found"); | ||
|
|
||
| } catch (StorageException e) { | ||
| LOG.warn("Failed to restore", e); |
There was a problem hiding this comment.
Same as below, could we include the blobID in these log messages?
| } catch (IllegalArgumentException e) { | ||
| LOG.warn("Invalid GCS path format: {}", path, e); | ||
| } |
There was a problem hiding this comment.
Actually just noticed this, not sure it makes sense to swallow the IllegalArgumentException in case the path format is incorrect? I consider this just like the null or empty case where recoverFile can just throw. The best effort really is for the calls to attempt recovery, not the other validation imo.
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
2 participants
Inspired by the initial implementation in S3FileIO, this PR implements
SupportsRecoveryOperationsfor GCSFileIO.GCP recommends using soft delete instead of object versioning, so this implementation attempts to-restore a soft deleted object first. If soft delete is not enabled (which may be common since it's a relatively new feature), then it attempts to restore a previously deleted version by coping it to the live location, similar to the S3FileIO implementation.