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
Expand PutBlob API to allow for idempotent creates #341
Changes from 15 commits
7326e8f
fe5bb76
b0add00
db5c659
eb00175
f96ba9a
4a09068
0c81ff3
eb6ec3b
36f9582
bf7ac3a
53da15d
7bd0b04
2737a12
a8fdccf
9ab4138
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -178,8 +178,11 @@ func (s *Storage) GetMetadata(ctx context.Context, blobID blob.ID) (blob.Metadat | |
|
||
// PutBlob implements blob.Storage. | ||
func (s *Storage) PutBlob(ctx context.Context, blobID blob.ID, data blob.Bytes, opts blob.PutOptions) error { | ||
if opts.HasRetentionOptions() { | ||
switch { | ||
case opts.HasRetentionOptions(): | ||
return errors.New("setting blob-retention is not supported") | ||
case opts.DoNotRecreate: | ||
return errors.New("setting blob do-not-recreate is not supported") | ||
Comment on lines
+184
to
+185
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. Wouldn't this be a function of the actual storage backend? It is true that currently the sharded provider is only used with storage backends that do not support DoNotRecreate, but still, it seems that it should be relayed to the backend. 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. That's a good point @julio-lopez. I can push this logic down to the individual storage providers. @Shrekster , I see you implement handling of 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. Good catch! Yes, my bad. I should have taken @adowair Can you move both the checks to the storage backend implementations of the shared interface ? 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. It'd may be a good idea to break down and restructure the PR in self-contained changes (commits) that can be individually reviewed. For example, the change about passing down the options to the storage backends can be one of those steps. Merely a suggestion. 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, that's the right way to do things. Rebasing upstream. |
||
} | ||
|
||
dirPath, filePath, err := s.GetShardedPathAndFilePath(ctx, 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.
With this implementation, it is the case that a precondition failure means the blob already exists, since it is the only condition being set.
But a
StatusPreconditionFailed
could be returned if a different precondition were set. How can we future proof this so a bug is not inadvertently introduced if for some reason we end up setting other pre condition checks?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.
Thanks for the suggestion @julio-lopez, the context needed for this is not available within
translateError()
, so we'd have to pass the put optionsopts
intotranslateError()
, or do some special error handling outside said function. Neither solution is particularly neat, but I am leaning towards the former. 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 hate to say this, but how about a simple error string match ?
I think passing in an error code or wrapping the error with more context in the same function where the pre-conditions are set is a better option, but I would avoid putting too much scope on this one PR.
Doing the wrap closer to the condition setup is the key. Passing in put-options is also a neat idea, but it may not be quite intuitive to code readers why a generic-op translateError() needs the put-opts. Not saying it cannot/should not be done though.