EZP-30296: Large API subtree delete leads to serious Solr index inconsistencies #150

kmadejski · 2019-09-24T11:08:15Z

JIRA ticket: https://jira.ez.no/browse/EZP-30296

Bulk deleting using deleteByQuery has been introduced. I couldn't find particular information about Solr's bulk operation size limit, at least not in their documentation, but I think it's good to have some limit to prevent exceeding GET length limit for instance.

After a talk with @andrerom and @alongosz I decided to move further work on slots to a separated PR. Therefore, this one covers only bulk deleting.

andrerom

Change to deleteAllItemsWithoutAdditionalLocation() seems good. The one one re indexing subtree seems like it needs a bit further work, maybe split it into separate PR/Issue depending on approach you choose to be able to move fast on delete issue.

Side: This should ideally be applied to earlier branches as well, even 1.0 if it applies (in case of delete change at least), but at least 1.5.

andrerom · 2019-09-24T11:23:00Z

lib/Resources/config/container/solr/slots.yml

@@ -15,7 +15,7 @@ parameters:
    ezpublish.search.solr.slot.move_user_group.class: eZ\Publish\Core\Search\Common\Slot\MoveUserGroup
    ezpublish.search.solr.slot.delete_user_group.class: eZ\Publish\Core\Search\Common\Slot\DeleteUserGroup
    ezpublish.search.solr.slot.copy_subtree.class: eZ\Publish\Core\Search\Common\Slot\CopySubtree
-    ezpublish.search.solr.slot.move_subtree.class: eZ\Publish\Core\Search\Common\Slot\MoveSubtree
+    ezpublish.search.solr.slot.move_subtree.class: EzSystems\EzPlatformSolrSearchEngine\Slot\MoveSubtree


So I'm a bit torn here

bulkIndexContent should be added to a SPI interface and handler here and others that can should implement it. then we can update kernel's AbstractSubtree::indexSubtree() to take advantage.

If we don't do that, we'll need to overload all subtree slots here (missing is hide and unhide at least from what I could see now in 6.13 branch).

As discussed on Slack, I'll do it in a separated PR(s).

kmadejski · 2019-09-24T14:07:34Z

It seems that my PR's tests are failing in the same places as the one done by @mateuszbieniek (#149). I'll try to fix these tests, as I would love to have Travis green before merge.

mateuszbieniek · 2019-09-24T14:08:21Z

yes please 😁

andrerom · 2019-09-24T15:08:29Z

It seems that my PR's tests are failing in the same places as the one done by @mateuszbieniek (#149). I'll try to fix these tests, as I would love to have Travis green before merge.

+1 (missing backports from 1.6/1.7 maybe, which fails with test on kernel 7.5? if so maybe solution is to stop supporting 7.5.x on 1.5 branch 🤷‍♂ and make it 1.x only)

Regarding the PHP 5.6 failure on composer running out of memory, I think QA might have worked quite a bit on solving this other places cc @mnocon

mnocon · 2019-09-25T10:34:09Z

I can't help you with the failiing tests, but I have a solution for memory issues 😉

See #151

kmadejski · 2019-09-26T14:39:53Z

I'm almost a 100% sure that there failing tests are related to a missing commit, the one from this PR: #133 to be more precise. I've temporarily pushed it here to check whether it indeed solves the problem.

Unfortunately, I can't be like this, since 1.5 can be installed also against eZ Platform v1.7/v1.13/v2.3 and v2.4 which are missing content hide/reveal feature. The only option to make tests work on this branch is to change composer.json and allow installing at most the latest v7.4.x (or even v6.x) kernel release.

kmadejski · 2019-09-27T09:26:38Z

@andrerom last build (https://travis-ci.org/ezsystems/ezplatform-solr-search-engine/jobs/589992661) confirms my suspicion. How would you like to proceed? Personally, I would drop the possibility to install 1.5 for v2.x.

Edit: Ok, it's green now!

@andrerom / @alongosz / @mnocon / @adamwojs / @lserwatka are you ok with the change I did in: 18c9c5e? It will be applied to 1.5 only.

kmadejski · 2019-10-02T15:12:56Z

Fixed issue found by @micszo in 4a99ddf.

andrerom · 2019-10-02T19:01:08Z

composer.json

@@ -12,7 +12,7 @@
    ],
    "require": {
        "php": "~5.6|~7.0",
-        "ezsystems/ezpublish-kernel": "~6.7.10@dev|^6.13.6@dev|~7.3.5@dev|^7.4.3@dev",
+        "ezsystems/ezpublish-kernel": "~6.7.10@dev|^6.13.6@dev|~7.3.5@dev|~7.4.3@dev",


I'm ok with this, so +1 once PR is green ;)

TODO: Drop this line on merge up.

andrerom · 2019-10-03T09:18:57Z

lib/Handler.php

@@ -328,7 +334,7 @@ public function commit($flush = false)
     */
    protected function deleteAllItemsWithoutAdditionalLocation($locationId)
    {
-        $query = $this->prepareQuery();
+        $query = $this->prepareQuery(self::SOLR_MAX_QUERY_LIMIT);


Ok I get why you wanted really high limit now..

I think unfortunately you'll have to change logic to iterate on this if you get back this amount of hits, or you might get total count which will tell you (roguthly*) how many iterations you need to make right?

* In the freak case of parallel async processes for some reason adding locations to this tree at the same time.

I think unfortunately you'll have to change logic to iterate on this if you get back this amount of hits, or you might get total count which will tell you how many iterations you need to make right?

It was my first idea, but @adamwojs was really against this approach since it might be expensive, performance wise, to perform two same queries, first to count and second to load locations. WDYT @andrerom?

But that said, that can be follow also. You definitely improve the logic here as previous limit was 1000...

So ignore this for now unless you feel up for it in this PR

AS said above, this is already way better then what is there today. So I'm fine with this as is.

@andrerom cool, let's stay with what we have now. I will talk to @adamwojs once we both have time and try to figure out some better way to improve it perhaps.

kmadejski · 2019-10-03T09:26:52Z

@micszo since it's green now, could you please test it again?

micszo

QA Approved on eZ Platform EE v2.5.5 with diff (w/o 18c9c5e).

lserwatka · 2019-10-03T10:04:44Z

@kmadejski you can merge it up but please remember about dropping line from composer.json

lserwatka · 2019-10-03T10:07:51Z

@kmadejski make sure you merge to 1.6, 1.7, 2.0 and master (3.0)

kmadejski · 2019-10-03T11:32:10Z

@lserwatka done in e66f9e8 / 205a256 / f95f0f3 / 2533029

kmadejski requested review from adamwojs, andrerom, alongosz, konradoboza and mateuszbieniek September 24, 2019 11:08

kmadejski self-assigned this Sep 24, 2019

kmadejski added the Work in progress label Sep 24, 2019

andrerom suggested changes Sep 24, 2019

View reviewed changes

kmadejski force-pushed the ezp_30296 branch 2 times, most recently from caee711 to bafb81b Compare September 24, 2019 11:52

kmadejski changed the base branch from 1.7 to 1.0 September 24, 2019 11:52

kmadejski changed the title ~~[WIP] EZP-30296: Large API subtree delete leads to serious Solr index inconsistencies~~ EZP-30296: Large API subtree delete leads to serious Solr index inconsistencies Sep 24, 2019

kmadejski added Ready for review and removed Work in progress labels Sep 24, 2019

andrerom approved these changes Sep 24, 2019

View reviewed changes

konradoboza approved these changes Sep 24, 2019

View reviewed changes

mateuszbieniek approved these changes Sep 24, 2019

View reviewed changes

kmadejski changed the base branch from 1.0 to 1.5 September 24, 2019 13:52

kmadejski force-pushed the ezp_30296 branch from 62c21ae to 04e6bd2 Compare September 24, 2019 13:52

alongosz approved these changes Sep 24, 2019

View reviewed changes

kmadejski added 2 commits September 27, 2019 15:12

EZP-30296: Introduced bulk document deleting using deleteByQuery

abf63dc

Updated composer.json to prevent installing ezpublish-kernel v7.5.x

18c9c5e

kmadejski force-pushed the ezp_30296 branch from 6ec6021 to 18c9c5e Compare September 27, 2019 13:14

kmadejski requested a review from lserwatka September 27, 2019 14:10

andrerom added Ready for QA and removed Ready for review labels Sep 29, 2019

micszo self-assigned this Sep 30, 2019

Updated query limits

4a99ddf

andrerom reviewed Oct 2, 2019

View reviewed changes

Updated SOLR_MAX_QUERY_LIMIT value due to JVM limitations

5c1f9de

andrerom suggested changes Oct 3, 2019

View reviewed changes

andrerom approved these changes Oct 3, 2019

View reviewed changes

adamwojs approved these changes Oct 3, 2019

View reviewed changes

kmadejski removed their assignment Oct 3, 2019

micszo approved these changes Oct 3, 2019

View reviewed changes

micszo added QA Approved and removed Ready for QA labels Oct 3, 2019

micszo removed their assignment Oct 3, 2019

lserwatka merged commit d1f4042 into ezsystems:1.5 Oct 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-30296: Large API subtree delete leads to serious Solr index inconsistencies #150

EZP-30296: Large API subtree delete leads to serious Solr index inconsistencies #150

kmadejski commented Sep 24, 2019 •

edited

Loading

andrerom left a comment •

edited

Loading

andrerom Sep 24, 2019

kmadejski Sep 24, 2019

kmadejski commented Sep 24, 2019 •

edited

Loading

mateuszbieniek commented Sep 24, 2019

andrerom commented Sep 24, 2019 •

edited

Loading

mnocon commented Sep 25, 2019

kmadejski commented Sep 26, 2019 •

edited

Loading

kmadejski commented Sep 27, 2019 •

edited

Loading

kmadejski commented Oct 2, 2019

andrerom Oct 2, 2019 •

edited

Loading

andrerom Oct 3, 2019 •

edited

Loading

kmadejski Oct 3, 2019

andrerom Oct 3, 2019

andrerom Oct 3, 2019

kmadejski Oct 3, 2019

kmadejski commented Oct 3, 2019

micszo left a comment

lserwatka commented Oct 3, 2019

lserwatka commented Oct 3, 2019 •

edited

Loading

kmadejski commented Oct 3, 2019

EZP-30296: Large API subtree delete leads to serious Solr index inconsistencies #150

EZP-30296: Large API subtree delete leads to serious Solr index inconsistencies #150

Conversation

kmadejski commented Sep 24, 2019 • edited Loading

andrerom left a comment • edited Loading

Choose a reason for hiding this comment

andrerom Sep 24, 2019

Choose a reason for hiding this comment

kmadejski Sep 24, 2019

Choose a reason for hiding this comment

kmadejski commented Sep 24, 2019 • edited Loading

mateuszbieniek commented Sep 24, 2019

andrerom commented Sep 24, 2019 • edited Loading

mnocon commented Sep 25, 2019

kmadejski commented Sep 26, 2019 • edited Loading

kmadejski commented Sep 27, 2019 • edited Loading

kmadejski commented Oct 2, 2019

andrerom Oct 2, 2019 • edited Loading

Choose a reason for hiding this comment

andrerom Oct 3, 2019 • edited Loading

Choose a reason for hiding this comment

kmadejski Oct 3, 2019

Choose a reason for hiding this comment

andrerom Oct 3, 2019

Choose a reason for hiding this comment

andrerom Oct 3, 2019

Choose a reason for hiding this comment

kmadejski Oct 3, 2019

Choose a reason for hiding this comment

kmadejski commented Oct 3, 2019

micszo left a comment

Choose a reason for hiding this comment

lserwatka commented Oct 3, 2019

lserwatka commented Oct 3, 2019 • edited Loading

kmadejski commented Oct 3, 2019

kmadejski commented Sep 24, 2019 •

edited

Loading

andrerom left a comment •

edited

Loading

kmadejski commented Sep 24, 2019 •

edited

Loading

andrerom commented Sep 24, 2019 •

edited

Loading

kmadejski commented Sep 26, 2019 •

edited

Loading

kmadejski commented Sep 27, 2019 •

edited

Loading

andrerom Oct 2, 2019 •

edited

Loading

andrerom Oct 3, 2019 •

edited

Loading

lserwatka commented Oct 3, 2019 •

edited

Loading