Improve performance for calculating breaches #101
Conversation
|
@pgrunewald thanks for creating this Pull Request and helping to improve Plone! TL;DR: Finish pushing changes, pass all other checks, then paste a comment: 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! |
| @@ -67,7 +69,9 @@ def get_breaches(self, items=None): | |||
| "No object found for %s! Skipping", brain_to_delete | |||
| ) | |||
| continue | |||
| for breach in self.get_breaches_for_item(obj): | |||
There was a problem hiding this comment.
I'm not sure if I read the code correctly but shouldn't this check the breach of the obj_to_delete instead of the obj ?
There was a problem hiding this comment.
No, because get_breaches_for_item does collect all the breaches for all the children, see https://github.com/plone/plone.app.linkintegrity/blob/master/plone/app/linkintegrity/browser/info.py#L138-L139.
But then again, obj_to_delete does not really do anything here. Now looking at it, there could be more improvements implemention-wise.
There was a problem hiding this comment.
ouch ... I think there is something like "depth": 1 missing in the get_breaches_for_item catalog path query ... otherwise the recursive lookup does things multiple times for the same folder.
There was a problem hiding this comment.
Oh, I see. Looking up the same items multiple times is really unnecessary. I will rewrite those parts even further.
There was a problem hiding this comment.
@petschki I have rewritten a good chunk of the code, while maintaining backwards compatibility. In the Plone code base I just found usages for get_breaches. In case anyone uses in their 3rd party add-on the other methods, they should still be able to do so.
|
@jenkins-plone-org please run jobs |
By the way the code runs now, this is not needed anymore.
See #100