Add some RabbitMQ managers by salimaboubacar · Pull Request #320 · miguelgrinberg/python-socketio · GitHub
Skip to content

Add some RabbitMQ managers#320

Merged
miguelgrinberg merged 4 commits into
miguelgrinberg:masterfrom
salimaboubacar:asyncio_rabbitmq_managers
Aug 4, 2019
Merged

Add some RabbitMQ managers#320
miguelgrinberg merged 4 commits into
miguelgrinberg:masterfrom
salimaboubacar:asyncio_rabbitmq_managers

Conversation

@salimaboubacar

Copy link
Copy Markdown
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

@salimaboubacar salimaboubacar force-pushed the asyncio_rabbitmq_managers branch from fe283f2 to 969aa77 Compare July 19, 2019 16:08
@miguelgrinberg

Copy link
Copy Markdown
Owner

@salimaboubacar

salimaboubacar commented Jul 20, 2019

Copy link
Copy Markdown
Contributor Author

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

miguelgrinberg commented Jul 21, 2019

Copy link
Copy Markdown
Owner

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
Copy Markdown
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

salimaboubacar commented Jul 26, 2019

Copy link
Copy Markdown
Contributor Author

@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.

2 participants