[Cache] TagAwareAdapterInterface::invalidateTags() should commit deferred items #27007
Conversation
This file contains hidden or 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
|
ping @Nyholm FYI |
ce30bd4 to
f538f57
Compare
fabpot
reviewed
Apr 27, 2018
| $foo->tag('tag'); | ||
|
|
||
| $pool1->saveDeferred($foo->set('foo')); | ||
| $pool1->invalidateTags(array('foo')); |
There was a problem hiding this comment.
I'm confused, 'foo' should be 'tag' here, right?
There was a problem hiding this comment.
good catch! fixed
f538f57 to
212c469
Compare
nicolas-grekas
added a commit
that referenced
this pull request
Apr 29, 2018
…commit deferred items (nicolas-grekas) This PR was merged into the 3.4 branch. Discussion ---------- [Cache] TagAwareAdapterInterface::invalidateTags() should commit deferred items | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - I noticed that we have a race condition window when invalidating tags. This PR fixes it by making `invalidateTags()` commit deferred items before actually invalidating tags on the back end. Deferred items are stored with the future version of the invalidated tags so that they can be fetched by concurrent processes and considered valid before and after the invalidation is committed. Commits ------- 212c469 [Cache] TagAwareAdapterInterface::invalidateTags() should commit deferred items
nicolas-grekas
added a commit
that referenced
this pull request
May 5, 2018
…pter (dmaicher) This PR was squashed before being merged into the 3.4 branch (closes #27158). Discussion ---------- [Cache] fix logic for fetching tag versions on TagAwareAdapter | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #27147 | License | MIT | Doc PR | - There was a problem introduced in #27007 which breaks tag invalidation. From what I can see there were some cases when the actual tag versions were never fetched from the tags pool and version=0 was used. @nicolas-grekas this is my attempt of understanding the logic within `TagAwareAdapter`. Please have a look if this makes sense to you 😉 Commits ------- d3790ca [Cache] fix logic for fetching tag versions on TagAwareAdapter
| { | ||
| $this->pool = $itemsPool; | ||
| $this->tags = $tagsPool ?: $itemsPool; | ||
| $this->knownTagVersionsTtl = $knownTagVersionsTtl; |
There was a problem hiding this comment.
How should we use this property when we always want to fetch the tag versions?
There was a problem hiding this comment.
doesn't 0 work? or is the check L351 wrong? PR welcome if you found a bug.
There was a problem hiding this comment.
0 didn't fix it for me, seems like the check is wrong.
Filling in 1000 did work.
I'll look into it a bit better.
There was a problem hiding this comment.
I've made a PR for this #33754
nicolas-grekas
added a commit
that referenced
this pull request
Sep 30, 2019
This PR was merged into the 3.4 branch. Discussion ---------- [Cache] fix known tag versions ttl check | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | none | License | MIT The introduced changes from PR #27007 came with a knownTagVersionTtl property defaulted to 0.15 microseconds. The if statement defined on line 353 checks if the ttl is higher then the already known tag versions. This results in the opposite of the wanted behavior. Instead of waiting for the ttl to expire the if statement is only true if the known versions are lower than the version ttl. So if this Adapter stays in memory the tag versions are never checked again if the ttl is already lower then the passed time. The unit test I've added tests once if the version is changed and another time it doesn't get checked cause the ttl hasn't been expired yet. Commits ------- 205abf3 [Cache] fix known tag versions ttl check
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.
I noticed that we have a race condition window when invalidating tags.
This PR fixes it by making
invalidateTags()commit deferred items before actually invalidating tags on the back end. Deferred items are stored with the future version of the invalidated tags so that they can be fetched by concurrent processes and considered valid before and after the invalidation is committed.