feat: add soft delete feature #2403
Conversation
product-auto-label
bot
added
size: l
| .setUserProject(Option.USER_PROJECT.getString(options)); | ||
| .setUserProject(Option.USER_PROJECT.getString(options)) | ||
| .setSoftDeleted(Option.SOFT_DELETED.getBoolean(options)); | ||
| // .setGeneration(Option.GENERATION.getLong(options)); |
| .setIfMetagenerationNotMatch(Option.IF_METAGENERATION_NOT_MATCH.getLong(options)) | ||
| .setIfGenerationMatch(Option.IF_GENERATION_MATCH.getLong(options)) | ||
| .setIfGenerationNotMatch(Option.IF_GENERATION_NOT_MATCH.getLong(options)) | ||
| .setCopySourceAcl(Option.COPY_SOURCE_ACL.getBoolean(options)) |
There was a problem hiding this comment.
This is an interesting addition to the API; surprised it needs to be added if you're restoring an object.
| * @throws StorageException upon failure | ||
| */ | ||
| @TransportCompatibility({Transport.HTTP, Transport.GRPC}) | ||
| public Blob get(String blob, Long generation, BlobGetOption... options) { |
There was a problem hiding this comment.
IIUC this is needed because you can't specify generation on Bucket.get() that exists today; Would recommend using BlobId instead of adding Long generation overload.
Do you think this is hard requirement to support this feature since there's storage.get(BlobId..)?
There was a problem hiding this comment.
I think it would feel pretty weird that soft deleted would become the only state an object can be in where you can get it from Storage but can't get it from the Bucket that the object lives in.
I also think using BlobId here would feel weird, because you're already calling from a Bucket, so it's weird to have to supply the same Bucket name into the BlobId. There are no other methods in Bucket.java that have a BlobId in the signature
There was a problem hiding this comment.
Bleh, missed that bucket name is also part of BlobId. thanks.
We haven't had generation in Bucket.get() at all which is required for version enabled buckets, that's why i asked this question as well.
For the sake of discussion, WDYT about having generation under BlobGetOption's? It's a query paramter just like IfGenerationMatch etc.
| ifNonNull(from.getSoftDeletePolicy(), softDeletePolicyCodec::encode, to::setSoftDeletePolicy); | ||
| if (from.getModifiedFields().contains(SOFT_DELETE_POLICY) | ||
| && from.getSoftDeletePolicy() == null) { | ||
| System.out.println("this is happening"); |
| @@ -201,7 +201,9 @@ public ImmutableList<?> parameters() { | |||
| new Args<>(BlobField.UPDATED, LazyAssertion.equal()), | |||
| new Args<>( | |||
| BlobField.RETENTION, | |||
| LazyAssertion.skip("TODO: jesse fill in buganizer bug here"))); | |||
| LazyAssertion.skip("TODO: jesse fill in buganizer bug here")), | |||
There was a problem hiding this comment.
I understand this code already exists; just wondering why is Retention field mask test skipped?
There was a problem hiding this comment.
This is related to object retention being JSON only. This will be removed once it's in the grpc api as well
| RestoreObjectRequest.newBuilder() | ||
| .setBucket(bucketNameCodec.encode(blobId.getBucket())) | ||
| .setObject(blobId.getName()); | ||
| ifNonNull(blobId.getGeneration(), builder::setGeneration); |
There was a problem hiding this comment.
Is the API failure helpful when generation isn't supplied?
There was a problem hiding this comment.
Yeah, it says "you must specify a generation"
| import static com.google.cloud.storage.Utils.projectNameCodec; | ||
| import static com.google.cloud.storage.Utils.topicNameCodec; | ||
| import static com.google.cloud.storage.Storage.BucketField.SOFT_DELETE_POLICY; | ||
| import static com.google.cloud.storage.Utils.*; |
There was a problem hiding this comment.
Revert com.google.cloud.storage.Utils.*;
| @@ -213,6 +213,11 @@ public ResultRetryAlgorithm<?> getForObjectsGet( | |||
| return retryStrategy.getIdempotentHandler(); | |||
| } | |||
|
|
|||
| public ResultRetryAlgorithm<?> getForObjectsRestore( | |||
| StorageObject pb, Map<StorageRpc.Option, ?> optionsMap) { | |||
| return retryStrategy.getIdempotentHandler(); | |||
There was a problem hiding this comment.
Missed this on first pass; ObjectRestore is idempotent because it can only succeed once which is similar to Bucket; is that your understanding as well?
There was a problem hiding this comment.
Yes, restore has the same idempotency as Copy and similar operations
| @@ -30,6 +30,8 @@ class HttpStorageRpcSpans { | |||
| static final String SPAN_NAME_LIST_OBJECTS = getTraceSpanName("list(String,Map)"); | |||
| static final String SPAN_NAME_GET_BUCKET = getTraceSpanName("get(Bucket,Map)"); | |||
| static final String SPAN_NAME_GET_OBJECT = getTraceSpanName("get(StorageObject,Map)"); | |||
|
|
|||
| OVERRIDE_UNLOCKED_RETENTION("overrideUnlockedRetention"); | ||
| OVERRIDE_UNLOCKED_RETENTION("overrideUnlockedRetention"), | ||
| SOFT_DELETED("softDeleted"), | ||
|
|
JesseLovelace
added
do not merge
Implements the soft delete feature as requested in go/gcs-soft-delete-client-request
Also adds the
get(String blob, Long generation, BlobGetOption...options)method signature to Bucket.java. It's already present in Storage.java, I'm adding it now because it's necessary to get a soft deleted object.