feat(handler, s3store): implement ServerDataStore for direct content serving #1208
Conversation
|
Please note I tried to avoid changing the go version, but This is a draft to get feedback. |
Acconut
changed the title
pkg/s3store/s3store.go
Outdated
| var _ handler.ServerDataStore = &S3Store{} | ||
|
|
||
| func (store S3Store) AsServableUpload(upload handler.Upload) handler.ServableUpload { | ||
| return &S3ServableUpload{ |
There was a problem hiding this comment.
If we implement ServeContent directly on s3Upload, we do not have to introduce another struct S3ServableUpload. This is already what we do for AsTerminatableUpload:
Lines 367 to 369 in 9779a84
There was a problem hiding this comment.
I can change this, though I did see value in it.
pkg/s3store/s3store.go
Outdated
| upload *s3Upload | ||
| } | ||
|
|
||
| func (su *S3ServableUpload) ServeContent(ctx context.Context, w http.ResponseWriter, r *http.Request) error { |
There was a problem hiding this comment.
Before merging, we should move these functions down in the file as they are not the most important ones. Maybe place the logic next to the other optional interfaces that S3Store implements.
pkg/s3store/s3store.go
Outdated
| @@ -1249,3 +1335,95 @@ func (store S3Store) releaseUploadSemaphore() { | |||
| store.uploadSemaphore.Release() | |||
| store.uploadSemaphoreDemandMetric.Dec() | |||
| } | |||
|
|
|||
| func (store S3Store) copyToResponseWriter(w http.ResponseWriter, r io.Reader) (int64, error) { | |||
There was a problem hiding this comment.
Can you explain what the benefit of this is over io.Copy?
There was a problem hiding this comment.
TBH, I rely on AI for development and a lot of this I was able to complete due to its effort. The data copy and range logic is the most complicated parts, and as I did not have a good reason to remove them based on what I saw, I just ensured it was all valid.
If needed, I can replace with io.Copy and move on.
pkg/s3store/s3store.go
Outdated
| return err | ||
| } | ||
|
|
||
| func (su *S3ServableUpload) handleRangeRequest(ctx context.Context, w http.ResponseWriter, r *http.Request, info handler.FileInfo, input *s3.GetObjectInput, rangeHeader string) error { |
There was a problem hiding this comment.
I'm wonder whether dedicated handling for range requests is necessary. If the request includes the Range header we can add it to GetObject without parsing the ranges and let S3 deal with it. Afterwards we just copy the Content-Range, Content-Length and ETag header from S3's response to tusd's response and copy the body.
The advantage is that we don't have to parse the header (and possibly introduce subtle bugs) and don't have to construct the Content-Range and Content-Length headers on our own.
We would also not need a dedicated handleRangeRequest function as this could all live in ServeContent.
What do you think?
There was a problem hiding this comment.
I believe given that multiple ranges are possible, it made since to check it ourselves. and due to the complexity of the range checking, its why I had a unit test for the range logic made.
I can punt it all to be forwarded to s3, but I also think it is valid to verify at-least the ranges earlier in the s3 impl.
pkg/handler/composer.go
Outdated
| @@ -14,6 +14,8 @@ type StoreComposer struct { | |||
| Concater ConcaterDataStore | |||
| UsesLengthDeferrer bool | |||
| LengthDeferrer LengthDeferrerDataStore | |||
| Server ServerDataStore | |||
There was a problem hiding this comment.
As mentioned in #1064 (comment), I am not sure on my own whether Server is a good name as it does have a different meaning in the HTTP-land. How about ContentServer as an alternative?
We would then have StoreComposer.ContentServer, StoreComposer.UsesContentServer, StoreComposer.UseContentServer, and tusd.ContentServerDataStore.
There was a problem hiding this comment.
works for me, I don't really care :P.
…ontent serving, closes tus#1064 - Add ServerDataStore interface - Update handlers to use ContentServerDataStore when available - Implement range request handling for s3upload - Add tests for new ContentServerDataStore functionality - Update Go version to 1.22.1
|
@Acconut changes have been made. kudos. |
Closes #1064