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

feat(contentful): memoizer #BOT-467 #1639

Merged
merged 5 commits into from
Jun 18, 2021
Merged

feat(contentful): memoizer #BOT-467 #1639

merged 5 commits into from
Jun 18, 2021

Conversation

dpinol
Copy link
Contributor

@dpinol dpinol commented Jun 14, 2021

Depends on #1640. ⚠️ Please review it first

Description

Use memoization to remember forever the last successful invocation for each combination of arguments, and use its result whenever Contentful fails.

Context

Contentful failing during 1h on 2021/6/8 and 2021/6/9.

Approach taken / Explain the design

  • Create a generic Memoizer class with several strategies.
  • Create a decorator (FallbackCachedClientApi) around the Contentful API class which uses the Memoizer fallbackStrategy.
  • Compose the new decorator within the existing decorator (CachedClientApi) which avoids recent calls.

FallbackCachedClientApi does not use the memoization library used by CachedClientApi (memoizee) because this library does not allow directly accessing the cache.

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 disable this feature, use to true the new plugin option beloew

  /**
   * By default, the result of the last delivery invocation will be cached
   * forever and will only be used when a delivery call with the same arguments
   * fail.
   */
  disableFallbackCache?: boolean

Testing

The pull request...

  • has unit tests

@dpinol dpinol force-pushed the contentful/memoizer branch from 2b51f0d to f454e41 Compare June 14, 2021 14:54
@dpinol dpinol force-pushed the contentful/memoizer branch from f454e41 to 313d221 Compare June 14, 2021 14:58
@dpinol dpinol changed the base branch from master to contentful/cache-forget-errors June 14, 2021 15:07
@dpinol dpinol changed the title feat(contentful): memoizer feat(contentful): memoizer #BOT-467 Jun 14, 2021
@linear
Copy link

linear bot commented Jun 14, 2021

BOT-467 mitigate Contentful outages with inmemory cache

In 2021/6/8 & 2021/6/9 contentful was down during 1h, which caused all our bots using this CMS to stop working.

We'll start with a simple memoization (lazy in memory cache)

In the future maybe we can:

  • Use redis
  • Preload all contents when starting the bot instance
  • Load the contents into the bot before deploying

@dpinol dpinol marked this pull request as ready for review June 14, 2021 15:18
export function fallbackStrategy(
usingFallback: (functName: string, args: any[], error: any) => Promise<void>
): MemoizerStrategy {
return async <Args extends any[], Return>(
Copy link
Contributor

Choose a reason for hiding this comment

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

do you no need to put again extends any[] in this type? I understand that Args will extend it as you have typed it as MemoizerStrategy, could be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I typed fallbackStrategy as MemoizerStrategy for the compiler to verify that the lambda I retry complies with this type, but the lambda is not defined to be of this type (generics arguments must always be explicit, otherwise changing their names in the definition would break all usages)

func: (...args: Args) => Promise<Return>,
...args: Args
) => {
// return func(...args)
Copy link
Contributor

Choose a reason for hiding this comment

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

delete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, done in ee9efdb

const cache: Cache<Return> = this.cacheFactory()
const f = (...args: Args) =>
this.strategy<Args, Return>(cache, this.normalizer, func, ...args)
// f.cache = cache
Copy link
Contributor

Choose a reason for hiding this comment

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

delete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, done in ee9efdb

@dpinol dpinol force-pushed the contentful/memoizer branch from 51bcd2b to 7ff7245 Compare June 16, 2021 07:36
@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.

👏 👏 Just one doubt in the tests

Base automatically changed from contentful/cache-forget-errors to master June 18, 2021 08:58
@dpinol dpinol force-pushed the contentful/memoizer branch from 7ff7245 to 63dca40 Compare June 18, 2021 09:10
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link

github-actions bot commented Jun 18, 2021

Unit Test Results

  1 files    7 suites   3m 0s ⏱️
32 tests 32 ✔️ 0 💤 0 ❌

Results for commit 63dca40.

♻️ This comment has been updated with latest results.

to avoid docusaurus issue
@dpinol dpinol merged commit 050841a into master Jun 18, 2021
@dpinol dpinol deleted the contentful/memoizer branch June 18, 2021 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation changes size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants