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(storage): add object retention feature #9072

Merged
merged 11 commits into from
Dec 13, 2023

Conversation

BrennaEpp
Copy link
Contributor

@BrennaEpp BrennaEpp commented Dec 4, 2023

  • API is not yet available in gRPC. This code ignores any retention setting attempt and will never return retention if using the gRPC API.

  • Updated EnableObjectRetention() to be a method on the bucket handle as per discussion w/ @tritone

Fixes #8929

@BrennaEpp BrennaEpp requested review from a team as code owners December 4, 2023 19:17
@product-auto-label product-auto-label bot added size: l Pull request size is large. api: storage Issues related to the Cloud Storage API. labels Dec 4, 2023
Copy link
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

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

Overall looks pretty good, thanks for the extensive integration tests. I have a few questions/concerns as noted.

@@ -162,6 +162,8 @@ func (c *grpcStorageClient) CreateBucket(ctx context.Context, project, bucket st
b.Location = "US"
}

// TO-DO: implement ObjectRetention once available - see b/308194853
Copy link
Contributor

Choose a reason for hiding this comment

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

I think for now we should return a NotImplemented error if a user tries to pass this, so that it doesn't just silently not happen.

Copy link
Contributor Author

@BrennaEpp BrennaEpp Dec 13, 2023

Choose a reason for hiding this comment

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

I saw a reference to a storage-specific Unimplemented error, but no actual definition. However, users can still attempt to get the retention attribute on an object without error; it'll just silently be nil.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant the gRPC unimplemented error code: see https://pkg.go.dev/google.golang.org/grpc@v1.60.0/codes#Unimplemented . I think we can just make and return this.

storage/storage.go Outdated Show resolved Hide resolved
storage/storage.go Outdated Show resolved Hide resolved
storage/bucket_test.go Show resolved Hide resolved
storage/bucket.go Outdated Show resolved Hide resolved
storage/integration_test.go Outdated Show resolved Hide resolved
storage/integration_test.go Outdated Show resolved Hide resolved
@BrennaEpp BrennaEpp added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 13, 2023
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 13, 2023
Copy link
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

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

Overall looking really good; a few nits and minor questions.

storage/bucket.go Outdated Show resolved Hide resolved
storage/bucket.go Outdated Show resolved Hide resolved
storage/client.go Show resolved Hide resolved
storage/grpc_client.go Outdated Show resolved Hide resolved
storage/storage.go Outdated Show resolved Hide resolved
// from Unlocked to Locked, to set it to null, or to reduce its
// RetainUntil attribute. It is not required for setting the ObjectRetention for
// the first time nor for extending the RetainUntil time.
func (o *ObjectHandle) OverrideUnlockedRetention(override bool) *ObjectHandle {
Copy link
Contributor

Choose a reason for hiding this comment

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

What does sending false for this query parameter do? (Same question applies for the bucket-level one as well.)

I'm wondering if we should remove the arg from these methods and just set to true if the user calls them. Thoughts?

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 don't think it does anything now, but I am hesitant to change it. I think we want to /really/ make sure users want to use these query params; in my opinion having the bool passed in adds a small extra step that doesn't really impact user experience negatively very much.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I guess it also mirrors the API itself more closely so there doesn't seem to be much harm either way.

Copy link
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

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

Nice work on this!

@BrennaEpp BrennaEpp enabled auto-merge (squash) December 13, 2023 23:52
@BrennaEpp BrennaEpp merged commit 16ecfd1 into googleapis:main Dec 13, 2023
9 checks passed
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 Cloud Storage API. size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

storage: add support for object retention
4 participants