-
Notifications
You must be signed in to change notification settings - Fork 70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add soft delete feature #2403
Changes from 1 commit
26326a4
407af36
22ae88d
bd4bb67
d77aa58
a888e80
2a85a65
4f1b6de
fd22008
7142bcb
0f837d3
c2884aa
ac6022e
1c363ff
dbe0d92
fb88608
04975fc
1de29b8
c98d8c7
3a4a00f
b3d70db
7986f9c
a93e162
5a744ca
0b8fe3f
be3e975
5e65c40
938e00c
8f38221
68ce8c0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,9 @@ | |
|
||
package com.google.cloud.storage; | ||
|
||
import com.google.api.client.util.Data; | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Revert |
||
|
||
import com.google.api.pathtemplate.PathTemplate; | ||
import com.google.cloud.Binding; | ||
import com.google.cloud.Condition; | ||
|
@@ -65,9 +67,6 @@ | |
import java.util.Map; | ||
import java.util.function.Function; | ||
|
||
import static com.google.cloud.storage.Storage.BucketField.SOFT_DELETE_POLICY; | ||
import static com.google.cloud.storage.Utils.*; | ||
|
||
final class GrpcConversions { | ||
static final GrpcConversions INSTANCE = new GrpcConversions(); | ||
|
||
|
@@ -90,7 +89,7 @@ final class GrpcConversions { | |
Codec.of(this::autoclassEncode, this::autoclassDecode); | ||
|
||
private final Codec<BucketInfo.SoftDeletePolicy, Bucket.SoftDeletePolicy> softDeletePolicyCodec = | ||
Codec.of(this::softDeletePolicyEncode, this::softDeletePolicyDecode); | ||
Codec.of(this::softDeletePolicyEncode, this::softDeletePolicyDecode); | ||
private final Codec<BucketInfo.LifecycleRule, Bucket.Lifecycle.Rule> lifecycleRuleCodec = | ||
Codec.of(this::lifecycleRuleEncode, this::lifecycleRuleDecode); | ||
private final Codec<BucketInfo, Bucket> bucketInfoCodec = | ||
|
@@ -291,7 +290,7 @@ private BucketInfo bucketInfoDecode(Bucket from) { | |
if (from.hasAutoclass()) { | ||
to.setAutoclass(autoclassCodec.decode(from.getAutoclass())); | ||
} | ||
if(from.hasSoftDeletePolicy()) { | ||
if (from.hasSoftDeletePolicy()) { | ||
to.setSoftDeletePolicy(softDeletePolicyCodec.decode(from.getSoftDeletePolicy())); | ||
} | ||
if (from.hasCustomPlacementConfig()) { | ||
|
@@ -380,7 +379,8 @@ private Bucket bucketInfoEncode(BucketInfo from) { | |
ifNonNull(from.getIamConfiguration(), iamConfigurationCodec::encode, to::setIamConfig); | ||
ifNonNull(from.getAutoclass(), autoclassCodec::encode, to::setAutoclass); | ||
ifNonNull(from.getSoftDeletePolicy(), softDeletePolicyCodec::encode, to::setSoftDeletePolicy); | ||
if(from.getModifiedFields().contains(SOFT_DELETE_POLICY) && from.getSoftDeletePolicy() == null) { | ||
if (from.getModifiedFields().contains(SOFT_DELETE_POLICY) | ||
&& from.getSoftDeletePolicy() == null) { | ||
System.out.println("this is happening"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. debug code |
||
System.out.println(Bucket.SoftDeletePolicy.getDefaultInstance().toString()); | ||
to.clearSoftDeletePolicy(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -412,22 +412,22 @@ public Blob restore(BlobId blob, BlobRestoreOption... options) { | |
private Blob internalObjectRestore(BlobId blobId, Opts<ObjectSourceOpt> opts) { | ||
Opts<ObjectSourceOpt> finalOpts = opts.prepend(defaultOpts).prepend(ALL_BLOB_FIELDS); | ||
GrpcCallContext grpcCallContext = | ||
finalOpts.grpcMetadataMapper().apply(GrpcCallContext.createDefault()); | ||
finalOpts.grpcMetadataMapper().apply(GrpcCallContext.createDefault()); | ||
RestoreObjectRequest.Builder builder = | ||
RestoreObjectRequest.newBuilder() | ||
.setBucket(bucketNameCodec.encode(blobId.getBucket())) | ||
.setObject(blobId.getName()); | ||
RestoreObjectRequest.newBuilder() | ||
.setBucket(bucketNameCodec.encode(blobId.getBucket())) | ||
.setObject(blobId.getName()); | ||
ifNonNull(blobId.getGeneration(), builder::setGeneration); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the API failure helpful when generation isn't supplied? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it says "you must specify a generation" |
||
RestoreObjectRequest req = finalOpts.restoreObjectRequest().apply(builder).build(); | ||
GrpcCallContext merge = Utils.merge(grpcCallContext, Retrying.newCallContext()); | ||
return Retrying.run( | ||
getOptions(), | ||
retryAlgorithmManager.getFor(req), | ||
() -> storageClient.restoreObjectCallable().call(req, merge), | ||
resp -> { | ||
BlobInfo tmp = codecs.blobInfo().decode(resp); | ||
return finalOpts.clearBlobFields().decode(tmp).asBlob(this); | ||
}); | ||
getOptions(), | ||
retryAlgorithmManager.getFor(req), | ||
() -> storageClient.restoreObjectCallable().call(req, merge), | ||
resp -> { | ||
BlobInfo tmp = codecs.blobInfo().decode(resp); | ||
return finalOpts.clearBlobFields().decode(tmp).asBlob(this); | ||
}); | ||
} | ||
|
||
@Override | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -213,7 +213,8 @@ public ResultRetryAlgorithm<?> getForObjectsGet( | |
return retryStrategy.getIdempotentHandler(); | ||
} | ||
|
||
public ResultRetryAlgorithm<?> getForObjectsRestore(StorageObject pb, Map<StorageRpc.Option, ?> optionsMap) { | ||
public ResultRetryAlgorithm<?> getForObjectsRestore( | ||
StorageObject pb, Map<StorageRpc.Option, ?> optionsMap) { | ||
return retryStrategy.getIdempotentHandler(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, restore has the same idempotency as Copy and similar operations |
||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theBucket
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 inBucket.java
that have aBlobId
in the signatureThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.