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

Contentful/cache size limit #1642

Merged
merged 4 commits into from
Jul 8, 2021
Merged

Conversation

dpinol
Copy link
Contributor

@dpinol dpinol commented Jun 15, 2021

Depends on #1639 . ⚠️ Please review it first

Description

#1639 created fallback cache decorator around Contentful API which remembers forever the last successful invocation for each combination of arguments, and use its result whenever Contentful fails.
This PR adds a limit in the in memory cache size to avoid memory overflows.

Context

In case any bot adds arguments with non repeated values, the cache could grow infinitely.

Approach taken / Explain the design

  • When Cache reaches 50%, 75% or 90% size, a warning is reported to the logger configured in Contentful options
  • When Cache surpass the maximum size, existing cache items are deleted until the new item fits.
  • In the future, we might use an external library to have a LRU cache. Not critical because typically data in CMS is small (we're not caching media, only their URLs)

To document / Usage example

The contentful plugin by default uses memoization to remember forever the last successful invocation for each combination of arguments, and use its result whenever Contentful fails. To limit the amount of memory

Testing

The pull request...

  • has unit tests
  • has integration tests
  • doesn't need tests because... [provide a description]

@dpinol dpinol changed the base branch from master to contentful/memoizer June 15, 2021 16:42
@dpinol dpinol force-pushed the contentful/cache-size-limit branch 2 times, most recently from fc447f8 to 69c5a7b Compare June 16, 2021 05:13
@dpinol dpinol force-pushed the contentful/memoizer branch from 51bcd2b to 7ff7245 Compare June 16, 2021 07:36
@dpinol dpinol force-pushed the contentful/cache-size-limit branch 5 times, most recently from 420d45f to 7ee38d3 Compare June 16, 2021 10:01
@dpinol dpinol marked this pull request as ready for review June 16, 2021 10:01
@dpinol dpinol added the documentation Documentation changes label Jun 16, 2021
Copy link
Contributor

@asastre asastre left a comment

Choose a reason for hiding this comment

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

Nice one!

packages/botonic-plugin-contentful/src/util/cache.ts Outdated Show resolved Hide resolved
@dpinol dpinol force-pushed the contentful/memoizer branch from 7ff7245 to 63dca40 Compare June 18, 2021 09:10
@dpinol dpinol force-pushed the contentful/cache-size-limit branch from 7ee38d3 to 6d56b17 Compare June 18, 2021 09:11
@github-actions
Copy link

Unit Test Results

1 files  ±  0  2 suites   - 3   5s ⏱️ -15s
3 tests  - 17  3 ✔️  - 17  0 💤 ±0  0 ❌ ±0 

Results for commit 6d56b17. ± Comparison against base commit 63dca40.

@dpinol dpinol force-pushed the contentful/cache-size-limit branch 2 times, most recently from a263128 to 51bf352 Compare June 18, 2021 09:20
Base automatically changed from contentful/memoizer to master June 18, 2021 10:31
@dpinol dpinol force-pushed the contentful/cache-size-limit branch from 51bf352 to ede58d1 Compare June 18, 2021 10:32
@vanbasten17 vanbasten17 merged commit 0e8a029 into master Jul 8, 2021
@vanbasten17 vanbasten17 deleted the contentful/cache-size-limit branch July 8, 2021 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation changes size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants