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

PLIP: assimilate collective.indexing #1343

Closed
MrTango opened this issue Jan 27, 2016 · 32 comments
Closed

PLIP: assimilate collective.indexing #1343

MrTango opened this issue Jan 27, 2016 · 32 comments

Comments

@MrTango
Copy link
Contributor

@MrTango MrTango commented Jan 27, 2016

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

  • plip config file on buildout.coredev to track the implementation and branches
  • jenkins job to test the plip config
  • move all functionality provided by collective.indexing to currently existing packages (Products.CMFCore and Products.CMFPlone)
  • 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)
  • add tests where the functionality moved
  • fix tests due to the way indexing is done with collective.indexing
  • documentation (describe the new feature, how to update tests, how to migrate from collective.indexing)
  • collective.solr branch
  • 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

@MrTango MrTango added this to the Future milestone Jan 27, 2016
@hvelarde hvelarde changed the title PLIP: PLIP: assimilate collective.indexing PLIP: assimilate collective.indexing Jan 27, 2016
@tomgross
Copy link
Member

@tomgross tomgross commented Jan 27, 2016

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
Copy link
Contributor

@gforcada gforcada commented Jan 27, 2016

@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
Copy link
Contributor

@gforcada gforcada commented Jan 27, 2016

@MrTango @tomgross I updated the proposal

@gforcada
Copy link
Contributor

@gforcada gforcada commented Jan 27, 2016

@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.

gforcada added a commit that referenced this issue Jan 28, 2016
That's the minimal patching that collective.indexing was performing
into Products.CMFPlone.

Part of PLIP #1343.
gforcada added a commit that referenced this issue Jan 28, 2016
As indexing now is defered at the transaction boundaries instead of
being done right away, some tests need to be adapted to it.

Part of PLIP #1343.
gforcada added a commit that referenced this issue Feb 21, 2016
That's the minimal patching that collective.indexing was performing
into Products.CMFPlone.

Part of PLIP #1343.
gforcada added a commit that referenced this issue Feb 21, 2016
As indexing now is defered at the transaction boundaries instead of
being done right away, some tests need to be adapted to it.

Part of PLIP #1343.
gforcada added a commit to plone/Products.CMFUid that referenced this issue Feb 21, 2016
With PLIP 1343 being merged some tests need to be adapted.

Now the catalog no longer processes indexing operations right away.

The utility that processes the indexing operations was not registered.

References:
plone/Products.CMFPlone#1343
gforcada added a commit to plone/plone.app.blob that referenced this issue Feb 21, 2016
With PLIP 1343 being merged some tests need to be adapted.

Now the catalog no longer processes indexing operations right away.

Some tests rely on that, so a minimal adaptation was needed.

References:
plone/Products.CMFPlone#1343
gforcada added a commit to plone/Products.ATContentTypes that referenced this issue Feb 21, 2016
With PLIP 1343 being merged some tests need to be adapted.

Now the catalog no longer processes indexing operations right away.

Some tests rely on that, so a minimal adaptation was needed.

References:
plone/Products.CMFPlone#1343
gforcada added a commit to plone/Products.CMFEditions that referenced this issue Feb 21, 2016
With PLIP 1343 being merged some tests need to be adapted.

Now the catalog no longer processes indexing operations right away.

Some tests rely on that, so a minimal adaptation was needed.

References:
plone/Products.CMFPlone#1343
gforcada added a commit to plone/plone.app.multilingual that referenced this issue Feb 21, 2016
With PLIP 1343 being merged some tests need to be adapted.

Now the catalog no longer processes indexing operations right away.

Some tests rely on that, so a minimal adaptation was needed.

References:
plone/Products.CMFPlone#1343
gforcada added a commit to plone/plone.app.upgrade that referenced this issue Feb 21, 2016
With PLIP 1343 being merged some tests need to be adapted.

Now the catalog no longer processes indexing operations right away.

Some tests rely on that, so a minimal adaptation was needed.

References:
plone/Products.CMFPlone#1343
gforcada added a commit to plone/plone.app.content that referenced this issue Feb 21, 2016
With PLIP 1343 being merged some tests need to be adapted.

Now the catalog no longer processes indexing operations right away.

Some tests rely on that, so a minimal adaptation was needed.

References:
plone/Products.CMFPlone#1343
gforcada added a commit to plone/collective.indexing that referenced this issue Feb 26, 2016
If PLIP 1343 gets merged a new release of collective.indexing needs to be done
with this commit merged.

It displays a warning on bin/instance fg so that users know that they should
move away from collective.indexing as soon as they update to Plone 5.X.Y.

References:
plone/Products.CMFPlone#1343
gforcada added a commit that referenced this issue Feb 26, 2016
That's the minimal patching that collective.indexing was performing
into Products.CMFPlone.

Part of PLIP #1343.
gforcada added a commit that referenced this issue Feb 26, 2016
As indexing now is defered at the transaction boundaries instead of
being done right away, some tests need to be adapted to it.

Part of PLIP #1343.
gforcada added a commit to plone/Products.ATContentTypes that referenced this issue Feb 26, 2016
With PLIP 1343 being merged some tests need to be adapted.

Now the catalog no longer processes indexing operations right away.

Some tests rely on that, so a minimal adaptation was needed.

References:
plone/Products.CMFPlone#1343
gforcada added a commit to plone/plone.app.content that referenced this issue Feb 26, 2016
With PLIP 1343 being merged some tests need to be adapted.

Now the catalog no longer processes indexing operations right away.

Some tests rely on that, so a minimal adaptation was needed.

References:
plone/Products.CMFPlone#1343
gforcada added a commit to plone/plone.app.multilingual that referenced this issue Feb 27, 2016
With PLIP 1343 being merged some tests need to be adapted.

Now the catalog no longer processes indexing operations right away.

Some tests rely on that, so a minimal adaptation was needed.

References:
plone/Products.CMFPlone#1343
@bloodbare
Copy link
Member

@bloodbare bloodbare commented Mar 2, 2016

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

@gforcada
Copy link
Contributor

@gforcada gforcada commented Mar 2, 2016

@bloodbare already asked here collective/collective.elasticsearch#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 added a commit to plone/collective.indexing that referenced this issue May 14, 2016
If PLIP 1343 gets merged a new release of collective.indexing needs to be done
with this commit merged.

It displays a warning on bin/instance fg so that users know that they should
move away from collective.indexing as soon as they update to Plone 5.X.Y.

References:
plone/Products.CMFPlone#1343
mister-roboto pushed a commit to plone/buildout.coredev that referenced this issue Feb 21, 2017
Branch: refs/heads/2.2
Date: 2016-11-07T23:23:54+01:00
Author: Gil Forcada (gforcada) <gforcada@gnome.org>
Commit: plone/Products.CMFUid@c689dfc

Process indexing operations

Part of PLIP: plone/Products.CMFPlone#1343

The utility that processes the indexing operations was not registered.

Files changed:
M Products/CMFUid/CHANGES.txt
M Products/CMFUid/tests/test_uidhandling.py
Repository: Products.CMFUid
Branch: refs/heads/2.2
Date: 2017-02-21T22:03:40+01:00
Author: Gil Forcada (gforcada) <gil.gnome@gmail.com>
Commit: plone/Products.CMFUid@f76c84c

Conditional import

This way,
the package can be used in both 5.1 (with collective.indexing merged in CMFCore)
and with 5.0 without it.

Files changed:
M Products/CMFUid/tests/test_uidhandling.py
Repository: Products.CMFUid
Branch: refs/heads/2.2
Date: 2017-02-21T22:04:50+01:00
Author: Gil Forcada Codinachs (gforcada) <gil.gnome@gmail.com>
Commit: plone/Products.CMFUid@39b9ffa

Merge pull request #3 from plone/merge-collective-indexing

Merge collective indexing

Files changed:
M Products/CMFUid/CHANGES.txt
M Products/CMFUid/tests/test_uidhandling.py
gforcada added a commit to plone/plone.app.blob that referenced this issue Feb 21, 2017
mister-roboto pushed a commit to plone/buildout.coredev that referenced this issue Feb 21, 2017
Branch: refs/heads/master
Date: 2017-02-21T22:14:58+01:00
Author: Gil Forcada (gforcada) <gforcada@gnome.org>
Commit: plone/plone.app.blob@e85355f

Process indexing operations

Part of PLIP: plone/Products.CMFPlone#1343

Files changed:
M CHANGES.rst
M src/plone/app/blob/tests/test_maintenance.py
Repository: plone.app.blob
Branch: refs/heads/master
Date: 2017-02-21T22:15:55+01:00
Author: Gil Forcada Codinachs (gforcada) <gil.gnome@gmail.com>
Commit: plone/plone.app.blob@efcbd7b

Merge pull request #27 from plone/merge-collective-indexing

Merge collective indexing

Files changed:
M CHANGES.rst
M src/plone/app/blob/tests/test_maintenance.py
mister-roboto pushed a commit to plone/buildout.coredev that referenced this issue Feb 21, 2017
Branch: refs/heads/master
Date: 2017-02-21T22:14:58+01:00
Author: Gil Forcada (gforcada) <gforcada@gnome.org>
Commit: plone/plone.app.blob@e85355f

Process indexing operations

Part of PLIP: plone/Products.CMFPlone#1343

Files changed:
M CHANGES.rst
M src/plone/app/blob/tests/test_maintenance.py
Repository: plone.app.blob
Branch: refs/heads/master
Date: 2017-02-21T22:15:55+01:00
Author: Gil Forcada Codinachs (gforcada) <gil.gnome@gmail.com>
Commit: plone/plone.app.blob@efcbd7b

Merge pull request #27 from plone/merge-collective-indexing

Merge collective indexing

Files changed:
M CHANGES.rst
M src/plone/app/blob/tests/test_maintenance.py
mister-roboto pushed a commit to plone/buildout.coredev that referenced this issue Feb 21, 2017
Branch: refs/heads/master
Date: 2017-02-20T12:40:50+01:00
Author: Gil Forcada (gforcada) <gforcada@gnome.org>
Commit: plone/Products.ATContentTypes@0fc25a0

Adapt tests

With PLIP 1343 being merged some tests need to be adapted.

Now the catalog no longer processes indexing operations right away.

Some tests rely on that, so a minimal adaptation was needed.

reindex_sanity.txt is removed because with collective.indexing the test
becomes unreadable or never passes.

References:
plone/Products.CMFPlone#1343

Files changed:
M CHANGES.rst
M Products/ATContentTypes/tests/test_criteria.py
M Products/ATContentTypes/tests/test_doctests.py
D Products/ATContentTypes/tests/reindex_sanity.txt
Repository: Products.ATContentTypes
Branch: refs/heads/master
Date: 2017-02-21T22:16:29+01:00
Author: Gil Forcada Codinachs (gforcada) <gil.gnome@gmail.com>
Commit: plone/Products.ATContentTypes@e39512c

Merge pull request #34 from plone/merge-collective-indexing

Merge collective indexing

Files changed:
M CHANGES.rst
M Products/ATContentTypes/tests/test_criteria.py
M Products/ATContentTypes/tests/test_doctests.py
D Products/ATContentTypes/tests/reindex_sanity.txt
gforcada added a commit to plone/plone.app.upgrade that referenced this issue Feb 21, 2017
With PLIP 1343 being merged some tests need to be adapted.

Now the catalog no longer processes indexing operations right away.

Some tests rely on that, so a minimal adaptation was needed.

References:
plone/Products.CMFPlone#1343
mister-roboto pushed a commit to plone/buildout.coredev that referenced this issue Feb 21, 2017
Branch: refs/heads/master
Date: 2017-02-21T22:19:54+01:00
Author: Gil Forcada (gforcada) <gforcada@gnome.org>
Commit: plone/plone.app.upgrade@5afb7c9

Adapt tests

With PLIP 1343 being merged some tests need to be adapted.

Now the catalog no longer processes indexing operations right away.

Some tests rely on that, so a minimal adaptation was needed.

References:
plone/Products.CMFPlone#1343

Files changed:
M CHANGES.rst
M plone/app/upgrade/v43/tests.py
Repository: plone.app.upgrade
Branch: refs/heads/master
Date: 2017-02-21T22:20:44+01:00
Author: Gil Forcada Codinachs (gforcada) <gil.gnome@gmail.com>
Commit: plone/plone.app.upgrade@eccbbc2

Merge pull request #75 from plone/merge-collective-indexing

Merge collective indexing

Files changed:
M CHANGES.rst
M plone/app/upgrade/v43/tests.py
mister-roboto pushed a commit to plone/buildout.coredev that referenced this issue Feb 21, 2017
Branch: refs/heads/master
Date: 2017-02-21T22:19:54+01:00
Author: Gil Forcada (gforcada) <gforcada@gnome.org>
Commit: plone/plone.app.upgrade@5afb7c9

Adapt tests

With PLIP 1343 being merged some tests need to be adapted.

Now the catalog no longer processes indexing operations right away.

Some tests rely on that, so a minimal adaptation was needed.

References:
plone/Products.CMFPlone#1343

Files changed:
M CHANGES.rst
M plone/app/upgrade/v43/tests.py
Repository: plone.app.upgrade
Branch: refs/heads/master
Date: 2017-02-21T22:20:44+01:00
Author: Gil Forcada Codinachs (gforcada) <gil.gnome@gmail.com>
Commit: plone/plone.app.upgrade@eccbbc2

Merge pull request #75 from plone/merge-collective-indexing

Merge collective indexing

Files changed:
M CHANGES.rst
M plone/app/upgrade/v43/tests.py
gforcada added a commit to plone/plone.app.content that referenced this issue Feb 21, 2017
With PLIP 1343 being merged some tests need to be adapted.

Now the catalog no longer processes indexing operations right away.

Some tests rely on that, so a minimal adaptation was needed.

References:
plone/Products.CMFPlone#1343
mister-roboto pushed a commit to plone/buildout.coredev that referenced this issue Feb 21, 2017
Branch: refs/heads/master
Date: 2017-02-21T22:25:25+01:00
Author: Gil Forcada (gforcada) <gforcada@gnome.org>
Commit: plone/plone.app.content@29f2396

Adapt tests

With PLIP 1343 being merged some tests need to be adapted.

Now the catalog no longer processes indexing operations right away.

Some tests rely on that, so a minimal adaptation was needed.

References:
plone/Products.CMFPlone#1343

Files changed:
M CHANGES.rst
M plone/app/content/basecontent.rst
M plone/app/content/tests/test_widgets.py
Repository: plone.app.content
Branch: refs/heads/master
Date: 2017-02-21T22:26:42+01:00
Author: Gil Forcada Codinachs (gforcada) <gil.gnome@gmail.com>
Commit: plone/plone.app.content@6565502

Merge pull request #91 from plone/merge-collective-indexing

Merge collective indexing

Files changed:
M CHANGES.rst
M plone/app/content/basecontent.rst
M plone/app/content/tests/test_widgets.py
mister-roboto pushed a commit to plone/buildout.coredev that referenced this issue Feb 21, 2017
Branch: refs/heads/master
Date: 2017-02-21T22:25:25+01:00
Author: Gil Forcada (gforcada) <gforcada@gnome.org>
Commit: plone/plone.app.content@29f2396

Adapt tests

With PLIP 1343 being merged some tests need to be adapted.

Now the catalog no longer processes indexing operations right away.

Some tests rely on that, so a minimal adaptation was needed.

References:
plone/Products.CMFPlone#1343

Files changed:
M CHANGES.rst
M plone/app/content/basecontent.rst
M plone/app/content/tests/test_widgets.py
Repository: plone.app.content
Branch: refs/heads/master
Date: 2017-02-21T22:26:42+01:00
Author: Gil Forcada Codinachs (gforcada) <gil.gnome@gmail.com>
Commit: plone/plone.app.content@6565502

Merge pull request #91 from plone/merge-collective-indexing

Merge collective indexing

Files changed:
M CHANGES.rst
M plone/app/content/basecontent.rst
M plone/app/content/tests/test_widgets.py
gforcada added a commit that referenced this issue Feb 21, 2017
That's the minimal patching that collective.indexing was performing
into Products.CMFPlone.

Part of PLIP #1343.
gforcada added a commit that referenced this issue Feb 21, 2017
As indexing now is defered at the transaction boundaries instead of
being done right away, some tests need to be adapted to it.

Part of PLIP #1343.
@jensens jensens moved this from Under Review (implemented) to Ready in PLIPs Feb 22, 2017
@jensens jensens moved this from Ready to Merged in PLIPs Feb 23, 2017
@jensens
Copy link
Member

@jensens jensens commented Feb 23, 2017

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

@ebrehault
Copy link
Member

@ebrehault ebrehault commented Feb 23, 2017

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

@jensens
Copy link
Member

@jensens jensens commented Feb 23, 2017

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
Copy link
Contributor

@gforcada gforcada commented Feb 24, 2017

I will try to make the pull request

@gforcada
Copy link
Contributor

@gforcada gforcada commented Feb 24, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
PLIPs
Merged
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.