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

Avoid communicating with S3 unless necessary #616

Merged
merged 2 commits into from
Aug 15, 2024

Conversation

garyttierney
Copy link
Contributor

Previously the S3 source would eagerly connect to S3 retrieve information about the object being requested when creating a stream. This change defers retrieval of properties from S3 until needed by the S3StreamFactory, and allows cached files to be served directly from disk with no intermediate S3 lookups.

It's a small optimization, but can shave a dozen or so MS off of a cached image request.

@DiegoPino
Copy link
Contributor

@glenrobson this works and tested over current develop branch. But I would love to have your larger pull merged first and test with it and ideally give this a spin on a production server with real load to see if having a get Object Info from S3 fail last (when deferred) has any other consequences, if that is OK

@glenrobson glenrobson requested a review from DiegoPino May 22, 2024 15:41
@DiegoPino
Copy link
Contributor

@garyttierney hi. I rebased against develop and created a docker container using S3 as cache and source backend. All works well (testing what happens if something fails/goes wrong is hard to replicate so my testing is based on successful operations). It would be great if you could rebase this pull against https://github.com/cantaloupe-project/cantaloupe so we can let the CI tests run. Please let us know if you could tackle that in the few next days so I can approve and we move forward with merging. Thanks so much for your contribution!

Previously the S3 source would eagerly connect to S3 retrieve
information about the object being requested when creating a stream.
This change defers retrieval of properties from S3 until needed by the
S3StreamFactory, and allows cached files to be served directly from disk
with no intermediate S3 lookups.
@garyttierney
Copy link
Contributor Author

garyttierney commented Aug 15, 2024

Yep no problem, rebased against latest changes in develop.

Copy link
Contributor

@DiegoPino DiegoPino left a comment

Choose a reason for hiding this comment

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

Works and checks (finally!) are all passing

@DiegoPino DiegoPino merged commit c41da6c into cantaloupe-project:develop Aug 15, 2024
6 checks passed
@DiegoPino
Copy link
Contributor

@garyttierney thanks for your contribution!

@DiegoPino DiegoPino added this to the 6.0 milestone Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants