Publish to multiple RabbitMQ queues #5387
Conversation
tests/core/test_broker.py
Outdated
| ("rasa_core_events", None, ["rasa_core_events"]), # default case | ||
| (["q1", "q2"], None, ["q1", "q2"]), # supplying a list for `queue` works too | ||
| ("q1", "q2", ["q2"]), # `queues` arg supplied, takes precedence | ||
| ("q1", ["q2", "q3"], ["q2", "q3"]), # same, but with a list | ||
| (None, "q1", ["q1"]), # only supplying `queues` works |
There was a problem hiding this comment.
Maybe we want to assert something also about whether a UserWarning should be shown for each case
There was a problem hiding this comment.
can we also do a (None, None, ["rasa_core_events"]) case where the user didn't provide any queue(s) configuration?
There was a problem hiding this comment.
not really because that would override the default argument queues=("rasa_core_events",). I'll add (None, ["rasa_core_events"], ["rasa_core_events"]) though which is the default case
|
I give it a review tomorrow |
| @@ -72,7 +73,7 @@ Here is how you add it using Python code: | |||
| pika_broker = PikaEventBroker('localhost', | |||
| 'username', | |||
| 'password', | |||
| queue='rasa_core_events') | |||
| queues=['rasa_events']) | |||
There was a problem hiding this comment.
let's better add this to the changelog. Could be that some people build on this default value 🤔
There was a problem hiding this comment.
or is this only changed in the docs?
There was a problem hiding this comment.
yeah I deliberately only changed it in the docs for now - we still have rasa_core_events as the default for backwards compatibility. maybe update this in 2.0?
There was a problem hiding this comment.
sounds good 👍 Should we create an issue for this?
rasa/core/brokers/pika.py
Outdated
| Attributes: | ||
| host: Pika host. | ||
| username: Username for authentication with Pika host. | ||
| password: Password for authentication with Pika host. | ||
| port: port of the Pika host. | ||
| queues: Pika queues to declare and publish to. | ||
| should_keep_unpublished_messages: Whether or not the event broker should | ||
| maintain a queue of unpublished messages to be published later in | ||
| case of errors. | ||
| raise_on_failure: Whether to raise an exception if publishing fails. If | ||
| `False`, keep retrying. | ||
| log_level: Logging level. |
There was a problem hiding this comment.
mhm, maybe the attributes should just not be public 😄 @alwx @chkoss @degiz @federicotdn Any preferences on the style for constructors?
Proposed changes:
Status (please check what you already did):
black(please check Readme for instructions)