- Notifications
You must be signed in to change notification settings - Fork615
Add dedicated exception forbasic.return messages.#1832
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
Conversation
Fixes#1831This PR adds the `PublishReturnException` class that includes theoriginating exchange and routing key for a `basic.return` message. Itshould be backwards-compatible in the API.
Uh oh!
There was an error while loading.Please reload this page.
bording commentedMay 9, 2025
While it's good to add these properties to the exception for inspection, I'd also really like to see exception messages being added that display these property values in the message text. Those messages tend to get logged along with stack traces and provide useful information in logs. Custom properties on an exception are much less likely to make it into a log file. |
lukebakken commentedMay 9, 2025
@SzymonPobiega and@Gsantomaggio all set for a re-review, thanks. |
lukebakken commentedMay 9, 2025
Feel free to suggest a patch! Or, modify my PR directly@bording ... I think you have write access here 😸 |
4a64a83 to39397a6Compare39397a6 to5b62dffComparebording commentedMay 9, 2025
I pushed up a change to add exception messages. The interesting one is for the return messages because there really isn't any useful information to include for a nack. |
Add more conditions to the testSigned-off-by: Gabriele Santomaggio <G.santomaggio@gmail.com>
Signed-off-by: Gabriele Santomaggio <G.santomaggio@gmail.com>
lukebakken left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks everyone!
1e6f8ab intomainUh oh!
There was an error while loading.Please reload this page.
Fixes#1831
This PR adds the
PublishReturnExceptionclass that includes the originating exchange and routing key for abasic.returnmessage. It should be backwards-compatible in the API.