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: secondary cache loader #123

Merged
merged 3 commits into from
May 26, 2021
Merged

feat: secondary cache loader #123

merged 3 commits into from
May 26, 2021

Conversation

wschurman
Copy link
Member

@wschurman wschurman commented Apr 3, 2021

Why

This PR adds a concept, EntitySecondaryCacheLoader that allows for arbitrary entity caching and retrieval based on arbitrary load params and configurable backing store load.

The idea is that there are cases in which the standard entity caching mechanism can't be used, generally loads using loadManyByFieldEqualityConjunctionAsync or loadManyByRawWhereClauseAsync (both with limit 1), that a cache would still be useful; most commonly hot/critical paths where DB indexes aren't sufficient.

This PR adds a general purpose read-through cache for an arbitrary load of a single entity field object by any means, which then constructs and authorizes that entity as normal. Note that cache invalidation cannot be inferred for this cacher, so it must be done manually in consumer code.

For example, Expo needs to load the most recent manifest for an app. To do this, loadManyByFieldEqualityConjunctionAsync with an order by timestamp and limit 1 is used. With this PR, a custom EntitySecondaryCacheLoader can be created and used at the callsite, and it can be invalidated manually when appropriate (when new manifest is written to the DB).

Test Plan

Run all tests (including some new ones).

@wschurman wschurman force-pushed the @wschurman/secondary-cache branch 4 times, most recently from 43a651d to c63d67a Compare May 13, 2021 04:47
@codecov
Copy link

codecov bot commented May 13, 2021

Codecov Report

Merging #123 (ec8de9f) into master (d78f452) will increase coverage by 0.20%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #123      +/-   ##
==========================================
+ Coverage   95.10%   95.31%   +0.20%     
==========================================
  Files          67       70       +3     
  Lines        1697     1772      +75     
  Branches      183      208      +25     
==========================================
+ Hits         1614     1689      +75     
  Misses         81       81              
  Partials        2        2              
Flag Coverage Δ
integration 95.31% <100.00%> (+0.20%) ⬆️
unittest 95.31% <100.00%> (+0.20%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ages/entity/src/internal/ReadThroughEntityCache.ts 95.45% <ø> (ø)
...tity-cache-adapter-redis/src/GenericRedisCacher.ts 100.00% <100.00%> (ø)
...ntity-cache-adapter-redis/src/RedisCacheAdapter.ts 100.00% <100.00%> (ø)
...ndary-cache-redis/src/RedisSecondaryEntityCache.ts 100.00% <100.00%> (ø)
packages/entity/src/EntityLoader.ts 88.88% <100.00%> (+0.12%) ⬆️
packages/entity/src/EntitySecondaryCacheLoader.ts 100.00% <100.00%> (ø)
packages/entity/src/utils/collections/maps.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d78f452...ec8de9f. Read the comment docs.

@wschurman wschurman marked this pull request as ready for review May 13, 2021 18:05
@wschurman wschurman requested review from ide and jkhales May 13, 2021 18:05
Copy link

@jkhales jkhales left a comment

Choose a reason for hiding this comment

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

Very exciting!

Copy link
Member

@ide ide left a comment

Choose a reason for hiding this comment

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

In a subset of cases where we have unique composite keys (UNIQUE (a, b)), we could tell the Entity framework that the pair (a, b) is cacheable and have the cache invalidated when the same way single-field keys get their cache entries invalidated. I think this autoinvalidation could be built on top of the more flexible, generic secondary cacher in this PR.

(Autoinvalidation would not work for the case like "ORDER BY created_at DESC LIMIT 1" where there is no unique key.)

I think it is good the secondary cache has a separate TTL. We always can pass in the same TTL as the main cache adapter context.

@wschurman
Copy link
Member Author

wschurman commented May 25, 2021

In a subset of cases where we have unique composite keys (UNIQUE (a, b)), we could tell the Entity framework that the pair (a, b) is cacheable and have the cache invalidated when the same way single-field keys get their cache entries invalidated. I think this autoinvalidation could be built on top of the more flexible, generic secondary cacher in this PR.

(Autoinvalidation would not work for the case like "ORDER BY created_at DESC LIMIT 1" where there is no unique key.)

To accomplish this I think a generalized approach would be better than separate implementation. I could see it being done by factoring out what already exists in the loader into things like loadByFieldsCompositeKey(Tuple<FieldName, FieldValue>[]) and then make loadByFieldEqualing the n=1 case that calls into it (and doing similar conversions for other existing methods).

The tough thing will be figuring out what the set of keys to auto-invalidate are:

  • Columns: a, b, c
  • User specifies a, b as composite key
  • User updates column b to new value, we need to invalidate the cartesian product of old and new sub-column values (though I haven't verified this yet)

Copy link
Member

@ide ide left a comment

Choose a reason for hiding this comment

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

The explicit nulls for lookup misses hopefully will be useful, especially from a typechecking perspective.

@wschurman wschurman merged commit 4cba01e into master May 26, 2021
@wschurman wschurman deleted the @wschurman/secondary-cache branch May 26, 2021 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants