Problem/Motivation

There is already a --limit option on the search-api:index drush command, but that does not take time into account.

In some scenarios, e.g. a worker running the drush command, having a clean time out would be nice to have, since the limit passed may not always index at the same pace.

Steps to reproduce

N.A.

Proposed resolution

Introduce a new --time-limit option to the command.

Remaining tasks

- Open the MR.
- Code review.

Issue fork search_api-3530660

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

marvil07 created an issue. See original summary.

marvil07 opened merge request !244

marvil07’s picture

marvil07 commented
Assigned: marvil07 » Unassigned
Issue summary: View changes
Status: Active » Needs review

Over MR 244 the new option is introduced.

From the commit message:

This time limit is a best effort, not a exact time out.
Given the underlying code uses Batch API to run the indexing, a more exact bail out is not truly possible.
Instead, do check after each batch is processed, and act accordingly.

Sample use:

drush search-api:index --time-limit=30 node_index

drunken monkey made their first commit to this issue’s fork.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented

Looks great, thanks a lot! And sorry for taking so long to reply.
I just had some minor nitpicks and made some code style changes, otherwise this looks good to go.
Feel free to review my changes and tell me what you think, otherwise I’ll just merge this in a week or so.

marvil07’s picture

marvil07 commented

Looks great, thanks a lot! And sorry for taking so long to reply.

🎉 Thanks for maintaining this project!

I just had some minor nitpicks and made some code style changes, otherwise this looks good to go.

Those changes look quite good, thanks!

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Needs review » Needs work

Found a minor issue when passing a time limit of 0. The code seems to specifically allow for this, but then any time limit of 0 is converted into -1.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs work » Needs review

@kristiaanvandeneynde: Thanks for catching that, you’re right.
Should be fixed now.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented

Does anyone else want to test/review, is there anything still missing, or is this RTBC?

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

Only nitpick I have is that the time limit is allotted for every index if you specify no index. You might want to document that as such. If you have 6 indexes and specify a time limit of 10 seconds, it will end up running for 60 seconds unless you manually specify a single index.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Reviewed & tested by the community » Needs review

You’re right, that’s ambiguously phrased. I changed it to this, which should also make it clearer that time spent will usually be a bit more than specified, not exactly (e.g.) 10.0 seconds:

The maximum number of seconds allowed to run indexing per index. Started batches will be completed before stopping. Defaults to -1 (no limit).

Do you think this is better, or do you have another suggestion?

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

The maximum number of seconds allowed to run indexing per index. Batches that were started before the limit was reached will run to completion and may therefore slightly exceed the limit. Defaults to -1 (no limit).

Is this better? It seems more clear about the potential impact.

Either way, feel free to commit your or my version without further feedback. I consider this issue RTBC. I'm not a fan of delaying things over nitpicking how we should phrase a comment :)

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs review » Fixed

Ah, yes, I like that even better. Changed the description and then merged.
Thanks again, everyone!

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.