Skip to content
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

[6.0] Use engine to flush records of model #310

Merged
merged 5 commits into from
Oct 8, 2018
Merged

[6.0] Use engine to flush records of model #310

merged 5 commits into from
Oct 8, 2018

Conversation

driesvints
Copy link
Member

@driesvints driesvints commented Oct 5, 2018

Instead of iterating over ids from the database we use the engine itself to flush the records of an index/model.

This solves the problem where models would get out of sync with the search records and old records weren't flushed. With this implementation all records are always removed.

If there are engines which don't provide this functionality they can still iterate over the model ids if they want to.

Fixes #101

@julienbourdeau would you mind also taking a look at this if this is correctly implemented? I didn't have the chance to test this with a live implementation of Algolia.

Instead of iterating over ids from the database we use the engine itself to flush the records of an index/model.

This solves the problem where models would get out of sync with the search records and old records weren't flushed. With this implementation all records are always removed.

If there are engines which don't provide this functionality they can still iterate over the model ids if they want to.

See #101
Copy link
Member

@nunomaduro nunomaduro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything seems good to me. I have submitted some feedback. 👍

What do you think @julienbourdeau?


$this->line('<comment>Flushed ['.$class.'] models up to ID:</comment> '.$key);
});

$model::removeAllFromSearch();

$events->forget(ModelsFlushed::class);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also don't need this line.

{
$index = $this->algolia->initIndex($model->searchableAs());

$index->clearIndex();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirm. This is good. 👍

CHANGELOG.md Outdated
@@ -0,0 +1,6 @@
# Release Notes

## [v6.0.0 (unreleased)](https://github.com/laravel/framework/compare/v5.0.3...v6.0.0)
Copy link
Member

@nunomaduro nunomaduro Oct 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be: ## [v6.0.0 (unreleased)](https://github.com/laravel/scout/compare/v5.0.3...v6.0.0)

@driesvints
Copy link
Member Author

@nunomaduro thanks Nuno!

@julienbourdeau
Copy link
Contributor

julienbourdeau commented Oct 5, 2018

All good 💯

Another great news with this implementation is that it will save a lot of indexing operations for the user. This command will only be counted (or charged) as 1 operation instead of as many as table rows.

@taylorotwell
Copy link
Member

So this removes the ModelsFlushed event?

@nunomaduro
Copy link
Member

@taylorotwell No. With @driesvints's proposal, the method $model::removeAllFromSearch() no longer calls the method$model::unsearchable(), so the event ModelsFlushed::class is not fired.

But, if the developer intencionally calls $modelsCollection::unsearchable(), the event ModelsFlushed::class is fired on every batch of 500 models. This way the developer have access to the current state of the batch operation: Models already flushed, last model flushed, etc.

@@ -34,16 +34,8 @@ public function handle(Dispatcher $events)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@driesvints This class no longer needs the line use Laravel\Scout\Events\ModelsFlushed;.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@driesvints
Copy link
Member Author

@taylorotwell @nunomaduro Taylor does bring up a good point that the ModelsFlushed event isn't fired anymore because it accepts an array of which models were flushed. With this new implementation we just assume all models were flushed. But some people may rely on the event to do stuff after that. So perhaps we'll need to introduce a new event or adjust the current event to work without having to pass a collection of models.

Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants