Fix regressions introduced with cron queue indexing

A secondary issue with this new approach is how emptying out a index works..

if you have a index that 10% indexed, it will have cron queue task items in the queue for the remaining 90% of the index. If you then ask to re-index or clear the index, the queued-status of all items will be reset, but the cron task queue wont be reset!

So after next cron run, you will now still have the original task for the remaining 90% of the index AND it will add tasks for what it believes is missing from the queue, which is everything since the queued-status has been reset, so it adds task to index 100% of the index to the cron queue. Hence the cron task queue will now contain tasks to index 190% of your index..

So when clearing the index, if the current method is preserved, steps should be taken to clear the cron task queue of indexing tasks for the same index!

i know they are two different problems, but they are both regressions brought in by changing the way indexing-at-Cron is handled from the old "take $limit from each index" to "enqueue all of the index in $limit-big chunks"..

I haven't tested out your patch, but by looking at it, I'll say it looks sane. I'll test it out when I get back to work on Monday.

As it is now, the manually indexing options that exists ("add to index" on the status-page and drushes sapi-i) are broken. The status-page fails with a "check the log" message, but nothing ever shows up in the log, because no error actually happened, it just failed to find any items to index. Drush just says it completed the task successfully, but didn't actually do anything.

Because of this, I would say that this issue is actually pretty 1.0-blocking, because as it is now, the module contains functionality that doesn't work. Feel free to disagree with me, and I guess most people never actually use the means of manually indexing - but the interfaces for it exists, but doesn't work.

As a quick-fix, I've substituted the -1 limit with the $limit in hook_cron, so it basically changes into working the way it used to do. This allows us to still use the manually indexing, to quickly re-index after deploys. Since we use immediate-indexing, our index-queue is pretty much always 0, so we don't get any benifit from being able to do more than $limit items per cron-run.

-Morten

I know they could have / should have been two separate issues - I just thought of the second "part" of the issue afterwards, and thought I'd share it quick before I forgot it again :)

I guess it is a viable option to just change the "index manual" to "manual run queued tasks" - but IMHO, the text-field should be changed from being "number of entities to index", and into "number of tasks to run - one task = $limit entities" because that's really the only feasible solution.

Also, what would / should happen if you change the number of items to index at a time? Would it need to clear the queue and then re-populate it with tasks of the new $limit size? Does this config setting even matter anymore, now that it can process more than one task of $limit entities per cron-run? Why not just hard-wire it to 50 entities?

Okay, got around to testing the patch now. There is one small bug in it - but fixing that, and it works how you'd expect it to work.

The bug is that _search_api_indexing_queue_process never actually returns anything, despite the docblock saying so, and search_api_index_items relying on it. Adding a return $num; solves this, and then it works. (Otherwise it keeps trying to index, never reaching the limit)

One small objection: Make the default number of items to process be 1, not -1. -1 on large indexes are sure to give a max-execution-time error.

Another observation: I've started to run into a lot of memory-limit problems on Cron with this new indexing. Mostly on the index that has a bundle filter that filters out 95% of the nodes in existence.
My guess to the cause of this is that entity_load_multiple keeps a static cache of retrieved entities so same-request loads of the same entity can be taken from this cache. And since the bundle-filter filters most of all entities, there is a high loaded-to-indexed ratio. So in the 15 seconds the Cron worker has, it can easily load upwards of a few thousand entities because it only has to actually index very few of these. Because of all these loads, the static entity cache runs out of memory.
I don't really know what could solve this, except perhaps seeing if it's possible to reset this cache once in a while, so it doesn't grow to error-inducing size..

Getting this working properly has un-earthed two bugs in my search api config, so after I've solved those I'll test it some more..

Okay, having sorted my config out I can now definitely say that this is a really nice improvement over both the old system, and the new system before these changes!

In my opinion, two small issues remain:
1) Running out of memory - as outlined above it _can_ happen, but not sure of the best way to combat it. A side-effect of this is that the task that was being worked on while running out of memory is left as claimed, but neither released or deleted. After a while the claim will expire and you can index it, but until then you have a index you cant get to 100% indexed!

2) Running out of time with manual indexing (only via GUI, as CLI as no time-limit). The "easy" way to combat this, is to build in a time-checking just like Core's Cron has that ensures that it will only attempt to process items in the queue if there is time left. Either a hard-coded limit of 15 secs, or perhaps max_execution_time() - 5 secs could be used as a limit. Note that it would be important to use use the time-limit if the max execution time is limited, so CLI runs don't get cut off early.
Alternatively batch-processing could be utilized - but honestly, if you can just hit "index now" to have it index items until just before it runs out of time, and then return to the status page, I don't really see a reason to spend time getting batch operations to work.
If this is fixed, I see now reason not to have -1 as the default for the index-now input field.

If 1 + 2 is sorted, it would be easy-as to fully re-index large indexes! That would be amazing. Until now, we've been using a bash for-loop around a fairly small number of items to index via Drush.

Thanks for taking the time to look at this - I assume a lot of people would never run into issues like this, because they don't have upwards of 50.000 items in an index.

Morningtime: If you read the last part of my comment in #5, this is because all the batched ops are created in one go using the setting supplied - if you change the setting it doesn't purge the queue and re-populate it with jobs of the new batch-size. So only future jobs will use a change setting, no the jobs already created..

This could be alleviated by purging the queue on batch-size, like on re-indexes..

Thomas: I concur with the Batch API resolve of the timing issues..

I'll try to see if I can't whip up a not-too-ugly implementation of emptying out the static cache, because then you should be able to just ask Drush to index all in one big swoop - which would be amazing.. Though not before I get back into work on Monday, at which time I'll also test out the patch in #9.

I think it looks good, if it wasn't for the fact that calls to _search_api_empty_cron_queue usually runs out of execution time, because it cant run through the search_api_indexing_queue items within 30 seconds (I have 4 indexes of each 550*50 entities, 1 of 200*50 entities, and one of 1480*50 entities).

If I have all more than one index waiting in the queue, and I attempt run _search_api_empty_cron_queue on the index that isn't the first index in the queue, it will usually run out of execution time before it even starts to look at the items for the index in question, so it never gets to where it's actually purging, and hence the queue will never be purged of that indexes items..

I know that our use-case with that many indexes with that many entities are a bit of a special case, and I don't really know how you could improve the speed of index-purging.
My only idea would be to see if it's possible to track the item_id of the items in the queue, and then do a manual mapping to search-index, so it's not required to claim every item, unserialize the item and then inspect it just to see what index it belongs to.. Because then purging could be a (fairly simple) delete from queue where item_id in (select item_id from search_api_queue_item_mapping where index = 'xx'); delete from search_api_queue_item_mapping where index = 'xx';

I just thought of what would happen if you delete a index (or say, reverted a feature with an index in) - does it then purge the queue too?

I agree with you sentiments towards 1.0 - once this we think this issue is done, I'll create a new one with regards to the entity caching leading to memory errors.

So my verdict: For normal use, it should work pretty well right now - but for large indexes, it has some very annoying flaws.

-Morten

Ah, I didn't know that releaseItem behaved that way - that makes more sense, why it failed so consistently, then.

Latest patch works well - queue purges only takes a few seconds now.

--

I haven't investigated, but does a index-delete trigger the clean up of the tracked state of the entities in the index? If so, wouldn't a feature revert (which really is just a delete + create) basically set all of the entities as needing indexing? And the cron queue would still contain items to index whatever was left from before the revert - in which case you could get duplicate cron queue items because it would add the entire index to the queue..

Okay, just did a quick review of the patch, and have one comment.

It appears you've solved the problem with purging the queues by adding the call to _search_api_empty_cron_queue() in dequeueItems(), however you have multiple lines were you go

$index->dequeueItems();
_search_api_empty_cron_queue($index);

where the call to _search_api_empty_cron_queue() is now unnecessary, as it is contained in dequeueItems().

Okay, I've updated the patch to remove the two times where _search_api_empty_cron_queue() was called just after calling dequeueItems()

Haha, right - yeah, I just hacked the .patch file and removed the two lines. Should have known better than to expect that to work :)

I no longer have any concerns with this issues except for the cache/out-of-memory problem that I'll post in a new issue. So I support this patch as-is right now. It concur that it would be great if Morningtime could sign off on it too, yes..

Thanks for looking at this (now lenghty) issue, and I now really think this new way of doing it is better to the old :)

-Morten