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.

Sign up for GitHub

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

Jump to bottom

handler, s3store: Serve GET requests directly from S3 #1208

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

handler, s3store: Serve GET requests directly from S3 #1208

wants to merge 1 commit into from

Conversation

pcfreak30
Copy link

@pcfreak30 pcfreak30 commented Oct 20, 2024 •
edited
Loading

  • Add ContentServerDataStore 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

Closes #1064

@pcfreak30
Copy link
Author

Please note I tried to avoid changing the go version, but go mod tidy seems to automatically do it.

This is a draft to get feedback.

@Acconut

@Acconut Acconut changed the title feat(handler, s3store): implement ServerDataStore for direct content serving, closes #1064 feat(handler, s3store): implement ServerDataStore for direct content serving Oct 21, 2024
Acconut
Acconut reviewed Oct 21, 2024
View reviewed changes
Copy link
Member

@Acconut Acconut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for working on this! I left a few comments but the direction where this is heading is looking good.

pkg/s3store/s3store.go Outdated Show resolved Hide resolved
pkg/s3store/s3store.go Outdated Show resolved Hide resolved
pkg/s3store/s3store.go Outdated Show resolved Hide resolved
pkg/s3store/s3store.go Outdated Show resolved Hide resolved
pkg/handler/composer.go Outdated Show resolved Hide resolved
@pcfreak30 pcfreak30 force-pushed the issue/1064 branch 2 times, most recently from 6e09111 to d9e8bca Compare October 21, 2024 07:47
@pcfreak30
Copy link
Author

@Acconut changes have been made. kudos.

Acconut
Acconut reviewed Nov 26, 2024
View reviewed changes
pkg/handler/datastore.go
@@ -121,6 +122,16 @@ type DataStore interface {
GetUpload(ctx context.Context, id string) (upload Upload, err error)
}

// ServableUpload defines the method for serving content directly
type ServableUpload interface {
ServeContent(ctx context.Context, w http.ResponseWriter, r *http.Request) error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function needs proper documentation. That's best done when we have agreed on what the handler does and does not.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lmk what/when you want docs on this.

pkg/handler/unrouted_handler.go Show resolved Hide resolved
pkg/s3store/s3store.go Outdated Show resolved Hide resolved
Acconut
Acconut requested changes Nov 26, 2024
View reviewed changes
Copy link
Member

@Acconut Acconut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the updates! This is moving in the right direction and I feel like we're getting close :)

pkg/s3store/s3store.go Show resolved Hide resolved
@pcfreak30
feat(handler, s3store): implement ContentServerDataStore for direct c…
79384ce 
…ontent serving, closes tus#1064

- Add ServerDataStore interface
- Implement ContentServerDataStore in S3Store with streaming support
- Add Range header support for partial content requests
- Update StoreComposer to support ContentServer capability
- Add tests for new ContentServerDataStore functionality
- Update Go version to 1.22.1
@pcfreak30
Copy link
Author

@Acconut changes have been made.

@Acconut Acconut changed the title feat(handler, s3store): implement ServerDataStore for direct content serving handler, s3store: Serve GET requests directly from S3 Nov 28, 2024
@Acconut
Copy link
Member

Acconut commented Nov 28, 2024

Thank you for the updates! I added some documentation and better handling for range requests and conditional requests, in particular regarding error responses. I couldn't push these changes to this PR because you didn't allow maintainers to modify your PR. So for now, I put them into a separate branch: main...s3-content-server

Feel free to review them. If you allow me to modify this PR, I can push them here. If not, we'll find another way to collaborate.

@pcfreak30 pcfreak30 marked this pull request as ready for review November 28, 2024 21:05
@pcfreak30
Copy link
Author

@Acconut sorry GitHub doesn't seem to be allowing me to give you access :/.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Acconut Acconut Acconut requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

handler: support HTTP range requests in GET /:upload
2 participants
@pcfreak30 @Acconut