-
Notifications
You must be signed in to change notification settings - Fork 330
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use queued job for "unsearching" when Scout queue is enabled #471
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lgtm overall. I do wonder about concurrency issues when both indexing and removing from index happens at the same time. But I guess this has been extensively tested in some production apps with the Scout Elasticsearch driver?
Co-authored-by: Dries Vints <dries@vints.io>
Co-authored-by: Dries Vints <dries@vints.io>
@chamby @matt-allan would be cool if you could give this a review if you can spare the time :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me! Thanks for putting this together!
Thanks @stevebauman! |
Happy to help! Thanks guys! |
Best day ever! Thanks @stevebauman, you're a hero to many. |
Btw @stevebauman did you test this with both Algolia & Meilisearch? |
@driesvints I didn't 😞 , do you have any links you may have handy so I could easily whip up a test environment for these providers? If not, I can dive in head first and give it a go -- let me know! |
Unfortunately not I'm afraid. You'll need to set up a test account with Algolia and follow the install instructions for Meilisearch. I'll hold off with the release until you've tested them out. No rush. |
Ok no worries! Challenge accepted 🙌 . I'll see if I can get this done tonight. |
tested with meilisearch on a small set of data (1100 items) |
Gonna skip this for this week's release then. Let me know if Algolia checks out alright as well. |
We're good to go! Queued.Unsearch.mp4 |
I've just tagged v9.1.0 with this. Thanks all! |
@shokme Thanks a ton for testing this with Meilisearch -- much appreciated 🙌 |
@stevebauman @driesvints After a few hours of investigation, we finally figured out that this PR broke our production setup when trying to remove Models from Algolia using an Aggregator. Not sure if the responsibility lies with Scout or Scout Extended? The following exception is thrown...
Here's the stack trace...
If you need any more information to figure this out, let me know. Thanks |
@GC-Mark since that's an Algolia Extended concept it's best to report that to their issue tracker. |
Added the issue over at https://github.com/algolia/scout-extended incase anyone else stumbles across this - algolia/scout-extended#282 |
Description:
Closes #331
This PR adds a
RemoveFromSearch
queuable job which will be dispatched whenscout.queue
is enabled.This specific queuable job has an overwritten
restoreCollection()
method that instantiates an Eloquent collection of the deleted models, by force-filling the model primary keys supplied by theModelIdentifier
object. This allows extended Scout engines to retrieve the ID used for indexing, so they can purge records properly.All credit goes to @matt-allan, as this is a slightly modified copy of his work on his forked repo 🙌
References:
Here are references to the most popular Scout Elasticsearch packages (they only use
getScoutKey()
for retrieving a unique identifier for the model):https://github.com/babenkoivan/elastic-scout-driver/blob/2e45a933dcc8b051b25cbf8010623e51ac457a35/src/Factories/DocumentFactory.php#L24
https://github.com/babenkoivan/scout-elasticsearch-driver/blob/15d349a0401e2bab6f774255dc3668fd28b6d4a0/src/Indexers/BulkIndexer.php#L67
https://github.com/babenkoivan/scout-elasticsearch-driver/blob/15d349a0401e2bab6f774255dc3668fd28b6d4a0/src/Payloads/DocumentPayload.php#L19-L29