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

Add experimental support for Symfony Messenger #16

Merged
merged 16 commits into from
Mar 8, 2020

Conversation

damienalexandre
Copy link
Member

Symfony Messenger is really simple to use. In this pull request I just add the basic of handling indexation request via Messenger.

There is a simple DTO, the provided handler must be overwritten to implement the fetchModel method, then I call the proper methods on the Indexer.

I use a very close version of this in production already 😋

composer.json Outdated Show resolved Hide resolved
src/Messenger/IndexationRequest.php Outdated Show resolved Hide resolved
src/Messenger/IndexationRequest.php Outdated Show resolved Hide resolved
src/Messenger/IndexationRequestHandler.php Outdated Show resolved Hide resolved
src/Messenger/IndexationRequestHandler.php Show resolved Hide resolved
src/Messenger/IndexationRequestHandler.php Show resolved Hide resolved
src/Messenger/IndexationRequestHandler.php Outdated Show resolved Hide resolved
src/Messenger/IndexationRequestHandler.php Outdated Show resolved Hide resolved
src/Messenger/IndexationRequest.php Outdated Show resolved Hide resolved
$model = $this->fetchModel($message->getType(), $message->getId());

if (!$model) {
// ID does not exists, delete
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Member Author

Choose a reason for hiding this comment

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

To me, if fetchModel fail to found the entity/the data, that means it does not exists. If it did exists before, triggering a delete is a safe way to sync the fetchModel behavior with what we have in Elasticsearch.

Copy link
Member

Choose a reason for hiding this comment

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

But if the does not exist, if you delete it you basically delete nothing.

@damienalexandre
Copy link
Member Author

I just pushed a better version and the documentation.

There this a "MultipleIndexationRequest" message now, and a listener to group single messages in this one, allowing spool / bulk of all the changes into one message!

@damienalexandre
Copy link
Member Author

The pooling feature is tested successfully on a real world project 🎉

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
damienalexandre and others added 4 commits January 24, 2020 10:40
Co-Authored-By: Loïck Piera <pyrech@gmail.com>
Co-Authored-By: Loïck Piera <pyrech@gmail.com>
Copy link
Member

@lyrixx lyrixx left a comment

Choose a reason for hiding this comment

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

Really cool 👍

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
src/Messenger/IndexationRequestHandler.php Outdated Show resolved Hide resolved
src/Messenger/IndexationRequestHandler.php Outdated Show resolved Hide resolved
src/Messenger/IndexationRequestSpoolSubscriber.php Outdated Show resolved Hide resolved
src/Messenger/MultipleIndexationRequest.php Outdated Show resolved Hide resolved
$this->schedule($indexer, $message);
}

$indexer->flush();
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we check for errors, then requeue the MultipleIndexationRequest (or IndexationRequest) only with those that have failed?

Copy link
Member Author

Choose a reason for hiding this comment

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

With Messenger, any Exception is gonna re-queue the entire message.

In the case of Elasticsearch indexing, a bulk can fail only for one doc, and the other docs will be OK in the index (no rollback). I like the idea but it means we should edit the message that's being "re-queue"; I don't know how to do that yet :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried to implement it here: #18

@damienalexandre damienalexandre merged commit 5aefff8 into master Mar 8, 2020
@lyrixx lyrixx deleted the feat/messenger branch April 27, 2020 08:35
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.

None yet

4 participants