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

Warn when user has declared the same queue on multiple procs #50

Closed jon-torodash closed 5 years ago

jon-torodash commented 5 years ago

Though it might be perfectly allowable, reusing the same queue cannot be relied upon to give accurate job counts given certain backends.

ryanhiebert commented 5 years ago

I definitely understand why it wouldn't be desirable, but why would it be inaccurate? What does RQ do that would make the counts inaccurate? I would expect that getting the count of the queue would be an operation that would have no side-effects, because it would be read-only. Is that not the case?

jon-torodash commented 5 years ago

Given my change, then yes, RQ should then be accurate (originally it wasn't), so I should amend my description. I don't have experience with Celery, RabbitMQ, so I can't comment to those, but I figured it would be generally useful to have this warning, but if you think otherwise I'll close the PR.

ryanhiebert commented 5 years ago

Gotcha. My memory was bad, I had to go look through the list of PRs to see that this came out of your first PR, that was split out.

I think that regardless of what we choose here, inaccurate counts should be considered a bug.

Given that this warning will be output on every process that loads up the procs, I think that we will either want to make this a warning of a future breaking change that will make it an exception, or we should forgo the warning entirely. If we consider this a valid use-case, then we should not warn. If we do not consider it a valid use-case, then we should intend to raise an error in a future version, once a reasonable deprecation period has been given.

For the sake of not breaking things that already exist, I've been inclined to avoid making that breaking change, or moving toward it. But I also have not been able to think of a case where someone would reasonably desire the behavior as it currently is. I didn't write the initial version of this, so this decision was made before I came to the project.

If you agree that there seems to be no legitimate use-case for this feature, and if noone else chimes in to disagree with us, then I'd be inclined to make this warning a DeprecationWarning, with the intent that sometime in the future we might choose to make a breaking change that would make it raise an exception instead.

jon-torodash commented 5 years ago

Right - I never expected to find that the implementation of the count before I came to the project was prone to double counting; that only dawned on me as I was investigating why performance was slow. The more I think about it, especially given the fixtures I'm declaring in the test, it's really a very clear and conscious action (aside from copy-paste errors) on the part of the Procfile setup to declare the same queue twice. I agree with the DeprecationWarning, though I suspect sooner someone would come up with a legitimate use case for wanting that capability than a maintainer removing the ability. I'll look into why the CI build failed in a bit.

ryanhiebert commented 5 years ago

@jon-torodash : Do you know why the tests are failing? We've got failing tests on #51 as well, and I'm wanting to get that merged. I think that you added those tests. If I can't get it figured out, I may revert the tests until we get them working.

ryanhiebert commented 5 years ago

Sorry, for some reason I thought you had written the tests, but it was @codingjoe . I'll see if I can get them working.

ryanhiebert commented 5 years ago

Between these two statements, I'm unclear on the direction you'd like to go with this.

it's really a very clear and conscious action (aside from copy-paste errors) on the part of the Procfile setup to declare the same queue twice.

I agree with the DeprecationWarning

If the behavior as-is is preferable, then we'll not do a warning at all. If it is not a good default, then we should raise a deprecation warning.

With my reworking of the tests, this needs a rebase. Feel free to open a new PR with the rebased code if you think it's a good idea. As ever, I'm still inclined personally to not bother, but if you think it's worthwhile I'm willing to hear your case.