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

Add ETag support to the Storage implementation #128

Closed
everettraven opened this issue Aug 1, 2023 · 4 comments
Closed

Add ETag support to the Storage implementation #128

everettraven opened this issue Aug 1, 2023 · 4 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Milestone

Comments

@everettraven
Copy link
Collaborator

As a follow up to #113, we should add ETag support to the HTTP server. This will allow clients to poll for changes using the ETag and only fetch all the catalog contents when something has changed. This would likely be middleware that we wrap our HTTP handler with.

Acceptance Criteria

  • The Storage implementation is updated to use middleware to enable ETag support on the HTTP server. This can be custom middleware or middleware provided by an external package
@joelanford joelanford modified the milestones: v0.5.0, v0.6.0 Aug 15, 2023
@everettraven everettraven added blocked kind/feature Categorizes issue or PR as related to a new feature. labels Aug 21, 2023
@everettraven everettraven modified the milestones: v0.6.0, v0.7.0 Aug 28, 2023
@anik120 anik120 removed the blocked label Sep 8, 2023
@joelanford
Copy link
Member

This might require some thinking:

  • Do we actually need an Etag (is last-modified header sufficient?)
  • Do we compute it on the fly? What would that do to response times?
  • Do we compute it once when we generate the all.json file? If so, where do we store the etag value, and how do we avoid races between reading the etag and reading the file? Maybe a RWMutex would need to come into play?

@everettraven
Copy link
Collaborator Author

I played around with this a bit and I think as long as we continue to use a fileserver the Last-Modified header should be sufficient. If we switch to having a custom server implementation that processes the information differently than serving a file, using the ETag header would likely be needed for client side caching.

@m1kola
Copy link
Member

m1kola commented Sep 15, 2023

Serving files is a perfect use case for Last-Modified/If-Modified-Since.

If we switch to having a custom server implementation that processes the information differently than serving a file, using the ETag header would likely be needed for client side caching.

I think even if we decide to implement server side filtering of all.json we can still rely on file modification date. As long as underlying file is the same - request with the same parameters should return the same result.

ETag would require doing the filtering on the server side to calculate some sort of hash. If we rely on file modification date we can avoid doing this even for endpoints that do some sort of processing of all.json. Edit: Thinking a bit more about this. I think we can do a hash of file modification date and use it as an ETag, but this makes little sense given that we can just use Last-Modified/If-Modified-Since.

@everettraven
Copy link
Collaborator Author

Closing this issue as obsolete as it has been discussed and consensus reached that the combination of using the pairing of Last-Modified / If-Modified-Since headers will be sufficient for implementing client side caching logic.

For more context, see the discussion here: https://kubernetes.slack.com/archives/C0181L6JYQ2/p1694786005446749

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
Archived in project
Development

No branches or pull requests

4 participants