Products.cmfplone: PLIP: assimilate collective.indexing

[MrTango]

Proposer : Gil Forcada

Seconder : Maik Derstappen

Abstract

Move all functionality on existing packages (Products.ZCatalog, Products.CMFCore and Products.CMFPlone) and apply all monkey patches directly to the packages they belong to, like Products.CMFCore or Products.Archetypes.

Motivation

Using collective.indexing has two main benefits:

• performance: indexing operations are queued, optimized and done at the transaction boundary. Only one indexing operation is done per object on any transaction. This means that plone is much faster, specially if lots of event handlers modify objects and keep reindexing the same object over and over again.
• pluggable: it creates a utility that enables third-party search indexers to be notified on any indexing operation (that's how collective.solr is plugged-in)

We think they are important and transparent enough that any Plone installation will benefit from it, specially for the performance part, although the pluggable makes even more easier to scale Plone as the site grows.

Assumptions

Some tests in Plone core will need to be updated to work around the indexing optimizations (as tests expect that objects are indexed/reindexed/unindexed right away).

No refactorings will be done on either CMFPlone CatalogTool nor CMFCore CatalogTool nor ZCatalog ZCatalog base class (which both CMFPlone and CMFCore override), this cleanup/refactoring is out of scope.

We deemed more importantly to get the functionality first in rather than merging the catalog tools together. At the same time we encourage any individual/company to step up and propose a PLIP that goes in that direction.

Proposal & Implementation

• [X] [plip config file on buildout.coredev](https://github.com/plone/buildout.coredev/blob/5.0/plips/merge-collective-indexing.cfg) to track the implementation and branches
• [X] [jenkins job](http://jenkins.plone.org/view/PLIPs/job/plip-collective-indexing) to test the plip config
• [x] move all functionality provided by collective.indexing to currently existing packages (Products.CMFCore and Products.CMFPlone)
• [x] the code of collective.indexing is now GPL and should be re-licensed by the PF-Board to ZPL before getting into CMFCore which are ZPL (@plone/plone-foundation-board)
• [x] add tests where the functionality moved
• [x] fix tests due to the way indexing is done with collective.indexing
• [x] documentation (describe the new feature, how to update tests, how to migrate from collective.indexing)
• [ ] collective.solr branch

• [X] deprecation warning on collective.indexing

  Deliverables

• CMFCore branch

• CMFPlone branch
• documentation on how to fix your tests and how the new functionality benefits Plone
• collective.solr branch (as is collective.indexing main consumer)
• new release of collective.indexing with deprecation warnings
  

  Risks

Some tests of 3th party packages can fail, but are easy to fix, with the processing_queue function which will be provided.

Participants

Gil Forcada
Maik Derstappen

[tomgross]

I'm +1 for this but propose a slightly different focus:

Provide a catalog / indexing API for which ZCatalog is the default implementation and Plone still ships with it but it can be easily repleced by another indexing solution (solr, pylucene, elasticsearch, ...) following the index / query / etc. API. It reads like batteries included but easily changeable.

Additional Deliverables:

Documentation of the API and example how to pull in a custom index

[gforcada]

@tomgross our main idea is to _drop_ the code in without major/any refactorings of any code. A follow up PLIP will be the one that can/should merge ZCatalog/CMFCore/CMFPlone code to make that a single code base without one patching the other in both directions.

Still, collective.indexing already allows external indexers (collective.solr at least) to be used.

[gforcada]

@MrTango @tomgross I updated the proposal

[gforcada]

@tomgross read this post from @hannosch back in 2010 to understand why is not possible/desirable to have a completely external catalog.

Full text searches (with faceted, and la la la), special indexes, etc etc can/should be of course made external to benefit from enterprise level search indexers (Solr, Elastic Search...).

At the same time, reducing the amount of metadata and indexes would help make things faster, see #1279 for that.

[bloodbare]

I really would love to hear the opinion of @vangheem about possibles issues with collective.elasticsearch

[gforcada]

@bloodbare already asked here https://github.com/collective/collective.elasticsearch/issues/10 but no reply so far :-(

I didn't dig into c.elasticsearch code but at least it's not using c.indexing at all...

[gforcada]

To test all pull requests together paste this on jenkins job:

https://github.com/zopefoundation/Products.CMFCore/pull/12
https://github.com/plone/Products.Archetypes/pull/57
https://github.com/plone/Products.CMFPlone/pull/1595
https://github.com/plone/plone.app.content/pull/91
https://github.com/plone/plone.app.upgrade/pull/75
https://github.com/plone/Products.CMFEditions/pull/37
https://github.com/plone/Products.ATContentTypes/pull/34
https://github.com/plone/Products.CMFUid/pull/3
https://github.com/plone/plone.app.blob/pull/27

[ale-rt]

Amazing :)

[gforcada]

Status for @plone/framework-team:

• Plone Foundation Board needs to approve re-licensing of code merged into CMFCore
• plone.app.multilingual may need some extra fixes, jenkins is currently running
• there are quite a few packages that are still being used also on 5.0 and do not have a specific branch for 5.1 and another one for 5.0 (or even 4.3). What should be done here?
• where should documentation go for this? is there any canonical place to document PLIPs? It would be great if we can compile them all together somewhere in documentation repository, one file per PLIP, so PLIPs could not be merged without a file there (which btw. @MrTango did some docs https://github.com/plone/documentation/pull/609 although I would like to document it more in the scope of 5.1 release)
• https://github.com/collective/collective.solr port is not done yet so far as it is still not plone 5 ready anyway, still either me or @MrTango will do the work once that porting effort is merged
• regarding https://github.com/collective/collective.elasticsearch I don't have any answer so far https://github.com/collective/collective.elasticsearch/issues/10 @vangheem

Apart from that code wise is ready for review. Thanks for your time

[thet]

For package branches - don't try hard to make it compatible with many Plone versions. If it's a generic bugfix, it should be compatible with as many Plone versions as possible, but for new features just create PRs against the master branch and create branches for the old Plone versions and use those in coredev.buildout sources.cfg files for the older Plone version. The following branch naming scheme turned out to be very useful: "MAJOR.MINOR.x", like "3.2.x".

[gforcada]

@thet @plone/framework-team new update:

• the Board relicensed collective.indexing to both ZPL and GPL (I'm still on talk with them to see what code/repository-wise means that)

Branches (following @thet advice) for plone.app.content, CMFEditions, plone.app.blob have been made and pushed commits to 5.0 and 4.3 on buildout.coredev accordingly. ATContentTypes master branch is already 5.1 only.

There are two packages that need to be branched for 4.3/5.0 and 5.1 still:

• CMFUid: both 5.0 and 5.1 target branch 2.2 although there is a 2.3 branch which seems to not be used... should we keep 2.2 branch for 5.0 and try to use this 2.3 for 5.1?
• plone.app.upgrade: no idea how to deal with this one, master is used by 4.3, 5.0 and 5.1, should I use conditional imports?

[jensens]

@gforcada awesome!

ad CMFUid - there is a beta release of 2.3 on PyPI - we should push a final here and go on with it in 5.1.

plone.app.upgrade is full of conditional imports, so yes, that is how it works in there.

[jensens]

btw. ... why do we need CMFUid? It looks like code from former times redundant to our UID implementations.

[gforcada]

@jensens no idea but I would love a PLIP to remove it if it's really not needed :-)

[gforcada]

New update:

• Products.CMFUid got bumped to 2.3 branch and the pull request was updated to it as well
• plone.app.upgrade pull request has been updated to use a conditional import

So I think it's already ready for a @plone/framework-team member to review and give feedback.

[tomgross]

Anything I can help to get this merged for Plone 5.1?

[gforcada]

@tomgross things on the top of my mind:

• a pull request on collective.solr updating its usage of collective.indexing to the new approach would be nice
• rebase all pull requests already made (see one comment of mine above)
• update some of those pull requests to add conditionals, so they can be kept compatible with 5.1 and 5.0

Is that enough work? :-)

[jensens]

Merging involves these tasks and packages:

• [x] Framework Team review
• [x] https://github.com/zopefoundation/Products.CMFCore/pull/12
• [x] https://github.com/plone/Products.Archetypes/pull/57
• [x] https://github.com/plone/Products.CMFPlone/pull/1595
• [x] https://github.com/plone/plone.app.content/pull/91
• [x] https://github.com/plone/plone.app.upgrade/pull/75
• [x] https://github.com/plone/Products.CMFEditions/pull/37
• [x] https://github.com/plone/Products.ATContentTypes/pull/34
• [x] https://github.com/plone/Products.CMFUid/pull/3
• [x] https://github.com/plone/plone.app.blob/pull/27
• [x] https://github.com/plone/collective.indexing/pull/12
• [x] According to @polyester relicensing issues are solved.
• [x] open question: CMFCore is a fork in plone organisation. How is it supposed to be released? /cc @esteele

[gforcada]

@jensens first it needs someone from the @plone/framework-team to actually review it, I haven't seen such a review so far

[jensens]

sure, i just added a checklist, this was not meant to be merged without FWT review. I add an item for this to the list.

[gforcada]

@plone/framework-team I rebased all branches used and the PLIP job is running at http://jenkins.plone.org/view/PLIPs/job/plip-collective-indexing/1/ so a review would be great so it could be in before 5.1 hits beta

[gforcada]

This one should be green: http://jenkins.plone.org/view/PLIPs/job/plip-collective-indexing/2/

[jensens]

I reviewed the PLIP:
https://github.com/plone/buildout.coredev/blob/5.1/plips/reviews/plip1343-review-jensens.rst

[gforcada]

@jensens thanks for the review! Actually there are some docs, although I will go over them again and improve them a little more https://github.com/plone/documentation/pull/609

[jensens]

despite the issue on how we proceed with Products.CMFCore, this is ready for merge.

@esteele may you elaborate on this?

[gforcada]

@plone/framework-team status update:

• a new pull request has been created on zopefoundation org: https://github.com/zopefoundation/Products.CMFCore/pull/12
• docs have been updated and expanded: https://github.com/plone/documentation/pull/609
• all branches have been rebased to master
• PLIP job run without an error: http://jenkins.plone.org/view/PLIPs/job/plip-collective-indexing/3

[jensens]

FYI: Yesterday I addressed some of the CMFCore issues and I also rebased all branches.

[jensens]

All merged, we just need to bring the CMFCore 2.2 changes into CMFCore master, right?

[ebrehault]

@jensens, isn't it precisely what @esteele made during our last meeting?

[jensens]

No they are only on the 2.2 branch we use in Plone, master still needs them
https://github.com/zopefoundation/Products.CMFCore/commits/master

[gforcada]

I will try to make the pull request

[gforcada]

Done: https://github.com/zopefoundation/Products.CMFCore/pull/15 now someone needs to merge it and the zope4 branch be rebased on top :-)