Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add method and class IDs to channel errors #447

Merged
merged 1 commit into from
Aug 12, 2018
Merged

Add method and class IDs to channel errors #447

merged 1 commit into from
Aug 12, 2018

Conversation

MitMaro
Copy link
Contributor

@MitMaro MitMaro commented Jul 30, 2018

What

This change adds a methodId and classId property to the channel errors.

Motivation

I have a higher level configuration for the topology that allows automatic deletion of exchanges and queues. I perform a deleteExchange/deleteQueue with ifUnused/ifEmpty options set, and against exchanges and queues that no longer exist. These operations will fail with an error in some cases. Currently it is only possible to determine if there was a precondition­failed error code (406) and not what operation failed. In the case of bad topology, for example a binding to a queue that does not exist, I produces a critical error fault in our monitoring system. In the case of a failure to delete an exchange or queue, I want to ignore the error, but currently cannot. With this change, errors will contain the class and method IDs, so that the source and type of error can be determined.

Notes

  • I've updated the existing tests to add checks for the class and method IDs where there was already an existing error.code check. I'm unsure on how or where to add any additional tests that may be needed.

@MitMaro
Copy link
Contributor Author

MitMaro commented Jul 31, 2018

Failing CI is because I used const in the code. This library is still testing against really old versions of Node. I will update to not use const.

Copy link
Collaborator

@squaremo squaremo left a comment

Choose a reason for hiding this comment

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

This seems like a reasonable requirement, and neatly done. Thanks! Will merge if you get the tests to pass OK (yes, it's written in very old JS ...).

@MitMaro
Copy link
Contributor Author

MitMaro commented Aug 7, 2018

Fixed that use of const by replacing with varand tests are passing this time.

Also, after re-reading my previous comment about the node versions, it may have came across as being a bit rude. If so, that was not my intent. :)

@MitMaro
Copy link
Contributor Author

MitMaro commented Aug 7, 2018

I also rebased onto the latest master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants