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

bug: pass Conn.connection_parameters to pika.BlockingConnection #80

Merged
merged 4 commits into from
Oct 24, 2024

Conversation

mackenzie-grimes-noaa
Copy link
Contributor

Linear Issue

N/A

Changes

  • Small bug fix for RabbitMQ Publisher and Consumer class __init__s to pass the right argument type to pika.BlockingConnection.

Explanation

The rabbitmq class property Conn has a property connection_parameters that transforms the NamedTuple that a caller would have created into a pika.ConnectionParameters which pika.BlockingConnection expects. However, we forgot to use this transformation throughout rabbitmq_utils.py.

For example, passing an Conn instance to Publisher() was throwing an uncaught exception:

                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/mackenzie.grimes/dev/idss-engine-commons/python/idsse_common/idsse/common/rabbitmq_utils.py", line 434, in __init__
    self.connection = BlockingConnection(conn_params)

AttributeError: 'str' object has no attribute 'connection_attempts'

because the actual Conn (type: NamedTuple) was being passed to pika.BlockingConnection.

@mackenzie-grimes-noaa
Copy link
Contributor Author

@Geary-Layne I'm not sure if you intended this change, but Consumer and Publisher inits are currently expecting a pika.ConnectionParameters type object.

If you meant to expect an idsse commons Conn like the previous RabbitMqUtils did, I believe we need this change to transform the Conn into a ConnectionParameters before passing to BlockingConnection.

If not, and you really did want require an arg of pika.ConnectionParameters, we'll need to make sure all services call Conn.connection_parameters before passing to Publisher or Consumer.

Copy link
Contributor

@Geary-Layne Geary-Layne left a comment

Choose a reason for hiding this comment

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

I'm not sure which is best, expecting a Conn or a ConnectionParams, but lets go with Conn, and thus the change you have here. Thanks

@Geary-Layne Geary-Layne merged commit 890bd90 into main Oct 24, 2024
2 checks passed
@Geary-Layne
Copy link
Contributor

I merged it since I would like to use these changes when fixing the other repos

@mackenzie-grimes-noaa mackenzie-grimes-noaa deleted the bug/rabbitmq-utils-conn-params branch October 24, 2024 17:00
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