I'm running into the same situation I think as mentioned in #1137734-18: Better memory usage on indexing: I have a drush command that touches lots of entities, and after the command code finishes, the post request indexer tries to index them all and exhausts memory. I saw a solution mentioned:
In D8, you can simply call
\Drupal\search_api\IndexInterface::startBatchTracking()before doing lots of entity operations to avoid the indexing at the end of the page request. (Or temporarily disable theindex_directlyoption on the index – that’s possible in both D7 and D8.)
I don't think I can use startBatchTracking() because I don't deal with the config entity directly, the indexing is triggered via search_api_entity_update(). I can update index_directly and save it and then undo the changes later (my current solution) but it feels a little ugly to change the behavior of the site for everyone when I just want to prevent the single drush command from being impacted by post request indexing.
I just wanted to ask if I'm missing anything and put in a feature request if not. Thanks!
(And forgive me if this is really part of #1137734: Better memory usage on indexing I wasn't sure whether to start a new issue or ask there...)
Thanks!
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | 3229495-4--improve_batch_tracking--FAIL.patch | 4.95 KB | andyf |
| #4 | 3229495-4--improve_batch_tracking.patch | 7.24 KB | andyf |
| #2 | 3229495-2--improve_batch_tracking.patch | 2.27 KB | drunken monkey |
Comments
Comment #2
drunken monkeyNo, you’re not missing anything, this is really a bit tricky to do at the moment. I think the problem was caused by me not realizing right away that config entities aren’t statically cached, so loading the same search index multiple times in a page request will get you different objects. Otherwise, of course, just loading the index and calling
startBatchTracking()would do the trick.I do think it should work that way, though. The attached patch should implement this, in a pretty simple way.
(Not even sure, come to think of it, that we need a static array, instead of a static integer for all indexes. Sure, it’s technically documented that way for now (that batch mode is per-index), but I think the use case of batch-tracking for one index but indexing items immediately for the other is marginal, at best.)
Anyways, please test/review!
Also, we’ll need test coverage for this new functionality. Would you be willing/able to write something? It would be pretty simple, after a bit of setup (not actually sure, though, whether we have a fitting test class already where we’d just add a new test method): load the index, set batch tracking, save a node and assert that it’s not indexed immediately. (Would probably have to take care to also stop batch tracking after the test, even for a fail, to ensure that this doesn’t “bleed” into other tests.) Should probably be a Kernel test.
Comment #3
drunken monkeyNW for the tests.
Comment #4
andyf commentedOoh, lovely, thanks! That seems to do the job.
Yeah I can't think of any usecase, dunno if it's a lack of imagination (:
If you were thinking about making changes, this is a bit bikesheddy/subjective, but FWIW a little feedback (and maybe it's because I'm thinking about the implementation too much) but I found myself misunderstanding the meaning of
startBatchTracking()etc. (The function documentation imho is excellent btw, so this is really just about the method naming - I said it was bikesheddy!).stopBatchTracking()first.isBatchTracking()before making a change so I could revert it (ie I thought it'd be the caller's responsibility to clean up after themselves).Don't have any concrete suggestions or anything, just thought I'd pass the feedback on (:
Comment #6
andyf commentedComment #8
drunken monkeyGreat job, thanks!
Just polished a few details and then committed. Thanks again!
Regarding your suggestion: As this is part of the API, we cannot easily change it. However, it is possible. If you feel it would be helpful (and, ideally, have better suggestions), please feel free to create a new issue for this. We’d need a few votes in there, though, to make sure changing this really makes things clearer.
Comment #9
andyf commentedAwesome, thanks!
Totally, I was really mentioning it to go along with your comment
which made me think you had in mind maybe to change it at some point, and I just wanted to give my 2 cents for when/if you do. I'm certainly not pushing for a change, it's well documented, and I suspect it's sufficiently edge-case that the churn might not be worth it.
Thanks again!