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

Revise CachePolicy based on what we've learned. #31050

Merged
merged 6 commits into from
Dec 7, 2023

Conversation

toddmgreer
Copy link
Contributor

@toddmgreer toddmgreer commented Nov 27, 2023

Commit Message:
Revise CachePolicy interface based on experience using a forked version.
Additional Description:

  • CachePolicy can now be stored in FilterState
  • CacheEntryUsability:
    • Added ttl for use in cache-status header
    • Added op==, op!= for use in tests
  • requestCacheable:
    • Now returns enum, so it can say it's ok to serve from cache but not insert into cahce
    • Renamed to requestCacheability to reflect not returning a bool
  • responseCacheable:
    • Now returns enum, so it can say to mark a cache key as uncacheable to enable later optimizations
    • Renamed to responseCacheability to reflect not returning a bool
    • Removed VaryHeader parameter--CacheFilter should only call responseCacheable if the Vary headers are ok
  • createCacheKey: renamed to cacheKey, to avoid implying that the key must literally be created here
  • Removed *CacheControl parameters--not always needed; CachePolicy implementations can make then if needed
  • Removed setCallbacks--CachePolicy implementations that need FilterState should take it as a ctor parameter
    Risk Level: None--not yet used
    Testing: n/a--it's just an interface
    Docs Changes: n/a--not yet used
    Release Notes: n/a--not yet used
    Platform Specific Features: n/a

Signed-off-by: Todd Greer <tgreer@google.com>
@toddmgreer toddmgreer requested a review from jmarantz as a code owner November 27, 2023 02:05
@toddmgreer
Copy link
Contributor Author

/assign @mkbehr

Signed-off-by: Todd Greer <tgreer@google.com>
Signed-off-by: Todd Greer <tgreer@google.com>
ravenblackx
ravenblackx previously approved these changes Nov 28, 2023
Copy link
Contributor

@mkbehr mkbehr left a comment

Choose a reason for hiding this comment

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

Can you add a summary to the commit description of what's changing and why?

@@ -23,82 +22,96 @@ struct CacheEntryUsability {
* Value to be put in the Age header for cache responses.
*/
Seconds age = Seconds::max();
/**
* Remaining freshness lifetime.
Copy link
Contributor

Choose a reason for hiding this comment

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

Remaining relative to when?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clarified

Signed-off-by: Todd Greer <tgreer@google.com>
@toddmgreer
Copy link
Contributor Author

Can you add a summary to the commit description of what's changing and why?

I don't know how to do that. A maintainer with merge authority will eventually squash all commits into one, and I don't know how that commit gets its description. I guess we can bring this up with the maintainer once that review gets going.

@jmarantz
Copy link
Contributor

Maintainers usually cut and paste from the Deescription to populate the message for the squashed commit.

ravenblackx
ravenblackx previously approved these changes Nov 30, 2023
@mkbehr
Copy link
Contributor

mkbehr commented Nov 30, 2023

Can you add a summary to the commit description of what's changing and why?

I don't know how to do that. A maintainer with merge authority will eventually squash all commits into one, and I don't know how that commit gets its description. I guess we can bring this up with the maintainer once that review gets going.

There's a comment at the top of the pull request that got populated with a template, and IME that's where the description comes from. I think github will let you edit the comment.

This looks good to me so far but I want to make sure I understand everything that's different from our internal changes (e.g. removing the callbacks) before I hit approve, so the summary will help that.

@toddmgreer
Copy link
Contributor Author

Can you add a summary to the commit description of what's changing and why?
There's a comment at the top of the pull request that got populated with a template, and IME that's where the description comes from. I think github will let you edit the comment.

Thanks! Updated.

@ravenblackx ravenblackx removed their assignment Dec 4, 2023
@toddmgreer
Copy link
Contributor Author

@jmarantz this is ready for your review

Copy link
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

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

one minor nit and I think I can merge this, as it's just an extension.

// Don't respond to this request from cache, or store its response into cache.
Bypass,
// This request is eligible for serving from cache, but its response must not be stored. (E.g.
// requests with "Cache-Control: no-store").
Copy link
Contributor

Choose a reason for hiding this comment

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

I think Raven asked this already but I missed the answer, but how does a no-store request get served from cache if the response cannot be stored? Maybe you could add that to the comment here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Expanded explanation. Is that sufficiently clear?

Signed-off-by: Todd Greer <tgreer@google.com>
jmarantz
jmarantz previously approved these changes Dec 5, 2023
Copy link
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

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

Thanks!

@jmarantz
Copy link
Contributor

jmarantz commented Dec 5, 2023

Needs format fix

@jmarantz
Copy link
Contributor

jmarantz commented Dec 5, 2023

/wait

@jmarantz jmarantz self-assigned this Dec 5, 2023
Signed-off-by: Todd Greer <tgreer@google.com>
@toddmgreer
Copy link
Contributor Author

Fixed formatting/spelling.

@toddmgreer
Copy link
Contributor Author

/retest

@jmarantz jmarantz merged commit a00b628 into envoyproxy:main Dec 7, 2023
104 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants