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.

Sign up for GitHub

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

Jump to bottom

[shopsys] added core for dispatch/consume system #2907

Merged
merged 8 commits into from Nov 7, 2023
Merged

[shopsys] added core for dispatch/consume system #2907

merged 8 commits into from Nov 7, 2023

Conversation

grossmannmartin
Copy link
Member

@grossmannmartin grossmannmartin commented Oct 30, 2023 •
edited by github-actions bot

Q A
Description, reason for the PR Prepares the message dispatch/consume for future usage. More description follows
New feature Yes
BC breaks No
Fixes issues ...
Have you read and signed our License Agreement for contributions? Yes

What is done:

  • AbstractMessageDispatcher should be used as a base class for all future dispatchers. Provides the necessary dependencies and is configured automatically
  • messages are truly sent to the queue at the end of the request/console-command/processed-message – after the master transaction is committed
  • services using internal cache now implement ResetInterface, so they are automatically cleared on each message (prevent stale in-memory data)
  • when entity manager is closed, the worker is automatically restarted

🌐 Live Preview:

@grossmannmartin grossmannmartin force-pushed the mg-messenger-messages branch 3 times, most recently from 7c926aa to d68fc0c Compare October 30, 2023 15:23
@grossmannmartin grossmannmartin marked this pull request as ready for review October 30, 2023 15:24
@grossmannmartin grossmannmartin force-pushed the mg-messenger-messages branch 2 times, most recently from 61cc905 to ef2ebac Compare October 31, 2023 19:05
vitek-rostislav
vitek-rostislav requested changes Nov 1, 2023
View reviewed changes
Copy link
Contributor

@vitek-rostislav vitek-rostislav left a comment

Choose a reason for hiding this comment

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

Hi, nice job, I have added just a few notes

packages/framework/src/Component/Messenger/ExampleDispatcher.php Outdated Show resolved Hide resolved
packages/framework/src/Component/Messenger/DeferredEnvelope/DeferEnvelopeMiddleware.php Outdated Show resolved Hide resolved
.../framework/src/Component/Messenger/DeferredEnvelope/DispatchCollectedEnvelopesSubscriber.php Outdated Show resolved Hide resolved
.../framework/src/Component/Messenger/DeferredEnvelope/DispatchCollectedEnvelopesSubscriber.php Outdated Show resolved Hide resolved
.../framework/src/Component/Messenger/DeferredEnvelope/DispatchCollectedEnvelopesSubscriber.php Outdated Show resolved Hide resolved
packages/product-feed-heureka/src/Model/FeedItem/HeurekaProductDataBatchLoader.php Show resolved Hide resolved
UPGRADE-14.0.md Outdated Show resolved Hide resolved
@vitek-rostislav
Copy link
Contributor

vitek-rostislav commented Nov 3, 2023 •
edited

feedback from testing:

  • We should respect the transactional master request - when there is an exception, data in DB are rollbacked. In such a case, we should not process the dispatched messages

✔️ When there is an error that causes the closing of the entity manager, the message processing is stopped immediately, and the message is not re-enqueued.

✔️ the messages are actually dispatched on kernel response, on console terminate, and after another message is processed - so it is ensured the data are already committed in the database when handling the message.

✔️ the internal service cache is cleared after a message is processed.

One thing that surprised me - when the message processing fails on a regular exception, it is then enqueued 3 more times - I believe that is ok. For a reason, I was expecting 3 attempts in total but actually, there are 4 attempts to process the message.

@grossmannmartin
Copy link
Member Author

feedback from testing:

  • We should respect the transactional master request - when there is an exception, data in DB are rollbacked. In such a case, we should not process the dispatched messages

Thanks for testing. I have improved this scenario :)

One thing that surprised me - when the message processing fails on a regular exception, it is then enqueued 3 more times - I believe that is ok. For a reason, I was expecting 3 attempts in total but actually, there are 4 attempts to process the message.

🤔 hmmm... I'm not sure. According to this

Snímek obrazovky 2023-11-05 v 20 12 24

it seems like it should be retried 3 times (that makes the 4 attempts in general)

but then this

Snímek obrazovky 2023-11-05 v 20 14 34

talks about three failings...

well, all in all, we can adjust the behavior with the max_retries configuration

@vitek-rostislav
Copy link
Contributor

Thanks for testing. I have improved this scenario :)

Awesome, thank you, I have re-tested it, works like a charm, so I am moving the task further in the process 😉

@grossmannmartin
created abstract message dispatcher
ce5cac8
@grossmannmartin
added example dispatcher
a7bcd45
@grossmannmartin
ensure messages are handled "at the end" of the current process
8bb9221 
- "at the end":
    - on kernel response (i.e. after a DB transaction is committed) if there is a request
    - on console terminate if in console
    - on message handled if in consumer
- custom DeferEnvelopeMiddleware stores all dispatched messages and stops propagation to the other middlewares
   - the middleware is actually called twice, however, the storing is done only when the message is dispatched, not at its receival
   - see https://symfony.com/doc/current/messenger.html#middleware
- DispatchCollectedEnvelopesSubscriber takes all the stored messages and redispatch them for further processing
   - custom stamp DeferredEnvelopeStamp ensures that this time these messages will be sent to the queue
@grossmannmartin
internal cache of services is now cleaned after consume
fd355e9 
- on services implementing ResetInterface, the reset() method is called automatically after each message
- see https://symfony.com/doc/current/messenger.html#stateless-worker
@grossmannmartin
consumer is now restarted on closed entity manager exception
cf7c23d
@grossmannmartin
added upgrade notes
61ac194
@grossmannmartin
emails are now not sent in test env
5b10902
@grossmannmartin
added rabbitmq to cypress tests
1b4c11a
@grossmannmartin grossmannmartin merged commit af55be6 into 14.0 Nov 7, 2023
16 of 18 checks passed
@grossmannmartin grossmannmartin deleted the mg-messenger-messages branch November 7, 2023 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vitek-rostislav vitek-rostislav vitek-rostislav approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@grossmannmartin @vitek-rostislav