brefphp / symfony-messenger

Bridge to use Symfony Messenger on AWS Lambda with Bref
MIT License
72 stars 22 forks source link

Support partial batch failure #58

Closed t-richard closed 2 years ago

t-richard commented 2 years ago

This allows the Symfony Messenger integration to benefit from "partial batch failures" with SQS.

This is done via a new argument in the service declaration (and proper SQS config ofc).

Bref\Symfony\Messenger\Service\Sqs\SqsConsumer:
        public: true
        autowire: true
        arguments:
            # Pass the transport name used in config/packages/messenger.yaml
            $transportName: 'async'
+           $partialBatchFailure: true
If you want to try this, in `composer.json` ```json { "require": { ... "bref/symfony-messenger": "dev-partial" }, "repositories": [ { "type": "vcs", "url": "https://github.com/t-richard/symfony-messenger" } ] } ```

Closes #57

tyx commented 2 years ago

Thanks @t-richard it looks perfect IMO !

t-richard commented 2 years ago

@tyx any chance you got to test this?

I'd love to get this merged and feedback would be welcomed to confirm it's working as intended :slightly_smiling_face:

tyx commented 2 years ago

Hi ! I don't have so much time for now. But I will test it ASAP for sure.

t-richard commented 2 years ago

I think this can be merged.

I'll try to release a 0.5 next week with this.

Any last thoughts?

t-richard commented 2 years ago

One case that is not covered here is FIFO queues.

AWS docs says about partial batch failure:

If you're using this feature with a FIFO queue, your function should stop processing messages after the first failure and return all failed and unprocessed messages in batchItemFailures. This helps preserve the ordering of messages in your queue.

Maybe we should handle this here too?

shouze commented 2 years ago

One case that is not covered here is FIFO queues.

AWS docs says about partial batch failure:

If you're using this feature with a FIFO queue, your function should stop processing messages after the first failure and return all failed and unprocessed messages in batchItemFailures. This helps preserve the ordering of messages in your queue.

Maybe we should handle this here too?

@t-richard Could be a good idea yes, to ensure that good pratice is applied by design. A good way is to throw a fatal exception to me, WDYT?

t-richard commented 2 years ago

@shouze I pushed a new commit with support to handle FIFO queues properly.

Throwing a fatal exception would fail the whole batch and put us in the current behaviour (ie. no partial batch failure).

What I'm doing here is running markAsFailed on all following messages in the batch if there is a failure.

Let me know what you think.

shouze commented 2 years ago

Indeed @t-richard looks like the right behavior!

mnapoli commented 2 years ago

Thanks! We should be good to merge then, could you rebase (or merge)? There are conflicts preventing to merge.

t-richard commented 2 years ago

Yes, I'll rebase, merge and release.

Do you think we should release this as part of 0.4.5 or 0.5.0 ?

mnapoli commented 2 years ago

Update following discussion: let's go with 0.5.0.