Improve caching logic for http.send #5316
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
In order to calculate if a cached entry expired or not, we would previously read the headers stored with the request, and parse each of the relevant date-related ones and use those to compare with the current time. open-policy-agent#4960 improved this by allowing us to ignore the Date header completely when force_cache is enabled. We now go one step further, and calculate and store an expiresAt value at the time the entry is created. This allows for a much simpler check of expiry, and avoids re-parsing the header values with each lookup. Fixes open-policy-agent#5300 Signed-off-by: Anders Eknert <anders@eknert.com>
anderseknert
force-pushed
the
improve-http-send-caching
branch
from
5c43419 to
41f6f73
Compare
srenatus
approved these changes
Oct 27, 2022
Comment on lines
+915
to
+924
| if maxAge != -1 { | ||
| createdAt, err := getResponseHeaderDate(headers) | ||
| if err != nil { | ||
| return time.Time{}, err | ||
| } | ||
| expiresAt = createdAt.Add(time.Second * time.Duration(maxAge)) | ||
| } else { | ||
| expiresAt = getResponseHeaderExpires(headers) | ||
| } | ||
| return expiresAt, nil |
There was a problem hiding this comment.
[nit] It's the same, I'm just trying how this variant feels...:
Suggested change
| if maxAge != -1 { | |
| createdAt, err := getResponseHeaderDate(headers) | |
| if err != nil { | |
| return time.Time{}, err | |
| } | |
| expiresAt = createdAt.Add(time.Second * time.Duration(maxAge)) | |
| } else { | |
| expiresAt = getResponseHeaderExpires(headers) | |
| } | |
| return expiresAt, nil | |
| if maxAge != -1 { | |
| createdAt, err := getResponseHeaderDate(headers) | |
| if err != nil { | |
| return time.Time{}, err | |
| } | |
| return createdAt.Add(time.Second * time.Duration(maxAge)), nil | |
| } | |
| return getResponseHeaderExpires(headers), nil |
I think I'd slightly prefer this. But it's not a blocker, of course.
There was a problem hiding this comment.
I like that it's more compact, but OTOH it's somewhat easier for me to read that the "expiresAt" is ultimately what is returned... I guess one can figure that out from the function name, but still :)
| } else { | ||
| _, err := parseResponseHeaders(resp.Header) | ||
| var err error | ||
| expiresAt, err = expiryFromHeaders(resp.Header) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } |
|
|
||
| // check the freshness of the cached response | ||
| if isCachedResponseFresh(c.bctx, headers, c.forceCacheParams) { | ||
| if getCurrentTime(c.bctx).Before(cachedRespData.ExpiresAt) { |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In order to calculate if a cached entry expired or not, we would previously read the headers stored with the request, and parse each of the relevant date-related ones and use those to compare with the current time. #4960 improved this by allowing us to ignore the Date header completely when force_cache is enabled.
We now go one step further, and calculate and store an expiresAt value at the time the entry is created. This allows for a much simpler check of expiry, and avoids re-parsing the header values with each lookup.
Fixes #5300
Signed-off-by: Anders Eknert anders@eknert.com