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] [Sqs] AddAddFifoStamp middleware#48095

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
nicolas-grekas merged 1 commit intosymfony:6.4fromtyx:feat/fifo-stamp-middleware
Oct 19, 2023

Conversation

tyx
Copy link
Contributor

@tyxtyx commentedNov 3, 2022•
edited
Loading

QA
Branch?6.4
Bug fix?no
New feature?yes
Deprecations?no
LicenseMIT
Doc PRon my way

Currently it is not very easy to use fifo stamp as you need to go down to the envelope when you often handle only message in your app.

We also think that the message should be responsible to choose its own groupId.

So we come with this very simple middleware and 2 interfaces. We use it in our own system since 3 months.

It allows to keep messenger concept outside our messages while continuing to benefit from it.

Example

To group message by user for example

Current

class SmsNotification {publicfunction__construct(privatestring$userId) {}}//....$bus->dispatch(newSmsNotification($userId), [newAmazonSqsFifoStamp($userId)    ]);

After

class SmsNotificationimplements WithMessageGroupId {publicfunction__construct(privatestring$userId) {}publicfunctionmessageGroupId()     {return$this->userId;    }}//....$bus->dispatch(newSmsNotification($userId));

Usage

It can be enabled with this kind of config

# config/packages/messenger.yamlframework:messenger:buses:event_bus:default_middleware:allow_no_handlers:truemiddleware:          -'Symfony\Component\Messenger\Bridge\AmazonSqs\Middleware\AddFifoStampMiddleware'

mnapoli, shouze, and valtzu reacted with thumbs up emojishouze reacted with rocket emoji
@carsonbotcarsonbot added this to the6.2 milestoneNov 3, 2022
@carsonbotcarsonbot changed the title[Messenger][Sqs] Add AddFifoStamp middleware[Messenger] [Sqs] Add AddFifoStamp middlewareNov 3, 2022
@tyxtyxforce-pushed thefeat/fifo-stamp-middleware branch from8c0a310 to161d613CompareNovember 3, 2022 13:50
@welcoMatticwelcoMattic modified the milestones:6.2,6.3Nov 3, 2022
@carsonbot
Copy link

Hey!

I think@filkaris has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@tyx
Copy link
ContributorAuthor

tyx commentedDec 13, 2022

Hello there !

I'll have some time soon to write some documentation but I'd like to be sure that the feature is interesting :D

@nicolas-grekasnicolas-grekas modified the milestones:6.3,6.4May 23, 2023
@nicolas-grekasnicolas-grekas added the ❄️ Feature FreezeImportant Pull Requests to finish before the next Symfony "feature freeze" labelOct 16, 2023
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.

Thank you

I like this PR.

What are the pros/cons to use one interface instead of two? (and make the return type nullable)

tyx reacted with heart emoji
@tyx
Copy link
ContributorAuthor

tyx commentedOct 18, 2023•
edited
Loading

What are the pros/cons to use one interface instead of two? (and make the return type nullable)

Hi@Nyholm !

In fact, deduplication is "optionnal" in sqs world. For example in our situation we rarely use it.

According to the documentation

Providing the message deduplication ID
The producer should provide message deduplication ID values for each message in the following scenarios:
Messages sent with identical message bodies that Amazon SQS must treat as unique.
Messages sent with identical content but different message attributes that Amazon SQS must treat as unique.
Messages sent with different content (for example, retry counts included in the message body) that Amazon SQS must treat as duplicates.

https://docs.aws.amazon.com/en_gb/AWSSimpleQueueService/latest/SQSDeveloperGuide/using-messagededuplicationid-property.html

So 2 interfaces reduce the boilercode to write by the end user (aka empty method deduplicationId)

xfifix reacted with heart emoji

@Nyholm
Copy link
Member

So 2 interfaces reduce the boilercode to write by the end user

Yeah, true. Thank you

Please add a doc block and I am super happy with this PR.

@OskarStarkOskarStark changed the title[Messenger] [Sqs] Add AddFifoStamp middleware[Messenger][Sqs] AddAddFifoStamp middlewareOct 18, 2023
@carsonbotcarsonbot changed the title[Messenger][Sqs] AddAddFifoStamp middleware[Messenger] [Sqs] AddAddFifoStamp middlewareOct 18, 2023
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

Can you please add a before/after to the description of your PR? How can one do today, and your proposal?

tyx reacted with thumbs up emoji
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

One minor comment and GTM!

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedOct 19, 2023•
edited
Loading

One last thing: please update the PR description to show how the middleware could be enabled.

tyx reacted with thumbs up emoji

@tyxtyxforce-pushed thefeat/fifo-stamp-middleware branch fromfe008fa to707b5a4CompareOctober 19, 2023 14:26
nicolas-grekas added a commit that referenced this pull requestOct 19, 2023
This PR was merged into the 6.4 branch.Discussion----------DX: nullable_type_declaration| Q             | A| ------------- | ---| Branch?       | 6.4| Bug fix?      | no| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tickets       | n/a| License       | MIThttps://cs.symfony.com/doc/rules/language_construct/nullable_type_declaration.html#example-1requested in#48095 (comment)I do not put this rule in ruleset, as it requires PHP 8 - if you want to move it to ``@Symfony`` ruleset, let me knowThere is concern how to have this rule configured and have PHPUnit bridge supporting 7.4, but I think I will leave it unresolved for now.Commits-------6aff9d9 DX: nullable_type_declaration
@nicolas-grekas
Copy link
Member

Thank you@tyx.

tyx reacted with hooray emoji

@nicolas-grekasnicolas-grekas merged commite942c1c intosymfony:6.4Oct 19, 2023
@tyxtyx deleted the feat/fifo-stamp-middleware branchOctober 19, 2023 16:32
This was referencedOct 21, 2023
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@NyholmNyholmNyholm approved these changes

@keraduskeraduskeradus left review comments

@OskarStarkOskarStarkOskarStark approved these changes

@xabbuhxabbuhAwaiting requested review from xabbuh

@lyrixxlyrixxAwaiting requested review from lyrixx

@dunglasdunglasAwaiting requested review from dunglas

@ycerutoycerutoAwaiting requested review from yceruto

@welcoMatticwelcoMatticAwaiting requested review from welcoMattic

@chalasrchalasrAwaiting requested review from chalasr

@jderussejderusseAwaiting requested review from jderusse

Assignees
No one assigned
Labels
FeatureMessenger❄️ Feature FreezeImportant Pull Requests to finish before the next Symfony "feature freeze"Status: Reviewed
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.

7 participants
@tyx@carsonbot@Nyholm@nicolas-grekas@OskarStark@keradus@welcoMattic
[8]ページ先頭
©2009-2025 Movatter.jp