I had this idea (during indexation of a large(ish) site with quite a bit of nodes) that the order of the nodes being indexed seems wrong: it indexes the oldest nodes first instead of the newest first.

I'm not sure where this can be changed, so I'm just typing a little feature-request.

Perhaps it could be some kind of option to reverse the order of items during indexation.
I assume the newest nodes offer the most relevant information and thus should be indexed first.
It is only a temporary-'problem', because once everything is indexed, all data should be available. It's only during the indexation (after a clear or re-queue) that the newest nodes cannot be found.

Comments

SpadXIII created an issue. See original summary.

drunken monkey’s picture

Priority: Minor » Normal

I don't think I've ever heard anyone else with the opinion that newer items should be indexed first. All modules I know work with the reverse principle, and so the Search API does the same. Thinking about it, your position does have a certain logic to it. I guess it all depends on the kind of site you have – if old content becomes almost irrelevant at some point, I guess starting indexing with those doesn't make much sense.
If enough people think this would be useful, I guess making this an option wouldn't be too hard – especially in Drupal 8, where the tracking functionality has its own plugin. But I'll wait for a few more to speak up before I'd consider that. As said, never heard this before.
Also, as you say, this problem usually only persists for a short time until everything has been indexed (again).

Otherwise, though, you can always override the behavior with a bit of coding: in Drupal 7 by overriding the datasource class with a custom subclass that implements this change, and in Drupal 8 probably just by creating a new tracker plugin with the changed behavior.

nwom’s picture

+1. I think this would be a great feature to have as well, especially when working with a site that consists of nothing but Search API views.

The idea is that you can re-index without worrying whether the newest content is still visible. Indexing now is a risk since the content that is currently being used, is the last to index. I hope this makes sense.

kristofferwiklund’s picture

Status: Active » Needs review
StatusFileSize
new536 bytes

I also had that problem & idea.

I was poking around in the code and found this:

When indexing items, it is sorted by the changed time. (When the item was made "dirty"). This is what you want if you have lots of continuous changes of nodes. So when cron start indexing "dirty" nodes, the items that was changed first will be index (FIFO queue). So when editors complains that things are not updated its just a matter of time. (Like cache invalidation.)

But there is a special case. When you clear the index or change the index. In this case all items gets the same changed time. And its is up to the db to sort them. And most of DB will just short them with the oldest item first.

So I came up with a solution. If the changed time is the same sort them by item_id in descending order.

I have attached a patch.

The drawback is the minimal performance penalty. This is my finding with mysql queries.
DB #1
5589 items in index ( 12706 in total)
Now: 0.0013 s
Patched: 0.0034 s

DB #2
25174 items in index ( 47259 in total)
Now: 0.0013 s
Patched: 0.0078 s

This query is run for every batch of items.

But in relation to other times. When indexing DB #1 with a Solr database with a batch size of 50 times it takes us around 5 minutes. Theoretical the patch would add up to a second to the total time. But the benefits is that the site is kind of working straight away. Some old data might not be there but for normal user the sites looks like it working as it should.

Status: Needs review » Needs work

The last submitted patch, 4: order_of_items_during-2669962-4.patch, failed testing. View results

drunken monkey’s picture

OK, thanks for your input!
The actual change is of course trivial, and you're right that the performance impact can also be neglected. The question is just whether this would be useful enough to add an option for it in the UI. Three people is already something, but I'd still like two or three more up-votes. Feature creep, and such. In the meantime, feel free to just run your patch on your site. Looks good. (The tests only fail because they expect the current order of indexing. When changing this to an option (and defaulting to the current handling) the fails should go away on their own.

AlexKirienko’s picture

I have the same issue. Changing the order in UI will be much helpful.
But patch 4 change only order in getChangedItems(). With such changes, new content will be indexed last, cos it has changed = 1 by default.
Check trackItemInsert(). In that case, 1 have to be replaced by REQUEST_TIME.

drunken monkey’s picture

But patch 4 change only order in getChangedItems(). With such changes, new content will be indexed last, cos it has changed = 1 by default.

No, since they also changed the field being sorted on to item_id. (This might actually not work well, though, in case of string IDs. But that's not such a problem in D7.) Still, having changed = 1 with this new implementation wouldn't really make sense, either, so might as well change that, too, sure.

kevin.dutra’s picture

Version: 7.x-1.x-dev » 8.x-1.x-dev
Assigned: Unassigned » kevin.dutra

+1 for me too.

I can completely understand the initial decision to order from oldest change to newest -- if I update my piece of content before some other person updates theirs, I'd expect my change to make it into the index first. During normal operation, items are likely to be going in quickly enough that it would be difficult to tell the ordering.

But as @NWOM pointed out, the real problem with that ordering becomes apparent when all the items are flagged to be indexed. As soon as that happens, any new piece of content added gets stuck behind all that other content waiting to go in. If you have a sufficiently large amount of content (and/or sufficient complexity in the processing you're doing at index time), you could be waiting a significant amount of time before everything catches up and that new piece of content finally gets indexed.

I'll work on an 8.x version of the change.

kevin.dutra’s picture

Assigned: kevin.dutra » Unassigned
Status: Needs work » Needs review
StatusFileSize
new3.27 KB

Here's a first stab at it.

Status: Needs review » Needs work

The last submitted patch, 10: indexing-order-d8-2669962-10.patch, failed testing. View results

kevin.dutra’s picture

Status: Needs work » Needs review
StatusFileSize
new4.11 KB
new724 bytes

Fix for failing test.

kevin.dutra’s picture

StatusFileSize
new4.58 KB
new658 bytes

What a dummy....I forgot the key piece! Third time's a charm.

kristofferwiklund’s picture

I notice it also. But you said "first stab".. :)

But looking at the Drupal 8 solution you have change sorting order (with a settings).

If you look at the Drupal 7 solution I wrote, I just added a secondary sort criteria. So that we solves the two requirements (re-indexing and continuous indexing)
That only has a minimal drawback of performance penalty of <1%, but rest works as you would like.

kevin.dutra’s picture

Thanks for the feedback @kristofferwiklund, but unless I'm missing something, just having a secondary sort does not help the re-indexing case. (At least for D8. I'm not familiar with prior versions of the module, so it might work quite differently.)

As an example, let's say I have 500k nodes indexed currently. I go mark all of content for re-indexing. Not only does this mark them as needing to be reindexed, it updates the changed time, so all 500k are now at time N. Shortly after, a user adds a new node, which is tracked at time N+5. If we leave the primary sort at "changed ASC", then the indexing process won't see the new node until after all 500k nodes at time N are finished, since N < N +5.

I'm less concerned with those 500k existing items that have been flagged for reindex -- they're already in the index, so they're at least still available for searching on. I'm more concerned with the things that have changed after things were marked for reindex. Does that make sense? I think the issue you're touching on is a related (but different) issue.

drunken monkey’s picture

Thanks a lot, great work!
I had some minor nitpicks, fixed in the attached patch, but otherwise this looks great. Having a test for this would of course be even better, but apparently there's no (explicit) test of the tracker plugin there at all at the moment, so this can be a separate issue: #2928279: Add a test for the default tracker plugin.
So, if this revised patch still works for you, we can commit it. Please test/review!

@ #14/#15: This indeed worked differently in D7, so the comment makes sense there, but it won't the same in D8.

drunken monkey’s picture

Component: Miscellaneous » Plugins
kevin.dutra’s picture

Thanks @drunken monkey. I'm happy to review the revised patch, but only if you attach it. ;)

drunken monkey’s picture

StatusFileSize
new3.01 KB
new4.57 KB

An altogether not unreasonable request, thanks …

kevin.dutra’s picture

Status: Needs review » Reviewed & tested by the community

The changes look good to me, and I didn't find any issues when testing, so I think this is RTBC.

borisson_’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

I'm here nits! Sorry.

  1. +++ b/src/Plugin/search_api/tracker/Basic.php
    @@ -105,6 +109,31 @@ public function setTimeService(TimeInterface $time_service) {
    +        'fifo' => $this->t('Index items in the same order in which they were saved'),
    +        'lifo' => $this->t('Index the most recent items first'),
    

    So many flashbacks to accountancy classes.

  2. +++ b/src/Plugin/search_api/tracker/Basic.php
    @@ -167,7 +196,8 @@ protected function createRemainingItemsStatement($datasource_id = NULL) {
    +    $select->orderBy('sai.changed', $indexing_order == 'lifo' ? 'DESC' : 'ASC');
    

    Can we use a strict equals here?

I also didn't seem to see a test for the last-in first-out type of indexing, we should add those.

drunken monkey’s picture

Can we use a strict equals here?

Right, good catch!

I also didn't seem to see a test for the last-in first-out type of indexing, we should add those.

That's why I created #2928279: Add a test for the default tracker plugin. It should be easy there to just have a date provider for testing both FIFO and LIFO.
Getting test coverage right away would of course be ideal, but I think it's out of scope for this issue (unless someone feels motivated).

Would you otherwise say this is RTBC?

borisson_’s picture

Otherwise this is RTBC for me, yeah!

drunken monkey’s picture

Status: Needs work » Fixed

OK, then changed the comparison to strict equals (I might have used == to stay within 80 characters, which is a really stupid reason, but also means I refactored a bit more for this) and committed.
Thanks again, everyone!

I'll make sure to do the tracker test today (or very soon) so the missing test isn't too much of a problem. (I do realize how much it helps to be strict regarding tests, and I'm glad you always call me out on it, but writing them is just so boring …)

nwom’s picture

Version: 8.x-1.x-dev » 7.x-1.x-dev
Status: Fixed » Patch (to be ported)

Thank you very much everyone for this. Is it possible we can get a ported version of 8.x's patch for 7.x as well? It would be hugely appreciated.