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

Cache collections with new CacheablePathsProcessor #9069

Merged
merged 5 commits into from
May 10, 2024

Conversation

jimchamp
Copy link
Collaborator

Closes #9058

Creates new CacheablePathsProcessor that can be used to cache Infogami pages without overriding the default Infogami handler for such pages.

This processor attempts to fetch pages from cache when the following are true:

  1. The request is a GET
  2. The request is not specially encoded data (json, yml, etc.)
  3. The request's path matches one of the patterns in the paths_and_expiries dictionary
  4. debug is not in the request's query string
  5. The requested page's mode is view

The processor has been set up to cache/collections and /collections/{collection_identifier} pages.

Technical

Cache keys will take the form:
{web.ctx.path}.{web.ctx.lang}{query_string}

...where query_string is either an empty string or an ordered query string (e.g. ?a=1&b=1&c=1)

Adding a new path pattern and expiry time to paths_and_expiries will enable caching for pages matching to path pattern.

Testing

  1. Deploy this branch to a testing environment
  2. Navigate to the /collections page
  3. On page load, refresh the page

The load time on refresh should be noticeably faster than the initial page load.

Screenshot

Stakeholders

@mekarpeles

The new processor attempts to return cached copies of `GET` requests to
paths defined in the `paths_and_expiries` dictionary.
from openlibrary.core.processors import ReadableUrlProcessor
from openlibrary.plugins.openlibrary.home import caching_prethread
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reviewer: Is it okay to use this caching_prethread?

I more or less re-implemented the homepage caching in this new processor, which uses caching_prethread.

The following are not cached:
- Pages with special encodings (json, yml, etc.)
- Pages where the mode is not `view` (edit pages, etc.)
- Pages where `debug` is included in the query parameters

Language is now taken into consideration when caching.

Cache key includes the language code and a sorted query string.
@jimchamp jimchamp force-pushed the cache-collections branch from 4c90b79 to c354ec2 Compare April 10, 2024 23:55
mc(_cache='delete')

return _page

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reviewer: Please check the order of the conditionals that return handler().

My goal is to order these such that we exit this method as quickly as possible if the page isn't being cached by this processor.

@mekarpeles
Copy link
Member

In addition to languages, print-disabled patron status and potentially safe-for-work mode may be useful to consider here.

It may be nice to codify what these are e.g. language, region, safe for work, print disabled, etc.

@mekarpeles mekarpeles assigned mekarpeles and unassigned jimchamp Apr 15, 2024
@mekarpeles mekarpeles added Priority: 2 Important, as time permits. [managed] Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] labels Apr 22, 2024
Copy link
Member

@mekarpeles mekarpeles left a comment

Choose a reason for hiding this comment

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

At minimum, cache should take into consideration lang and printdisabled (in he future sfw

@jimchamp jimchamp force-pushed the cache-collections branch from 48d9103 to 2e0416c Compare April 25, 2024 23:28
@jimchamp jimchamp added Needs: Review This issue/PR needs to be reviewed in order to be closed or merged (see comments). [managed] and removed Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] labels Apr 25, 2024
@jimchamp
Copy link
Collaborator Author

Caching localized pages was already in place (we both missed that during our quick review). Have added additional caching for print-disabled page requests, and have included commented-out code for sfw.

@jimchamp jimchamp force-pushed the cache-collections branch from 731ec1f to bde02c1 Compare April 25, 2024 23:40
@jimchamp jimchamp added the State: Blocked Work has stopped, waiting for something (Info, Dependent fix, etc. See comments). [managed] label May 6, 2024
@jimchamp

This comment was marked as off-topic.

@jimchamp jimchamp removed the State: Blocked Work has stopped, waiting for something (Info, Dependent fix, etc. See comments). [managed] label May 6, 2024
@mekarpeles mekarpeles removed the Needs: Review This issue/PR needs to be reviewed in order to be closed or merged (see comments). [managed] label May 10, 2024
@mekarpeles mekarpeles merged commit 810b0c5 into internetarchive:master May 10, 2024
4 checks passed
@jimchamp jimchamp deleted the cache-collections branch May 30, 2024 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: 2 Important, as time permits. [managed]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Carousel Macros to be cacheable
2 participants