search

ryanhiebert / hirefire

A Python lib to integrate with the HireFire service -- The Heroku Proccess Manager.
https://hirefire.readthedocs.io/
Other
34 stars 21 forks source link

Routing keys with wildcards are not matched #19

Closed deppy closed 5 years ago

deppy commented 8 years ago

While using version 0.4 we are seeing a bunch of key errors when the routing key has a wildcard.

We have an exchange + routing key ['internal', 'users.user.*'] which works just fine, but the hirefire package throws a KeyError because it's not an exact match.

KeyError: ('internal', 'users.user.created')

ryanhiebert commented 8 years ago

It sounds like you're using Celery. Is that correct?

deppy commented 8 years ago

Yes. Django + Celery

deppy commented 8 years ago

Can also provide a bit more of the errors we were seeing:

TypeError hirefire.procs.celery in identify_queue get_status_task_counts() missing 1 required positional argument: 'status KeyError: ('internal', 'users.user.created') hirefire/utils.py in missing at line 114

deppy commented 8 years ago

Had a bit more time to actually debug this. It looks like it was the changes introduced in hirefire 0.4 . I went back to version 0.3 for now and everything is working fine. Indeed the status isn't always sent 100% of the time. If I get more of a chance, I'll look into fixing it and submitting a PR (unless someone beats me to it).

ryanhiebert commented 7 years ago

Interestingly, while attempting to upgrade to Celery 4, I had this same error happen to me. For whatever reason (I don't understand yet), the queue is registered as having (exchange, routing_key) of ('queuename', 'queuename'), but the tasks themselves are identified with ('queuename', ''). I don't understand why the routing_key is empty.

@deppy : If you've got any insight on my issue, that would be great. For the issue you mention, on version 0.4 you can add tasks_statuses = [] to the class.

I was wondering how many people even use routing_keys that are different from their queue name. Clearly there is at least one (you), so I'll need to make sure that whatever solution I come up with, it doesn't break your implementation, assuming that you have indeed upgraded to 0.4.

I'm debating adding a flag to let us just assume that the exchange, routing_key, and queue_name are all the same. It would sidestep the issue that I'm having currently, and probably be correct for the general case, though it would, if used, break the use-case that from the OP. It seems like, if it fixes Celery 4 inspecting, that it may be a reasonable default even.

@jezdez , @mjtamlyn , and anyone else: have any thoughts on the right approach?

ryanhiebert commented 5 years ago

I'm still happy for this to be implemented if somebody ever cares about it enough to write it. It's not something that's important to my uses, though, so I'm going to close it until there's somebody that would like to take it on.