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

[RFC] fix: poller-service locking does not work #8450

Closed
wants to merge 1 commit into from

Conversation

TheMysteriousX
Copy link
Contributor

Needs another look once I'm a little more awake, but this could use some testing and feedback.

I have no idea what's up with the original locking, other than it doesn't work. I found #5619, which didn't include a solution either, so I've ripped out the database locking, and replaced it with thread level locking.

It's not a fantastic fix due to the overall structure of the poller service, but it works for me.

After applying this, CPU load has dropped on my less capable switches from between 60-80%, down to ~10%

screenshot 2018-03-22 00 41 56

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

@scrutinizer-notifier
Copy link

The inspection completed: No new issues

@laf
Copy link
Member

laf commented Mar 22, 2018

I'm assuming this makes it non-distributed aware if the locking is done local.

If so, I doubt we can merge this in because it would break distributed poller setups.

@TheMysteriousX
Copy link
Contributor Author

I'll admit I didn't consider distributed polling; I could hide the alternative locking behaviour behind a toggle.

@laf
Copy link
Member

laf commented Mar 22, 2018

That could be an option.

@murrant had a PR in that replaced this script but we never merged it (I can't find the PR right now). Overall it replaced cron for everything so it would be better to focus on that if you fancy helping. It had an issue with auto updating but aside from that, I think it worked extremely well.

@murrant
Copy link
Member

murrant commented Mar 22, 2018

I sent it as a PR here #8455

Still has the file handle leak as I haven't touched it.

@murrant
Copy link
Member

murrant commented Mar 22, 2018

@TheMysteriousX Original locking worked with older versions of MySQL. But the server behavior changed at some point.

@TheMysteriousX
Copy link
Contributor Author

(signed in on the right account this time)

@murrant 's is the much better option - I didn't realise a rewrite was already in progress.

Re: the file descriptor leak, it's something I've seen before - if it's what I think it is, the cause FD's being inherited across process executions, even when calling exec*().

There's a bug in the python tracker on it: https://bugs.python.org/issue12107

Solution seems to be use Python3.4+, or don't rely on file descriptors auto closing. I suspect though it'll be in the MySQL or redis drivers, which makes fixing it impossible without patching upstream).

@murrant
Copy link
Member

murrant commented Mar 22, 2018

@TheMysteriousX yeah, it has to do with the redis driver

@murrant
Copy link
Member

murrant commented Mar 29, 2018

Closing this, continued at #8455

@murrant murrant closed this Mar 29, 2018
@lock lock bot locked as resolved and limited conversation to collaborators May 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants