Pluggable registry storage for uploads #1216
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1216 +/- ##
==========================================
- Coverage 73.76% 73.30% -0.47%
==========================================
Files 111 111
Lines 8181 8255 +74
==========================================
+ Hits 6035 6051 +16
- Misses 1549 1598 +49
- Partials 597 606 +9
Continue to review full report at Codecov.
|
| // uploadHandler represents a minimal upload storage backend, capable of | ||
| // appending streamed upload contents and finally returning them to be stored | ||
| // in blob storage. | ||
| type uploadHandler interface { |
There was a problem hiding this comment.
I'm not entirely sure I follow why this is separate from the Put interface above that enables making things writeable. Nor why the "Finalize" stuff below is separate.
There was a problem hiding this comment.
One variant I was thinking about was surfacing an upload interface for each of the possible upload styles:
- POST/PATCH/PUT (most common)
- POST/PUT
- PUT
These each have names we could use. I haven't fully thought this through, so mostly thinking out loud.
There was a problem hiding this comment.
I'm not entirely sure I follow why this is separate from the
Putinterface above that enables making things writeable.
A registry may only support some upload protocols and not others, and the way they'd tell us which they support is which interfaces they implement.
Nor why the "Finalize" stuff below is separate.
Finalize is meant to be a shortcut if you have your own way of finalizing an upload into the final blob. In memHandler we don't re-read the whole blob to move it to a finalized blob, we just move it to the blob map.
I'm open to naming them better definitely, I'll try some things out this week.
There was a problem hiding this comment.
A registry may only support some upload protocols and not others
This is sort of what I was suggesting by re-orienting the interfaces around those upload protocols. I believe each has a name, e.g. "monolithic upload". Did that make sense?
Finalize is meant to be a shortcut if you have your own way of finalizing an upload into the final blob
That makes sense. I assume you are using this downstream for something. Maybe just reflect this info into the comment block, if you haven't already?
There was a problem hiding this comment.
I've thought about this a bit more and I don't know if "which upload protocols I support" matches very well to "what operations my backing store can support". At least not well enough to make them the one and only knob.
I may have an in-memory impl that still shouldn't accept pushes (even though memHandler supports it), and I might want to express those differently. OTOH, you might have a storage impl that doesn't support pushes, and just bringing that should be enough to tell us not to accept pushes.
I'm thinking that might look like:
h := registry.New(
registry.WithStorage(layoutStore),
registry.DisablePush, // or just registry.DisableChunkedUpload
)or
h := registry.New(
registry.WithStorage(readOnlyLayoutStorage),
)(these would be equivalent)
An alternative would be to provide a ReadOnlyer that takes whatever impl it's given and shadows the implementation of PutBlob, *Upload, etc. -- that feels a bit like a too-cute use of Go's type system, and ultimately less expressive than just making the caller tell us whether to disable pushes.
This way, storage implementations get some control over what registry operations are allowed, based on what they support, but so do callers setting up the registry.
All of this might just indicate we're being too cute with the type system in general, and I should refocus on exactly the two impls we expect to see: in-memory and layout-on-disk, both of which support all these methods, including the shortcutting upload finalizer.
|
|
||
| // AppendUpload appends the contents of the ReadCloser to the current | ||
| // upload contents, and returns the new total size. | ||
| AppendUpload(ctx context.Context, uploadID string, rc io.ReadCloser) (int64, error) |
There was a problem hiding this comment.
Can we [ab]use io.WriterAt to express this instead? Feels like it maps pretty well to chunked uploads.
|
This Pull Request is stale because it has been open for 90 days with |
Continuing from #1158
This adds interfaces and an in-memory implementation for storing partial uploads and committing them to finished blobs.