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

Improve http.send cache lookup and freshness logic #5300

Closed
anderseknert opened this issue Oct 25, 2022 · 5 comments · Fixed by #5316
Closed

Improve http.send cache lookup and freshness logic #5300

anderseknert opened this issue Oct 25, 2022 · 5 comments · Fixed by #5316

Comments

@anderseknert
Copy link
Member

The current approach to cache lookups and freshness checks is to store the headers as sent by the remote server, and each time we check if an entry should be fetched from cache or from the remote, we parse all the relevant date-related headers — Date, Cache-Control (including max-age) and Expires in order to determine if the cache entry is fresh.

Let's instead do this parsing only once — when the cache entry is created, and store the timestamps directly for comparison in the freshness checks. This would improve performance of cache lookups, but it would also nicely abstract away the details of the HTTP spec from the caching logic.

In order to do this, we'll need to extend the test suite for inter query caching in http.send, which surprisingly was almost non-existent before #5298 .. while some e2e tests are added there, we'll probably need another approach to test expiry.

anderseknert added a commit to anderseknert/opa that referenced this issue Oct 27, 2022
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 added a commit that referenced this issue Oct 27, 2022
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>
@lukyer
Copy link
Contributor

lukyer commented Oct 27, 2022

@anderseknert does this also improve some kind of automatic cache items invalidation when the max-age time is reached? We store a lot (thousands) http.send responses with low TTL (10s). Currently it is unnecessarily hanging in the cache until caching.inter_query_builtin_cache.max_size_bytes is reached (and when it is, items get wiped in FIFO manner instead of respecting the TTL).

Thanks!

@srenatus
Copy link
Contributor

Could you open an issue, please? (Or search for an existing one?) It seems like having a periodic cache-cleaning routine would help you, but let's flesh out the details together.

@lukyer
Copy link
Contributor

lukyer commented Oct 27, 2022

Sure, I wasn't sure if this improves the situation somehow or not. I'm not too familiar with your codebase yet.
If you think this does not improve the cache cleaning behaviour, I'm happy to create new feature request issue.

@srenatus
Copy link
Contributor

The idea here was to straighten something out that was a bit quirky in the code. No functional changes involved. Thanks for the issue!

@anderseknert
Copy link
Member Author

@lukyer: @srenatus is right about that. However, this change does pave the way for what you suggest, as this would have been quite expensive before, as we'd have to iterate over each item, parse all the date-related headers, and check for expiry. Now we can just check for expiry directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants