-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
rest_cherrypy: remove sleep call #41235
Conversation
@moio, thanks for your PR! By analyzing the history of the files in this pull request, we identified @pass-by-value, @whiteinge and @rallytime to be potential reviewers. |
PS. to dear colleagues @meaksh, @dincamihai, @pkazmierczak and @isbm - if you see any evident reason I can't see why this is a super bad idea your input is more than welcome! |
@@ -2326,7 +2326,6 @@ def signal_handler(signal, frame): | |||
logger.error( | |||
"Error: Salt event has non UTF-8 data:\n{0}" | |||
.format(data)) | |||
time.sleep(0.1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is likely to suppress high CPU load on idle. I'd leave sleep
here, but set to 0.01
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CPU is flat at 0 when idle, so this does not seem to be the problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pass-by-value we are wondering why you would add that sleep
here. 😉 Any insights?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To my first comment, I think it was needed pre-Tornado, which does now the same thing here. OK, I think that it is safe to remove after all.
@moio please remove |
ad788e0
to
21b4162
Compare
Argh! Right @isbm, thanks for pointing that out. Fixed. |
Go Go Jenkins! |
I'd like to have @whiteinge come by here to have a look at this before we get this in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know the history here but if I were to guess, I'd concur with @isbm 's assessment that this was for a non-Tornado concurrency lib.
I seem to recall there were a couple teams members looking into Gevent back when this was added which monkey-patches 👍 from me. (Thanks for the ping @cachedout.) |
@cachedout thanks for merging! Should I open PRs to backport this to other versions, or will someone from the team simply cherry-pick the commit? Is there any need for unit tests? |
@moio I have marked this for backporting. As always, we would gladly accept unit tests for this. :] |
@cachedout: I am willing to invest some time in a unit testing that "covers" this patch, I can't help wondering if it is worth it. The only implementation idea that came to me is to mock several objects, call If this is the "correct way", does such a test bring any value? IOW: do you believe it is worth it to have such mock-heavy tests, that know so much about implementation details, and rely so much about them? Or, do you see any other way a test could be implemented to capture better the nature of this patch? From my perspective an integration test would make more sense here, and specifically a performance regression test. Side note: we have a plan to introduce general ones for critical Salt areas based on evil-minions, but that will take some time before it becomes reality. Thanks! |
@moio Yes, I think that's right. In truth, a regression here would likely be detected by the existing integration tests so I'm really not even sure what additional integration tests might be added that would make sense on this one. |
Back-port #41235 to 2016.11
What does this PR do?
It removes a delay when processing events from the
rest_cherrypy
netapi that limited the throughput to 10 events per second maximum.What issues does this PR fix or reference?
None known.
Previous Behavior
If more than 10 events per second were published on the Event bus,
netapi
clients would only get up to 10 per second, and the rest would be queued.This can be easily reproduced with the evil-minions Salt load generator.
New Behavior
Events are processed as fast as possible.
Tests written?
No.
This PR is more of a question rather than a code contribution (to @pass-by-value, @whiteinge or anyone with in-depth knowledge of this matter). I am not experienced enough to see why this might be needed or what would an alternative solution look like at the moment.
I did not observe negative side effects with this patch.
Thanks in advance!