Don't delay cache additions until after the transaction ends #3172
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I have a fix for the |
reosarevok
reviewed
Feb 13, 2024
The methods in question were copied from the `GIDRedirect` role in 8a841d6, but I believe we can just use that role directly.
mwiencek
force-pushed
the
mbs-13449-no-cache-add-delay
branch
2 times, most recently
from
d43657e
to
25bb8b7
Compare
This will fix test failures in a subsequent commit. It appears that we don't currently do this, but issues did not arise in the tests due to cache additions being delayed (i.e., there being nothing to invalidate).
`automatically_reorder` (invoked by `reorder_for_entities`) sorts series items by "part of" relationship, which are loaded via our Perl data models, which in turn will load entities on those relationships. Those entities may have been updated in the current transaction and thus should be invalidated from the cache before we attempt to sort the series items based on stale entity data. (This will fix test failures in a subsequent commit.)
This partially reverts 82c79f5 and 42be4bc. After searching for issues similar to MBS-13481 (where we load additional data onto an entity which is then entered into the cache, but where changes to that additional data do not invalidate the entity's cache entry), I believe that there are simply far too many uncovered cases which we'd have to handle. While storing more data in the cache and having intelligent invalidation logic for all the linked data sounds desirable, it's too many changes to manage for now, so we need to restore the previous behavior. I realized (later than I should have) that we don't need to delay cache additions until after the database transaction ends. We may have had to in a previous implementation I had, but no longer. The `post_txn_callbacks` ran regardless of whether or not the commit succeeded, so an entity would always be cached in the state it was initially loaded in. The only difference now is when that occurs. There's no type of race condition that isn't already handled: * If the entity is deleted from the cache right after it's added, then there is no issue, just like before. * If the entity is added to the cache right after it's deleted, then there is still no issue, because our existing code to delete the added entry if there exists a `recently_invalidated` key for the same still applies. The existing `*EntityCache` tests pass, and I've added a couple additional tests to ensure that neither release events nor release labels are entered into the cache with a release.
mwiencek
force-pushed
the
mbs-13449-no-cache-add-delay
branch
from
25bb8b7
to
49fa867
Compare
yvanzo
approved these changes
Feb 16, 2024
There was a problem hiding this comment.
Thanks for the in-depth investigation of this cache issue even though it doesn’t come to the desired improvements. I agree that we cannot manage such changes at the moment.
Changes look good, commit messages are very explanatory, and I also quickly tested the website locally just in case.
Merging and deploying to both beta and production as requested on IRC. 🚢
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
After searching for issues similar to MBS-13481 (where we load additional data onto an entity which is then entered into the cache, but where changes to that additional data do not invalidate the entity's cache entry), I believe that there are simply far too many uncovered cases which we'd have to handle.
While storing more data in the cache and having intelligent invalidation logic for all the linked data sounds desirable, it's too many changes to manage for now, so we need to restore the previous behavior.
Solution
This partially reverts 82c79f5 and 42be4bc.
I realized (later than I should have) that we don't need to delay cache additions until after the database transaction ends. We may have had to in a previous implementation I had, but no longer.
The
post_txn_callbacksran regardless of whether or not the commit succeeded, so an entity would always be cached in the state it was initially loaded in. The only difference now is when that occurs. There's no type of race condition that isn't already handled:If the entity is deleted from the cache right after it's added, then there is no issue, just like before.
If the entity is added to the cache right after it's deleted, then there is still no issue, because our existing code to delete the added entry if there exists a
recently_invalidatedkey for the same still applies.Testing
The existing
*EntityCachetests pass, and I've added a couple additional tests to ensure that neither release events nor release labels are entered into the cache with a release.