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.

Sign up for GitHub

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

Jump to bottom

Use epoll when available to support fds > 1023 #448

Merged
merged 1 commit into from
Jun 4, 2017
Merged

Use epoll when available to support fds > 1023 #448

merged 1 commit into from
Jun 4, 2017

Conversation

packysauce
Copy link
Contributor

Here's my take on the fd > 1023 issue. I'm open to suggestions for the unit test.

Major points:
When epoll is available, and the highest fd in use is > 1023, route through epoll.
Otherwise, use the existing select() behavior so by and large nothing changes.

closes #266
closes #171

houndci-bot
houndci-bot reviewed Jun 2, 2017
View reviewed changes
kazoo/tests/test_threading_handler.py Outdated
h._epoll_select(socks, [], [])
h.stop()


Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blank line contains whitespace

kazoo/tests/test_threading_handler.py Outdated
h._select(socks, [], [])
h._epoll_select(socks, [], [])
h.stop()

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blank line contains whitespace

kazoo/tests/test_threading_handler.py Outdated
@@ -45,7 +46,33 @@ def test_double_start_stop(self):
h.stop()
h.stop()
self.assertFalse(h._running)


Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blank line contains whitespace

kazoo/tests/test_threading_handler.py Outdated
@@ -1,3 +1,4 @@
import tempfile
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'tempfile' imported but unused

kazoo/handlers/threading.py Outdated
@@ -166,6 +201,51 @@ def select(self, *args, **kwargs):
raise
# if we hit our timeout, lets return as a timeout
return ([], [], [])

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blank line contains whitespace

kazoo/handlers/threading.py Outdated
@@ -33,6 +37,26 @@

log = logging.getLogger(__name__)

_HAS_EPOLL = hasattr(select, "epoll")

def _to_fileno(obj):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expected 2 blank lines, found 1

@bbangert
Copy link
Member

bbangert commented Jun 2, 2017

How does this change coverage? I'll be getting automatic coverage checking up next to make it a bit easier to track.

Otherwise I think this looks good, the commits need to be squashed and updated per the CONTRIBUTING doc style guide. Thanks!

@packysauce packysauce force-pushed the epoll branch 2 times, most recently from bad82a4 to 321d7c5 Compare June 4, 2017 19:51
@packysauce
Copy link
Contributor Author

Alright I think I finally figured out how to get the commits looking the way we want them.

@bbangert
Copy link
Member

bbangert commented Jun 4, 2017

The easiest way I've found is to use git reset COMMITBEFORENEWONE, stash the changes, rebase to master, then stash pop to re-apply the changes, and make the single commit before force-pushing to the branch. There might be an easier way for more powerful git wizards, but that's how I usually end up doing it.

@packysauce
Copy link
Contributor Author

ended up resetting to commit^, then rebase -i and squash, then force push. Finally down to one commit though. Is github's automatic squash-merging not available for automatic merges?

@bbangert
Copy link
Member

bbangert commented Jun 4, 2017 •
edited
Loading

It doesn't seem apply quite right for some reason. For the git commit message, it should look like this:

feat(core): use epoll when available to support fds > 1023

When epoll is available, and the highest fd in use is > 1023, route through epoll.
Otherwise, use the existing select() behavior so by and large nothing changes.

Closes #266, #171

Easiest way to change the text is git commit --amend, update it, then force push. Sorry bout the hassle, having a uniform commit style makes it easier to track changes as the automated tool generates helpful changelog entries with linkage, ie, https://github.com/python-zk/kazoo/releases/tag/2.3.1

@packysauce
feat(core): use epoll when available to support fds > 1023
267e61b 
When epoll is available, and the highest fd in use is > 1023, route through epoll.
Otherwise, use the existing select() behavior so by and large nothing changes.

Closes python-zk#266, python-zk#171
bbangert
bbangert approved these changes Jun 4, 2017
View reviewed changes
Copy link
Member

@bbangert bbangert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@packysauce
Copy link
Contributor Author

No, it's my bad. I failed to properly grok the CONTRIBUTING.md guidelines. I'm totally ok with the "hassle" to keep things tidy :)

@bbangert bbangert merged commit 1960796 into python-zk:master Jun 4, 2017
@packysauce packysauce deleted the epoll branch June 4, 2017 21:25
@bbangert
Copy link
Member

bbangert commented Jun 4, 2017

Thanks for the PR!

@mattmb mattmb mentioned this pull request Jan 10, 2018
Merged
@zhang-jc zhang-jc mentioned this pull request Sep 30, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@bbangert bbangert bbangert approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@packysauce @bbangert @houndci-bot