Skip to content
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

Merged
merged 30 commits into from
Mar 14, 2024
Merged

feat: add soft delete feature #2403

merged 30 commits into from
Mar 14, 2024

Conversation

JesseLovelace
Copy link
Contributor

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.

@JesseLovelace JesseLovelace requested a review from a team as a code owner February 8, 2024 23:15
@product-auto-label product-auto-label bot added size: l Pull request size is large. api: storage Issues related to the googleapis/java-storage API. labels Feb 8, 2024
@JesseLovelace JesseLovelace requested a review from a team as a code owner February 8, 2024 23:18
Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have a couple of questions. Thanks Jesse

.setUserProject(Option.USER_PROJECT.getString(options));
.setUserProject(Option.USER_PROJECT.getString(options))
.setSoftDeleted(Option.SOFT_DELETED.getBoolean(options));
// .setGeneration(Option.GENERATION.getLong(options));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might not be intentional

.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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Member

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..)?

Copy link
Contributor Author

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 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

Copy link
Member

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.

ifNonNull(from.getSoftDeletePolicy(), softDeletePolicyCodec::encode, to::setSoftDeletePolicy);
if (from.getModifiedFields().contains(SOFT_DELETE_POLICY)
&& from.getSoftDeletePolicy() == null) {
System.out.println("this is happening");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

debug code

@@ -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")),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand this code already exists; just wondering why is Retention field mask test skipped?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the API failure helpful when generation isn't supplied?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.*;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)");

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove extra line

OVERRIDE_UNLOCKED_RETENTION("overrideUnlockedRetention");
OVERRIDE_UNLOCKED_RETENTION("overrideUnlockedRetention"),
SOFT_DELETED("softDeleted"),

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove extra line

@JesseLovelace JesseLovelace added do not merge Indicates a pull request not ready for merge, due to either quality or timing. and removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Mar 12, 2024
@JesseLovelace JesseLovelace merged commit 989f36f into main Mar 14, 2024
22 of 23 checks passed
@JesseLovelace JesseLovelace deleted the softdel branch March 14, 2024 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the googleapis/java-storage API. size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants