Skip to content
This repository has been archived by the owner on May 10, 2022. It is now read-only.

Support for SQS #3

Open
kasperg opened this issue Sep 9, 2019 · 9 comments
Open

Support for SQS #3

kasperg opened this issue Sep 9, 2019 · 9 comments
Labels
enhancement New feature or request

Comments

@kasperg
Copy link

kasperg commented Sep 9, 2019

Thanks for sharing this @Nyholm. Along with your excellent blogpost it helped me get started with Bref and Symfony Messenger.

I am also looking into Amazon SQS and it seems as if much of the setup would be the same as in this package:

  • Built on sroze/messenger-enqueue-transport
  • A message consumer function which will be invoked by the queue

Have you considered if/how SQS could work with this package?

One way would be to create a sister package brefphp/symfony-messenger-sqs. Another would be to expand this package to allow it to work with both queue systems.

@Nyholm
Copy link
Collaborator

Nyholm commented Sep 9, 2019

I’ve not worked with SQS but I’m sure it would be very similar to this package.

Maybe you could make a PR with some changes to this package so it works with SQS. Don’t make the perfect final solution, just do small modification so it works with SQS. (It is fine if it doest work with sns anymore).
Now we can clearly see if this should be a separate package OR if we should add some config to this package so it work with both.

Is that a fair way of moving forward?

@mnapoli mnapoli added the enhancement New feature or request label Sep 9, 2019
@mnapoli
Copy link
Member

mnapoli commented Sep 9, 2019

Hey, turns out I'm looking into this too!

AFAICT there is not much difference in the code, but I haven't tested, that's just my impression from looking at it.

@mnapoli
Copy link
Member

mnapoli commented Sep 9, 2019

And I was starting a new issue to discuss this but it's actually related to SQS:

I think we should not catch exceptions in Consumer:: consume(). At least for SQS, maybe not SNS.

The reason is that the Lambda SQS integration works like this:

  • SQS sends a few events to lambda
  • if lambda succeeds, the messages are marked as OK
  • il lambda fails, the messages are marked as failed
  • if failed, the messages can be optionally retried or put in a dead letter queue

If we always catch exceptions, we loose failure tracking and the features that go with it.

However I'm not sure about SNS. I think it is possible to have Lambda automatically put failed SNS messages into a SQS dead letter queue, which would support doing the same for SNS. But I'm not sure at all. I've read your article (http://developer.happyr.com/sns-retry) @Nyholm but it handles failures manually: if there's a native way to do that that might be a better (simpler and safer) option.

@kasperg
Copy link
Author

kasperg commented Sep 9, 2019

I think we should not catch exceptions in Consumer:: consume(). At least for SQS [...]

Agreed. Without having looked deeper into SQS or the Enqueue transport I would expect the Messenger retry strategies to work for handling of lambda failures.

How would you prefer that I proceed with this? I am fine with following the process outlined by @Nyholm but if you already have something cooking @mnapoli I am happy to take a look at that as well.

@mnapoli
Copy link
Member

mnapoli commented Sep 9, 2019

I am working on this at the moment, I'll share once I have something working.

@mnapoli
Copy link
Member

mnapoli commented Dec 19, 2019

Hey! I have released https://github.com/brefphp/symfony-messenger-sqs as a separate package.

The reason I created a separate package is that it works very differently. I tried to integrate SQS as described here, using the native Lambda-SQS behavior.

Symfony does not poll SQS, instead we rely on all the features AWS provides around SQS-Lambda. That also means that many features or Messenger are rendered obsolete (e.g. the retry count, error handling, etc.).

I think we could turn https://github.com/brefphp/symfony-messenger-sqs into a generic package that supports SQS, SNS and EventBridge. I worked with all 3 and they work very similarly here.

Let me know what you think.

@kasperg
Copy link
Author

kasperg commented Dec 19, 2019

I have not played around with it yet but based on the README it looks right to me and the code seems nice and simple.

One question: This package provides a Consumer.php handler. When using brefphp/symfony-messenger-sqs you have to create your own. Is there a reason for not providing an actual implementation in the package?

@mnapoli
Copy link
Member

mnapoli commented Dec 20, 2019

@kasperg yes that's a very good point! I haven't dived into Symfony flex recipes, I kind of gave up from the start with that because AFAIK you have to submit them to a 3rd party repository and you have no control whether they will be accepted or not. It also makes maintenance harder.

But it would be an improvement to do what is done in this repository.

@mnapoli
Copy link
Member

mnapoli commented Dec 20, 2019

Also I had to create the SQS package to be able to build it and distribute it quickly to a client, but now that it's working well I think we can go a step further: we could merge both repositories into one bref/symfony-messenger package that could support SQS, SNS and EventBridge.

But first I'd love to know what you think of the approach in that new package @Nyholm and see if that's the way we would want to go.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants