-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
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.
Overall looks pretty good, thanks for the extensive integration tests. I have a few questions/concerns as noted.
storage/grpc_client.go
Outdated
@@ -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 |
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 for now we should return a NotImplemented error if a user tries to pass this, so that it doesn't just silently not happen.
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 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.
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 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.
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.
Overall looking really good; a few nits and minor questions.
// 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 { |
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.
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?
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 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.
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.
Sure, I guess it also mirrors the API itself more closely so there doesn't seem to be much harm either way.
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.
Nice work on this!
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/ @tritoneFixes #8929