[Messenger] Add "non sendable" stamps #31471
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4a027a1 to
42e4158
Compare
Tobion
reviewed
May 11, 2019
src/Symfony/Component/Messenger/Transport/Serialization/RemoveNonSendableStampsTrait.php
Outdated
Show resolved
Hide resolved
Tobion
reviewed
May 11, 2019
src/Symfony/Component/Messenger/Transport/Serialization/RemoveNonSendableStampsTrait.php
Outdated
Show resolved
Hide resolved
sroze
reviewed
May 12, 2019
src/Symfony/Component/Messenger/Transport/Serialization/RemoveNonSendableStampsTrait.php
Outdated
Show resolved
Hide resolved
42e4158 to
37a43d6
Compare
Fixes a bug where Symfony serialization of the AmqpReceivedStamp sometimes caused problems.
37a43d6 to
34e7781
Compare
|
Moved implementation to a new I've only added the new "non-sendable stamp interface" to 3 transport-specific "received" stamps. Generally speaking, I think that stamps should be sent, unless there is a clear reason otherwise. Allowing stamps to be sendable (the default) can make the messages bigger when being retried after failure. But they also allow for a really beautiful "history" as you can look at an Envelope and see exactly what happened to it over time. Anyways, this is ready to go! |
|
This is still ready to go. |
|
Thank you @weaverryan. |
fabpot
added a commit
that referenced
this pull request
May 21, 2019
This PR was merged into the 4.3 branch. Discussion ---------- [Messenger] Add "non sendable" stamps | Q | A | ------------- | --- | Branch? | 4.3 | Bug fix? | yes | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #31460 | License | MIT | Doc PR | not needed Fixes a bug where Symfony serialization of the AmqpReceivedStamp sometimes caused problems. It's still a mystery why the `AmqpReceivedStamp` caused a segfault *sometimes* when going through the Symfony serializer or the `VarDumper`. But, that stamp really didn't need to be sent on redelivery anyways. I don't love making the removal the responsibility of the serializers, but it didn't work well anywhere else. Cheers! Commits ------- 34e7781 Adding a new NonSendableStampInterface to avoid sending certain stamps
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes a bug where Symfony serialization of the AmqpReceivedStamp sometimes caused problems.
It's still a mystery why the
AmqpReceivedStampcaused a segfault sometimes when going through the Symfony serializer or theVarDumper. But, that stamp really didn't need to be sent on redelivery anyways.I don't love making the removal the responsibility of the serializers, but it didn't work well anywhere else.
Cheers!