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

Added support for connecting via unix socket #189

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vobinics
Copy link

@vobinics vobinics commented Jun 5, 2020

These changes do not violate the old code, but complement it
Unix socket connection is 30% faster than TCP
Now, if there is a file along the path /tmp/memcached.sock, then a unix connection is selected
I think this change will help other developers who need a Unix connection.

@aio-libs aio-libs deleted a comment from CLAassistant Jan 14, 2022
@Dreamsorcerer
Copy link
Member

Could you merge master and also add information to the documentation about this option?

@@ -9,7 +10,7 @@

class MemcachePool:

def __init__(self, host, port, *, minsize, maxsize, loop=None):
def __init__(self, host, port, *, minsize, maxsize, loop=None, unix=None):
Copy link
Member

Choose a reason for hiding this comment

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

I think this should accept host/port or unix. Currently a unix connection requires the host/port even though it is unused.

Copy link
Member

Choose a reason for hiding this comment

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

Or, we could try unix first, and then fallback to host/port if the connection fails. In which case, we should support host/port as None to disable this fallback behaviour and always get an error if the unix connection fails.

@Dreamsorcerer
Copy link
Member

We also need a new test.

So, if you'd like to finish off this PR and get it merged, we need:

  • Merge master.
  • Add a test.
  • Update documentation.
  • Allow using unix without host/port.

Sorry for the delay in reviewing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants