Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Messenger] Add SQS transport#32454

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:masterfromjderusse:messenger-sqs
Feb 10, 2020
Merged

Conversation

@jderusse
Copy link
Member

@jderussejderusse commentedJul 9, 2019•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketsN/A
LicenseMIT
Doc PRTODO

This PR add the AWS SQS transport in messenger.

It also add aDisconnectedReceiverInterface 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

chalasr, Taluu, adamwojs, JanTvrdik, andreybolonin, yceruto, krewetka, plozmun, deguif, Hemric, and 5 more reacted with thumbs up emojideguif, andreybolonin, Nek-, phansys, JoppeDC, mykiwi, rimoi, and gmoreira reacted with hooray emoji
Copy link
Member

@javiereguiluzjaviereguiluz left a comment

Choose a reason for hiding this comment

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

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

@jderussejderusse changed the base branch frommaster to4.4July 9, 2019 11:14
@nicolas-grekasnicolas-grekas added this to thenext milestoneJul 10, 2019
@Tobion
Copy link
Contributor

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
Copy link
Member

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.

krewetka and rimoi reacted with thumbs up emoji

@jderussejderusseforce-pushed themessenger-sqs branch 2 times, most recently from4949046 to8907adcCompareAugust 7, 2019 12:01
@fabpot
Copy link
Member

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.

krewetka reacted with thumbs up emoji

@jderussejderusseforce-pushed themessenger-sqs branch 2 times, most recently fromac6e397 tobdafae5CompareAugust 9, 2019 08:20
@bendavies
Copy link
Contributor

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

@jderusse
Copy link
MemberAuthor

@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

krewetka and jderusse reacted with thumbs up emoji

Copy link
Member

@NyholmNyholm left a comment

Choose a reason for hiding this comment

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

Im happy with this PR. Great work!

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

@jderussejderusseforce-pushed themessenger-sqs branch 2 times, most recently frombf7f60c to86ffde5CompareJanuary 29, 2020 13:16
@fabpot
Copy link
Member

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

Taluu reacted with thumbs up emoji

@chalasr
Copy link
Member

Looks good to me.

@fabpot
Copy link
Member

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

@jderusse
Copy link
MemberAuthor

yes it's ok for me

@bendavies
Copy link
Contributor

bendavies commentedFeb 10, 2020•
edited
Loading

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

This namesapce could beAmazon instead ofAmazonSqs here too, with all classes renamed to includeSqs*, just like mailer does.

@Nyholm
Copy link
Member

Excellent point. However, I thinkAmazonSqs 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 haveAmazonSqs andAmazonSns.

@bendavies
Copy link
Contributor

bendavies commentedFeb 10, 2020•
edited
Loading

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

@jderusse
Copy link
MemberAuthor

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

Nyholm and bendavies reacted with thumbs up emoji

@fabpot
Copy link
Member

Thank you@jderusse.

Nyholm, lyrixx, chalasr, deguif, and shambhu-valuelabs reacted with hooray emoji

fabpot added a commit that referenced this pull requestFeb 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        | TODOThis 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
@fabpotfabpot merged commitc226479 intosymfony:masterFeb 10, 2020
@jderussejderusse deleted the messenger-sqs branchMarch 5, 2020 20:03
@nicolas-grekasnicolas-grekas modified the milestones:next,5.1May 4, 2020
@fabpotfabpot mentioned this pull requestMay 5, 2020
@tomcoonen
Copy link

How are we on the docs on this? Can I do anything to help?

@Nyholm
Copy link
Member

Nyholm commentedJun 4, 2020•
edited
Loading

Yes please. There is an issuesymfony/symfony-docs#13093 for that.

I'll be happy to help reviewing if you create content.

Let's discuss further onsymfony/symfony-docs#13093

EDIT: I see that there is already an open PR:symfony/symfony-docs#13328

@tomcoonen could you help review that?

tomcoonen reacted with thumbs up emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@javiereguiluzjaviereguiluzjaviereguiluz left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@stofstofstof left review comments

@NyholmNyholmNyholm approved these changes

@fabpotfabpotfabpot approved these changes

@chalasrchalasrchalasr approved these changes

+4 more reviewers

@srozesrozesroze left review comments

@deguifdeguifdeguif left review comments

@gmponosgmponosgmponos left review comments

@medinaemedinaemedinae left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

5.1

Development

Successfully merging this pull request may close these issues.

16 participants

@jderusse@Tobion@weaverryan@fabpot@bendavies@Nyholm@chalasr@tomcoonen@javiereguiluz@nicolas-grekas@stof@sroze@deguif@gmponos@medinae@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp