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 some RabbitMQ managers #320

Merged

Conversation

salimaboubacar
Copy link
Contributor

Hello !

I ended up writing some custom managers for uses cases that were not covered :

  • Asyncio manager with rabbitMQ
  • Manager as a task queue instead of pubsub

Is it something that you would be intersted in integrating ? If yes, let me know and I can write tests + docstring for those new managers, otherwise I'll just use my fork instead

@miguelgrinberg
Copy link
Owner

I like adding the aio_pike manager, thanks for that!

With regards to the task queue classes, I honestly don't understand what use case you are trying to solve. If messages are queued, then that means that a process that was offline for a while is going to receive one or several stale messages and will try to execute them when the process starts. But when a process starts it does not have any clients connected, so what's the point?

@salimaboubacar
Copy link
Contributor Author

salimaboubacar commented Jul 20, 2019

With regards to the task queue classes, I honestly don't understand what use case you are trying to solve. If messages are queued, then that means that a process that was offline for a while is going to receive one or several stale messages and will try to execute them when the process starts. But when a process starts it does not have any clients connected, so what's the point?

It's because it's not one task queue per process, but per client, so: "if messages are queued, then that means that a process that was offline for a while is going to receive one or several stale messages and will try to execute them when the process starts" is true if you replace "process" by "client", which is exactly the point of this manager.

I like adding the aio_pike manager, thanks for that!

I'll start adding tests for it then, let me know if you want the task queue manager to be integrated with this lib, or if you think this should be handled outside of it, in this case I can open an other PR with only the pubsub aio_pika manager

@miguelgrinberg
Copy link
Owner

miguelgrinberg commented Jul 21, 2019

I still don't understand how queuing messages can be useful as a general purpose offering in this package. I agree that for certain types of applications it may be useful to queue messages, but in general applications implement a "catch-up" mechanism for this, queuing messages is not really a good way to do that. For example, in a chat application a client that disconnects may want to get all the messages that it missed as soon as it reconnects, but this is best done by getting them in a single event or request. I don't see how queuing lots of little messages can be useful. What if the user went offline for two weeks on vacation? There is nothing that prevents these messages from accumulating forever in the queue.

@salimaboubacar
Copy link
Contributor Author

I understand your concerns. For sure, this is not a general use case, it just happens that every drawback you mentioned does not apply to us :

  • Our users are always temporary, they rarely live more than a few minutes/hours => we can set a reasonable expiration time for each queue
  • Messages can't accumulate if the user is not active, so each queue can only have one or two pending messages at most.
  • An applicative catch-up mechanism means we will need to store messages and have them accessible from our socket.io server, which we do not want because our server is basically just a socket.io to HTTP/webhook converter with 0 business logic => Having a task queue in it is a cheap way to have both horizontal scalability and ability to send missed messages.

I understand that this may be too specific to be in this package. I'll remove them and keep the aio_pika pubsub manager. I can work on it next week :)

@salimaboubacar
Copy link
Contributor Author

salimaboubacar commented Jul 26, 2019

I would say this is ready for review. I'm not sure unit tests have a lot of added value for this class : this is a mix of kombu_manager and asyncio_redis_manager, which both don't have unit tests on their _publish and _listen methods, where all the logic is

@miguelgrinberg miguelgrinberg self-assigned this Jul 28, 2019
@miguelgrinberg miguelgrinberg merged commit d984f32 into miguelgrinberg:master Aug 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants