Skip to content

Cache patron's loans to reduce IA requests #7929

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

Merged

Conversation

JaydenTeoh
Copy link
Collaborator

Closes #7591

Implement cache logic that caches patron's loan with 5 minutes time-to-live. If this 5 minute window has passed or if patron has borrowed another book (via /borrow/ia/$(ocaid)), then the cache will be invalidated => loans will be fetched again via api.

Stakeholders

@mekarpeles

@JaydenTeoh JaydenTeoh force-pushed the 7591/feature/cache-active-loans branch from 3880023 to c2f2f1d Compare June 3, 2023 17:16
@JaydenTeoh JaydenTeoh changed the title scaffold cache logic patron loans caching Jun 3, 2023
@mekarpeles mekarpeles requested a review from jimchamp June 5, 2023 19:26
@mekarpeles mekarpeles self-assigned this Jun 5, 2023
@mekarpeles mekarpeles added the Priority: 1 Do this week, receiving emails, time sensitive, . [managed] label Jun 5, 2023
@mekarpeles
Copy link
Member

I think this is close, but I think we want both the ability to fetch loans and also to fetch cached loans independently. The borrow flow needs to have an authoritative way to fetch loans with no chance of them being cached. Other pages (like where LoanStatus macro fetches loans) should use the cache.

@JaydenTeoh
Copy link
Collaborator Author

JaydenTeoh commented Jun 8, 2023

I think this is close, but I think we want both the ability to fetch loans and also to fetch cached loans independently. The borrow flow needs to have an authoritative way to fetch loans with no chance of them being cached. Other pages (like where LoanStatus macro fetches loans) should use the cache.

@mekarpeles I have implement a use_cache=True argument within get_loans_of_user. Within the borrow.py flow, I have added two definite checks to ensure that the loans data will be fetched from API rather than cached results.
First, if a user is present,
lending.get_loans_of_user.cache_reset(user.key) is called to wipe all the cache for that user.key

Also, for all the parent functions of get_loans_of_user that exist within borrow.py, I have added the use_cache=False argument to ensure that only API results are returned.

Although I am unable to test loans on local, I have tested the flow of this cache implementation with a basic python script and it is behaving as expected. See picture below:
image

@mekarpeles
Copy link
Member

mekarpeles commented Jun 14, 2023

Currently, the places where we're making expensive calls for a patron's loans and waitlists are:

  1. In macros.LoanStatus
    which is used for Books Page, Search Results, My Books
  1. My Loans Page
  2. When a borrow occurs in core/lending.py or plugins/upstream/borrow.py

I believe the only time we want to use cache (for now) is within [1].

I would be nice if we had a new @public function called get_patron_loans, which calls our existing slow functions which returns a dictionary that looks like...

{
 "loans": [...],
 "holds": [...]
}

This would get cached for 5 minutes and could be called by any non-critical path, such as [1] LoanStatus. Within the critical paths (borrow endpoints) we should use the expensive, non-cached endpoints).

We may be able to leverage the use_cache=True logic you added for this.

@mekarpeles mekarpeles assigned jimchamp and unassigned mekarpeles Jun 14, 2023
@JaydenTeoh
Copy link
Collaborator Author

@jimchamp
Latest changes:

  1. Refactor caching mechanism to use memcache instead of my own cache decorator because there are vulnerabilities in my original cache implementation such as memory leak and memory overload
  2. Implement a 'opt-in' instead of 'opt-out' system for using cache
  3. Use a constant variable ttl_for_cached_loans = 5 * cache.MINUTE_SECS rather than a magic number 300
  4. Use delegate.fakeload() such that the cache function get_cached_user_loans can interact with the API if this is first time calling the function or if the previous cached result has expired. I am not sure whether this would work so I'll need help with staging and testing this.

One issue with my current implementation is that it only caches the user loans, but there is no caching implemented for user's waiting loans yet. Should I do so for waiting loans as well? Is it an expensive call?

Copy link
Collaborator

@jimchamp jimchamp left a comment

Choose a reason for hiding this comment

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

There are quite a few changes to the critical path that I think should be reverted. Holds (AKA waitlist items) are also expensive to fetch and should also be cached.

Now that you're using Open Library's cache, you'll have to invalidate the cache using memcache_delete. The exact invocation that I suggested may not work, so you may need to experiment a bit.

Following Mek's suggestion to create a public get_patron_loans function that returns both loans and holds would be helpful when we expand this functionality to the My Book page.

@jimchamp
Copy link
Collaborator

Since we're only focusing on the LoanStatus buttons for this PR, let's punt on the new get_patron_lists function.

@cdrini cdrini added Priority: 2 Important, as time permits. [managed] Needs: Patch Deploy and removed Priority: 1 Do this week, receiving emails, time sensitive, . [managed] labels Sep 11, 2023
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.

lgtm, tested on testing, to be patch-deployed on prod (on Monday)

@mekarpeles mekarpeles added Priority: 1 Do this week, receiving emails, time sensitive, . [managed] and removed Priority: 2 Important, as time permits. [managed] labels Nov 22, 2023
Comment on lines 522 to 523
loandata = web.ctx.site.store.values(type='/type/loan', name='user', value=user_key)
loans = [Loan(d) for d in loandata] + (_get_ia_loans_of_user(account.itemname))
Copy link
Member

Choose a reason for hiding this comment

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

Q: why are we calling both IA loans (which presumably uses s3) and OL web.ctx loans store?

@mekarpeles
Copy link
Member

finally lgtm!!

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.

lgtm

@mekarpeles mekarpeles merged commit 3bad332 into internetarchive:master Dec 8, 2023
@cdrini cdrini changed the title patron loans caching Cache patron's loans to reduce IA requests Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Patch Deploy Priority: 1 Do this week, receiving emails, time sensitive, . [managed]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cache Patron's Active Loans
4 participants