Skip to content

pulse: Support routing by exchange/topic, fixes #28.#32

Merged
La0 merged 4 commits intomozilla:masterfrom
La0:fix-28
Dec 3, 2019
Merged

pulse: Support routing by exchange/topic, fixes #28.#32
La0 merged 4 commits intomozilla:masterfrom
La0:fix-28

Conversation

@La0
Copy link
Copy Markdown
Contributor

@La0 La0 commented Dec 3, 2019

No description provided.

@La0 La0 requested a review from marco-c December 3, 2019 10:57
@La0 La0 self-assigned this Dec 3, 2019
Comment thread libmozevent/pulse.py Outdated
self.got_message,
self.virtualhost,
)
listeners = [
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think it'd be better to have a single pulse connection, to reduce resource usage.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So a single callback that find the good route from the message routing information ?
It would be more stable pulse-wise, but may lead to bugs if we have a message that matches multiple routes

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

https://www.cloudamqp.com/blog/2018-01-19-part4-rabbitmq-13-common-errors.html suggests having one connection per process and one channel per thread.
Perhaps we could have one connection only, and one channel for each listener.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Or you can use the exchange_name info from the envelope.

Or yet another option could be to use a consumer_tag in basic_consume (which will also be available in the envelope), so each consumer can be tagged.

Comment thread tests/test_pulse.py
Comment thread libmozevent/pulse.py
pulse = None
await asyncio.sleep(5)

def find_matching_queues(self, payload_exchange: str, payload_routes: List[bytes]):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It'd probably be better to use another of the solutions (especially the multiple channels one), but we can do it in a follow-up. Can you file it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@La0 La0 merged commit d3876b3 into mozilla:master Dec 3, 2019
@La0 La0 deleted the fix-28 branch December 3, 2019 12:09
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