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

[Messenger] multiple failed transports support #34979

Open
wants to merge 4 commits into
base: master
from

Conversation

@monteiro
Copy link
Contributor

monteiro commented Dec 14, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #34911
License MIT
Doc PR MISSING

Strategy applied

  • Pass a map of transports and failed transports to the SendFailedMessageToFailureTransportListener. This way we re-use the same listener.
  • Local failed transport has more priority than a global failed transport defined.

Configuration example

# config/packages/messenger.yaml
framework:
    # no need to set failed transport globally if I want a specific behaviour per transport.
    failure_transport: failed # all transports have this failed transport
    messenger:
        transports:
            failed: 'doctrine://default?queue_name=failed'
            failed_important: 'doctrine://default?queue_name=failed_important'
            async:
                dsn: '%env(MESSENGER_TRANSPORT_DSN)%'
                failure_transport: failed # takes precedence over the global defined "failed_transport"
                retry_strategy:
                    max_retries: 3
                    delay: 1000
                    multiplier: 2
            async_no_failure_transport: # it will use the global defined transport if no one is defined.
                dsn: '%env(MESSENGER_TRANSPORT_DSN)%'
                retry_strategy:
                    max_retries: 3
                    delay: 1000
                    multiplier: 2
            async_send_specific_failure_queue:
                dsn: '%env(MESSENGER_TRANSPORT_DSN)%'
                failed_transport: failed_important # takes precedence over the global defined "failed_transport"
                retry_strategy:
                    max_retries: 3
                    delay: 1000
                    multiplier: 2

More information on issue #34911

What needs to be done so this can be merged:

  • validate strategy
  • update src/**/CHANGELOG.md files
  • update tests to cover all cases
  • create doc PR
@nicolas-grekas nicolas-grekas added this to the next milestone Dec 15, 2019
@monteiro monteiro force-pushed the monteiro:messenger-multiple-failed-transports branch from 9a7eb40 to 099a646 Dec 27, 2019
@monteiro monteiro requested review from dunglas, lyrixx and xabbuh as code owners Dec 27, 2019
@monteiro monteiro changed the base branch from 4.4 to master Dec 27, 2019
@monteiro monteiro force-pushed the monteiro:messenger-multiple-failed-transports branch from 93c5188 to f995ffa Dec 27, 2019
@monteiro monteiro changed the title messenger: multiple failed transports support [Messenger] multiple failed transports support Jan 2, 2020
@monteiro

This comment has been minimized.

Copy link
Contributor Author

monteiro commented Jan 20, 2020

@sroze thanks a lot for the review.

[Messenger] Multiple failure transports support

fix case when there is no failure transport defined

avoid BC

rebase with master

php-cs-fix

add multiple failed transport support for failed commands

add support for specific failed transports with failed commands
@monteiro monteiro force-pushed the monteiro:messenger-multiple-failed-transports branch from 5f3e52f to 98eaef7 Mar 25, 2020
monteiro added 3 commits Mar 25, 2020
@monteiro

This comment has been minimized.

Copy link
Contributor Author

monteiro commented Mar 25, 2020

Can someone help me with the review?

I was very careful in order to not break any command or any class by extending some classes with more constructor parameters or adding more options to commands (the failed commands).

I probably need more tests on the FrameworkBundle (extension part).

@monteiro monteiro requested a review from sroze Mar 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.