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] Add SQS transport #32454

Merged
merged 1 commit into from Feb 10, 2020
Merged

[Messenger] Add SQS transport #32454

merged 1 commit into from Feb 10, 2020

Conversation

@jderusse
Copy link
Contributor

jderusse commented Jul 9, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets N/A
License MIT
Doc PR TODO

This PR add the AWS SQS transport in messenger.

It also add a DisconnectedReceiverInterface that allows the worker to release not-proceeded message (which are not automatically released in SQS and have to wait a TTL). Tell me if you prefer to move it in a dedicated PR.

accepted DNS:

  • sqs://default/accountId/queueName
  • sqs://default/queueName
  • sqs://default/queueName?region=us-east-2
  • sqs://my_custome_endpoint:12345/queueName?sslmode=disabled

To reduce AWS costs, the implementation performs a long polling call and prefetch several messages.
TO get ~real time worker, one could use ./bin/console messenger:consume --sleep 0.001

@jderusse jderusse requested a review from sroze as a code owner Jul 9, 2019
@jderusse jderusse force-pushed the jderusse:messenger-sqs branch 5 times, most recently from 453a19d to 6d4de47 Jul 9, 2019
Copy link
Member

javiereguiluz left a comment

Very nice PR and a very useful feature. Thanks Jérémy. I left some very minor comments.

@jderusse jderusse force-pushed the jderusse:messenger-sqs branch from 6d4de47 to 3c07d2e Jul 9, 2019
@jderusse jderusse changed the base branch from master to 4.4 Jul 9, 2019
@nicolas-grekas nicolas-grekas added this to the next milestone Jul 10, 2019
@Tobion

This comment has been minimized.

Copy link
Member

Tobion commented Jul 11, 2019

To me we should prioritize finalizing the messenger API for 4.4 and fixing all the open messenger problems. So adding another transport should come after that.

@weaverryan

This comment has been minimized.

Copy link
Member

weaverryan commented Jul 17, 2019

To me we should prioritize finalizing the messenger API for 4.4 and fixing all the open messenger problems. So adding another transport should come after that.

I don't want that to block this PR... as we won't be 100% sure of API changes until feature freeze for 4.4. I think we should push this forward.

@jderusse jderusse force-pushed the jderusse:messenger-sqs branch 2 times, most recently from 4949046 to 8907adc Aug 7, 2019
@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Aug 9, 2019

I'm also for merging this PR rather sooner than later, so that when we evolve the code for 4.4 stabilization, it can be included as well in the changes.

@jderusse jderusse force-pushed the jderusse:messenger-sqs branch from 8907adc to 5e9073a Aug 9, 2019
@jderusse jderusse force-pushed the jderusse:messenger-sqs branch 2 times, most recently from ac6e397 to bdafae5 Aug 9, 2019
@bendavies

This comment has been minimized.

Copy link
Contributor

bendavies commented Aug 9, 2019

sqs has first class support for 'failures', dead letter queues. is it possible to support them somehow?

@jderusse

This comment has been minimized.

Copy link
Contributor Author

jderusse commented Aug 9, 2019

@bendavies In this PR I included the minimal features required by Messenger. I think that behaviors like FIFO, Deadletter, could be added in dedicated PR

@jderusse jderusse force-pushed the jderusse:messenger-sqs branch from bdafae5 to 722315d Sep 17, 2019
@jderusse jderusse force-pushed the jderusse:messenger-sqs branch from 2366e2b to c6e92d5 Jan 25, 2020
Copy link
Member

Nyholm left a comment

Thank you for moving this to a separate package.

@jderusse jderusse force-pushed the jderusse:messenger-sqs branch from 36a1790 to e60863d Jan 26, 2020
Copy link
Member

chalasr left a comment

Can't wait to play with this :)

@jderusse jderusse force-pushed the jderusse:messenger-sqs branch 2 times, most recently from 2f82cb9 to 58b4cec Jan 27, 2020
@Nyholm
Nyholm approved these changes Jan 29, 2020
Copy link
Member

Nyholm left a comment

Im happy with this PR. Great work!

Could you rebase your PR to get rid of the Doctrine related changes?

@jderusse jderusse force-pushed the jderusse:messenger-sqs branch 2 times, most recently from bf7f60c to 86ffde5 Jan 29, 2020
@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Feb 3, 2020

@jderusse Can you finish this one so that it can be merged? Ping me when it's done :) Thank you.

@jderusse jderusse force-pushed the jderusse:messenger-sqs branch from 86ffde5 to 5c7d8fb Feb 3, 2020
@jderusse jderusse force-pushed the jderusse:messenger-sqs branch from 5c7d8fb to c226479 Feb 4, 2020
@chalasr

This comment has been minimized.

Copy link
Member

chalasr commented Feb 9, 2020

Looks good to me.

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Feb 10, 2020

@jderusse Is it ready on your side? Perhaps a last rebase before merging?

@jderusse

This comment has been minimized.

Copy link
Contributor Author

jderusse commented Feb 10, 2020

yes it's ok for me

@fabpot
fabpot approved these changes Feb 10, 2020
@bendavies

This comment has been minimized.

Copy link
Contributor

bendavies commented Feb 10, 2020

I noticed that the namespaces are at odds with Mailer here, where the namespace is Amazon.
https://github.com/symfony/symfony/tree/master/src/Symfony/Component/Mailer/Bridge/Amazon/Transport

This namesapce could be Amazon instead of AmazonSqs here too, with all classes renamed to include Sqs*, just like mailer does.

@Nyholm

This comment has been minimized.

Copy link
Member

Nyholm commented Feb 10, 2020

Excellent point. However, I think AmazonSqs is good because it could be a future to include a SNS transport. If so, it would be in a separate package. So it make sense to have AmazonSqs and AmazonSns.

@bendavies

This comment has been minimized.

Copy link
Contributor

bendavies commented Feb 10, 2020

Sure, but my point is that, that's not how mailer has done it. all impls are in one Amazon namespace:
https://github.com/symfony/symfony/tree/master/src/Symfony/Component/Mailer/Bridge/Amazon/Transport
You could argue they are all SES, but...

@jderusse

This comment has been minimized.

Copy link
Contributor Author

jderusse commented Feb 10, 2020

In my opinion,the difference is AWS provides a single mailing service but several messaging services. (SQS, SNS, Kinesis).

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Feb 10, 2020

Thank you @jderusse.

fabpot added a commit that referenced this pull request Feb 10, 2020
This PR was merged into the 5.1-dev branch.

Discussion
----------

[Messenger] Add SQS transport

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | N/A
| License       | MIT
| Doc PR        | TODO

This PR add the AWS SQS transport in messenger.

It also add a `DisconnectedReceiverInterface` that allows the worker to release not-proceeded message (which are not automatically released in SQS and have to wait a TTL). Tell me if you prefer to move it in a dedicated PR.

accepted DNS:
- `sqs://default/accountId/queueName`
- `sqs://default/queueName`
- `sqs://default/queueName?region=us-east-2`
- `sqs://my_custome_endpoint:12345/queueName?sslmode=disabled`

To reduce AWS costs, the implementation performs a long polling call and prefetch several messages.
TO get ~real time worker, one could use `./bin/console messenger:consume --sleep 0.001`

Commits
-------

c226479 [Messenger] Add SQS transport
@fabpot fabpot merged commit c226479 into symfony:master Feb 10, 2020
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details
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.

None yet

You can’t perform that action at this time.