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

Issues with AbstractTagAwareAdapter #33924

Closed
nicolas-grekas opened this issue Oct 9, 2019 · 5 comments

Comments

@nicolas-grekas
Copy link
Member

commented Oct 9, 2019

I'm having a closer look at the implementation of AbstractTagAwareAdapter. Here are some notes for discussion (/cc @andrerom as the original author):

  • deleteItems() calls doFetch() to get access to the tags stored next to an item. Because the values are stored as ['value' => $item->value, 'tags' => [...]], it means the value is actually unserialized when only the tags are needed. This can be problematic (perf-wise at least).
    That's the reason why TagAwareAdapter stores them under separate keys in the storage. Can we do the same here?
  • doClear($namespace) doesn't clear the tags related to the items found in the namespace when one is provided. For FilesystemTagAwareAdapter that looks fixable. For RedisTagAwareAdapter I'm not sure how involving it could be. We could also consider this not worth fixing: the only side effect is that a value might be invalidated while it shouldn't (because it doesn't have the tag anymore).
  • why use symlinks in FilesystemTagAwareAdapter instead of a single file that lists all item files? Using files would allow fixing doInvalidate, which leaks inodes right now.
@javiereguiluz javiereguiluz added the Cache label Oct 9, 2019
@andrerom

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

That's the reason why TagAwareAdapter stores them under separate keys in the storage. Can we do the same here?

Unsure about code/logic implications here right now, but I think we can.

doClear($namespace) doesn't clear the tags related to ...

I think this was on purpose as it would eventually be consistent, but if we can fix it why not.

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2019

@andrerom about the 3 listed issues, would you be able to fix some of them soon enough for 4.4? (item 1 or 3 at least, 3 being the most critical)

@andrerom

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

I'm adding a third item: why use symlinks instead of a single file that lists all item files?

no specific reason, for more broad windows support it can be changed

@andrerom about the 3 listed issues, would you be able to fix some of them soon enough for 4.4?

no availability right now

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2019

File representing cache tag where you append cache keys?
Could do that, but then:

  • more work (with race) to clean up relationship when tags are removed from an item _(unless we continue to view that as eventually consistent)

eventually consistent would be fine to me

  • can it hit file limits if it only appends and never cares if cache key is already there?

dunno, I'd say we don't care :)
a bloom filter could help implementing some dedup logic, but let's wait for the issue to arise.

  • on invalidation probably need to rename like with redis to be able to iterate and clear without race possibilities

yep - the current approach already has the possible races, but renaming the directory would break on Windows that's why we don't do it, isn't it?

Would it be better than replacing symlinks with just simple files that contain the cache key as value?

Better maybe, by saving many inodes and fixing possible race conditions.

@andrerom

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

but renaming the directory would break on Windows that's why we don't do it, isn't it?

unsure

nicolas-grekas added a commit that referenced this issue Oct 10, 2019
…eAdapter::doDelete() (nicolas-grekas)

This PR was merged into the 4.3 branch.

Discussion
----------

[Cache] ignore unserialization failures in AbstractTagAwareAdapter::doDelete()

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Making things more robust, part of #33924

Commits
-------

a1f334c [Cache] ignore unserialization failures in AbstractTagAwareAdapter::doDelete()
nicolas-grekas added a commit that referenced this issue Oct 11, 2019
…e when using AbstractTagAwareAdapter (nicolas-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

[Cache] add TagAwareMarshaller to optimize data storage when using AbstractTagAwareAdapter

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | #33924
| License       | MIT
| Doc PR        | -

This is the final touch in my series of PR that fixes the linked issue.

Remarkably, the solutions I implemented for this issue are completely different than the one I described there. Fortunately, the issues themselves were correctly identified.

Plannification of implementation is gambling :)

/cc @andrerom

Commits
-------

5a4a30c [Cache] add TagAwareMarshaller to optimize data storage when using AbstractTagAwareAdapter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.