Optimization: Don't reindex containers when their contents are modified

[davisagli]

Dexterity content is currently reindexed whenever there is an object modified event. That includes the ContainerModifiedEvent fired when contents of a container are added/removed. Let's see if we can avoid reindexing in that case...

This is NOT a fully backwards compatible change:

• Some sites may have custom indexes or catalog metadata that calculates a value based on the contents of a container.
• It means a container's modification time will not be updated when a contained item is added or removed. (That may be a good change actually, but it's certainly different.)

So I fully expect this to break some tests and I'm not sure if it is a good idea or not. But I want to at least run the tests and see where we're at.

If it does turn out to be okay, we should get it in soon or else wait for Plone 7.

[mister-roboto]

@davisagli thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

[davisagli]

@jenkins-plone-org please run jobs

[davisagli]

The py3.8 tests only show 3 failures which look like off-by-one object counts that seem easy to fix (I guess this change means the portal is no longer indexed during these tests).

The py3.7 and py3.9 tests hit a POSKeyError during setup of the Plone fixture; I'm not sure what's up there.

[jensens]

I had this idea too already, but were that sure it will blow up something, so I did not investigate further. For now, it seems it doesn't and my assumption was wrong.

[jensens]

I think we could merge this for 6.1.
@mauritsvanrees what do you think?

[davisagli]

@jenkins-plone-org please run jobs

[rafaelbco]

@davisagli what are the benefits of this change?

I suppose performance gains. But how big is the impact? I think that if the item is already being reindexed then its parent being reindexed is not a big deal, or is it?

This is not a strong opinion, I am just curious. Thanks!

[davisagli]

@rafaelbco Reindexing is an expensive operation in terms of performance. If we can index fewer objects, that helps the overall speed of updating an item. I don't have actual measurements of the impact.

[davisagli]

@jensens It's a bit hard to predict possible side effects of this change. I'd be more comfortable merging it as one of the first changes in 6.2 or 7.0, rather than as one of the last changes in 6.1.

[wesleybl]

There was a discussion about this at:

plone/plone.volto#118 (comment)

[mauritsvanrees]

This could disrupt some use cases, like some caching strategies, and what Wesley points to.
It makes sense, but indeed it could be better for Plone 7.
Or keep the old behavior by default, but allow to opt-in to the new behavior with an environment variable.

[jensens]

Or keep the old behavior by default, but allow to opt-in to the new behavior with an environment variable.

This would be a good solution.