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

fix: move cache invalidation to after transaction commit #77

Merged
merged 4 commits into from
Aug 25, 2020

Conversation

wschurman
Copy link
Member

@wschurman wschurman commented Aug 19, 2020

Why

Implementation of second idea from #76.

How

  • Append bound async functions to array of fns that are executed after transaction has been committed.
  • Move general entity integration tests into separate package that integrate all cache and database adapters.
  • Write a test that fails with the previous implementation and passes with the new implementation (EntityCacheInconsistency-test). This test runs an update in a long-running transaction and does a single targeted load between when the cache is invalidated (in the old implementation) and when the transaction completes. With the old implementation, the cache would have been cleared before the change was visible to other machines. With the new implementation, the cache is cleared at commit time (right after commit). Technically there still could be a race if the loads of a single entity are super frequent and an update occurs but that problem can't be solved without a distributed reader-writer lock which at this point is probably overkill.

Test Plan

Run new test.

@wschurman wschurman force-pushed the @wschurman/deferred-after-commit-fns branch from 3e946ab to 47ad5a3 Compare August 19, 2020 17:43
@wschurman wschurman force-pushed the @wschurman/deferred-after-commit-fns branch from 47ad5a3 to 8c56ce5 Compare August 19, 2020 17:44
@codecov
Copy link

codecov bot commented Aug 19, 2020

Codecov Report

Merging #77 into master will increase coverage by 0.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #77      +/-   ##
==========================================
+ Coverage   94.48%   94.61%   +0.12%     
==========================================
  Files          59       60       +1     
  Lines        1506     1522      +16     
  Branches      171      181      +10     
==========================================
+ Hits         1423     1440      +17     
+ Misses         81       80       -1     
  Partials        2        2              
Flag Coverage Δ
#integration 94.61% <100.00%> (+0.12%) ⬆️
#unittest 94.61% <100.00%> (+0.12%) ⬆️

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

Impacted Files Coverage Δ
packages/entity/src/EntityCompanionProvider.ts 100.00% <ø> (ø)
...ter-knex/src/PostgresEntityQueryContextProvider.ts 100.00% <100.00%> (ø)
...ample/src/adapters/InMemoryQueryContextProvider.ts 100.00% <100.00%> (ø)
packages/entity/src/EntityCompanion.ts 100.00% <100.00%> (ø)
...s/entity/src/EntityMutationTriggerConfiguration.ts 100.00% <100.00%> (ø)
packages/entity/src/EntityMutator.ts 98.00% <100.00%> (-0.03%) ⬇️
packages/entity/src/EntityQueryContext.ts 100.00% <100.00%> (+5.55%) ⬆️
packages/entity/src/EntityQueryContextProvider.ts 100.00% <100.00%> (ø)
packages/entity/src/ViewerScopedEntityCompanion.ts 100.00% <100.00%> (ø)
packages/entity/src/internal/EntityDataManager.ts 100.00% <100.00%> (ø)
... and 3 more

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 62bfd6c...133b774. Read the comment docs.

@wschurman wschurman force-pushed the @wschurman/deferred-after-commit-fns branch from 895db7d to f7d2297 Compare August 19, 2020 19:40
@wschurman wschurman requested a review from ide August 19, 2020 19:42
@wschurman wschurman marked this pull request as ready for review August 19, 2020 19:44
@wschurman
Copy link
Member Author

One thing to figure out is if we want these invalidations to occur right before or right after the transaction commit.

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.

Really like the regression test.

@wschurman wschurman merged commit dbc3c81 into master Aug 25, 2020
@wschurman wschurman deleted the @wschurman/deferred-after-commit-fns branch August 25, 2020 18:16
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.

2 participants