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

adjust queue depth warnings to emit when all threads are busy #243

Merged
merged 7 commits into from Apr 3, 2019

Conversation

Projects
None yet
3 participants
@mmerickel
Copy link
Member

commented Apr 3, 2019

No description provided.

@mmerickel mmerickel force-pushed the queue-depth-warnings branch 2 times, most recently from c831dc9 to 5670065 Apr 3, 2019

@mmerickel mmerickel force-pushed the queue-depth-warnings branch 2 times, most recently from 6810093 to e02fe79 Apr 3, 2019

bertjwregeer and others added some commits Apr 3, 2019

Instead of iterating over dictionary, use integer
This avoids looping over the dictionary in `add_task` for the extra
complexity of keeping a simple counter of which threads are active or
not activate.
Merge pull request #244 from Pylons/queue-depth-warnings-int
Queue depth warnings (using integer inc/dec for active)

@bertjwregeer bertjwregeer merged commit d060d24 into master Apr 3, 2019

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@bertjwregeer bertjwregeer deleted the queue-depth-warnings branch Apr 3, 2019

@j4mie

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

Hi @bertjwregeer and @mmerickel! As the person who contributed the original code for the queue depth warning, would you mind a quick explanation of what issue(s) this PR actually fixes? I don't really understand the diff I'm afraid. Just for my own interest really! Thanks :)

@mmerickel

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

The main issue is that it would warn if you submitted several tasks in a row (like a web browser does) and you have available threads to process them, they just haven't switched over to grab them yet. This was causing a lot of false positive warnings. So this change starts tracking the number of idle threads and only warns if all threads are busy. I only just realized while typing this that there is one uncovered case right now where it's possible to submit more tasks than can be handled but no threads are busy and so no warning will be emitted. We should check the queue size and compare it to the number of idle threads.

@j4mie

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

Ah cool, that makes sense. We have actually seen behaviour a bit like that, so hopefully this change will help. Thanks for the explanation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.