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

Use plone.memoize to cache groupby criteria #88

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

instification
Copy link
Member

@instification instification commented Sep 5, 2019

Fixes #87

PR replaces the current cache of the groupby criteria with a memory cache. It uses the sites ID and catalog.getCounter() to produce a cache key.

This solves the issues of:

  • New indexes/metadata being unavailable in the groupby dropdown
  • Indexes being cached across sites within a single plone instance

@instification
Copy link
Member Author

instification commented Sep 5, 2019

So it looks like there is no undoable_transactions or other zodb attributes on the portal_catalog object from 5.2 onwards (presumably as a result of moving to Zope 4.0).

I will see if there is a better way of finding out the last modified date of the catalog object.

…e cache_key. This method is not compatible with Plone 5.2/Zope 4.0.
@djay
Copy link
Member

djay commented Oct 3, 2019

@instification its not explained why you want the last modified of the catalog but there is a counter thats used as a cache key when anything is added or removed or reindexed. collective/plone.app.eventindex#2. That would seem the best thing to use as a key for groupby if its the unique values of a given index.

@instification
Copy link
Member Author

instification commented Oct 3, 2019

@instification the cache only needs to be invalidated if the structure of the index changes (ie a new index is added, modified or deleted) as opposed to being invalidated every time the contents of the index are changed.

The list in the collectionfilter portlet configuration is the list of available indexes on the site, so it doesn't need to be recomputed often, only when the indexes are modified.

I'm not sure if the counter you mention would increment on changes to the index structure?

@djay
Copy link
Member

djay commented Oct 3, 2019

@instification the id's and types of the index should be enough then. That should be cheap. I don't think any modified date for teh catalog would be kept up to date.
The counter I'm referring to would get updated too often (on any content change in the site)

@agitator
Copy link
Member

agitator commented Oct 3, 2019

@djay
Copy link
Member

djay commented Oct 4, 2019

@instification https://github.com/plone/Products.CMFPlone/blob/master/Products/CMFPlone/CatalogTool.py#L364 is the mentioned counter

@agitator @petschki I discussed with @instification. There doesn't seem to be any easy cache key which is cheaper than actually getting loading the catalog and getting the types and ids of the indexes. Unless this vocabulary is used multiple times per request then I don't see any point is caching it. The Catalog counter doesn't help as that changed when site content changes (often) rather than when the indexes are added/removed (rarely).

@djay
Copy link
Member

djay commented Oct 4, 2019

@instification looking at the code a bit more I can see the utility is called (without context which is problematic) in collectionfilter.py which is the traversal hook but thats already got protection to make sure its not called too many times. In addition it will be called multiple times if there are multiple portlets on the same page. Some maybe just cache it on the request. I can't see that it safes much caching it between requests. The only other alternative is caching it for a few hours as well as hooking into some events for adding and removing indexes (if they exist) but I don't think its worth it.

@instification
Copy link
Member Author

hi @petschki I finally got round to implementing the catalog counter as per your suggestion. I've merged from master & all tests are green. I also updated the description with a bit more context. Is it ok to get this merged & into the next release?

@djay djay requested a review from petschki November 17, 2021 03:34
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.

groupbycriteria utility cache spills information across sites
5 participants