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(providers): expand PutBlob API to allow for idempotent puts #1654

Merged
merged 5 commits into from Jan 27, 2022

Conversation

adowair
Copy link
Contributor

@adowair adowair commented Jan 12, 2022

Today, executing a PutBlob operation will always overwrite a blob if it exists. It would be helpful for some customer workflows to be able to write blobs only if they don't exist (i.e. don't overwrite/recreate blobs). This PR expands the PutBlob API with an additional option DoNotRecreate, which toggles this feature. Note that not all backends support idempotent creates, so this PR also implements error logic to rejects such PutBlobs if they can't be executed.

@codecov
Copy link

codecov bot commented Jan 13, 2022

Codecov Report

Merging #1654 (4fd99d9) into master (e67f84e) will decrease coverage by 0.03%.
The diff coverage is 45.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1654      +/-   ##
==========================================
- Coverage   70.72%   70.69%   -0.04%     
==========================================
  Files         386      386              
  Lines       30760    30775      +15     
==========================================
  Hits        21755    21755              
- Misses       7336     7351      +15     
  Partials     1669     1669              
Impacted Files Coverage Δ
repo/blob/retrying/retrying_storage.go 90.69% <0.00%> (-4.43%) ⬇️
repo/blob/sharded/sharded.go 89.02% <ø> (+1.59%) ⬆️
repo/blob/storage.go 95.83% <ø> (ø)
repo/blob/filesystem/filesystem_storage.go 85.84% <20.00%> (-1.55%) ⬇️
internal/providervalidation/providervalidation.go 65.63% <57.14%> (-0.24%) ⬇️
repo/blob/sftp/sftp_storage.go 57.76% <60.00%> (-0.09%) ⬇️
cli/command_repository_validate_provider.go 76.47% <100.00%> (+1.47%) ⬆️
internal/listcache/listcache.go 79.41% <0.00%> (-3.93%) ⬇️
internal/ownwrites/ownwrites.go 78.70% <0.00%> (-2.78%) ⬇️
repo/maintenance/maintenance_schedule.go 71.55% <0.00%> (-2.59%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e67f84e...4fd99d9. Read the comment docs.

@jkowalski
Copy link
Contributor

Is this only going to work for GCS? What about S3-like providers?

@Shrekster
Copy link
Collaborator

Is this only going to work for GCS? What about S3-like providers?

Currently only GCS, yes. If/When S3 has support for write-once then we can extend.

@adowair adowair marked this pull request as draft January 18, 2022 17:25
@adowair adowair marked this pull request as ready for review January 19, 2022 23:59
@adowair adowair marked this pull request as draft January 20, 2022 16:41
@jkowalski jkowalski changed the title Expand PutBlob API to allow for idempotent puts feat(providers): expand PutBlob API to allow for idempotent puts Jan 24, 2022
@adowair adowair marked this pull request as ready for review January 24, 2022 19:01
When `DoNotRecreate` is set as true, the blob put operation should
only succeed if no blob with the given blob ID already exists.
Othwerwise, `ErrBlobAlreadyExists` is returned.
By default, storage providers should not support idempotent creates.
This commit adds error handling to exit early if `DoNotRecreate` is
set to true. The commit also verifies this behavior in the provider
validation test.
When PutBlob options were introduced, error handling logic for them
was implemented for the Sharded storage interface. However, the
behavior of different providers that implement Sharded can be
different, so it's better to push the options down to be processed in
the provider implementations.
To unify error handling code and make it more maintainable, introduce
a new error type `blob.ErrUnsupportedPutBlobOption`, which is to be
returned whenever a storage provider implementation is given put
options it does not support.
@jkowalski
Copy link
Contributor

LGTM, although I'm still unsure how broadly useful this will be, if GCS is the only provider that can support it.

@jkowalski jkowalski merged commit 7ca8b85 into kopia:master Jan 27, 2022
@adowair
Copy link
Contributor Author

adowair commented Jan 27, 2022

Thank you @jkowalski, I will also investigate other storage providers and add support in future PRs as is possible.

jkowalski added a commit to jkowalski/kopia that referenced this pull request Jan 29, 2022
There was a small regression in kopia#1654 - only in negative tests.

Also removed unnecessary retries on ErrUnsupportedPutBlobOption
+ switched provider test to run once per day, instead of 12 times/day.
julio-lopez pushed a commit that referenced this pull request Jan 29, 2022
There was a small regression in #1654 - only in negative tests.

Also removed unnecessary retries on ErrUnsupportedPutBlobOption
+ switched provider test to run once per day, instead of 12 times/day.
@julio-lopez julio-lopez deleted the idempotent-put-blob branch June 17, 2022 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants