Skip to content

Add support for Redis unix sockets #57

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

brennentsmith
Copy link

Adds support for unix sockets on connecting to redis. Can result in a 50% increase in performance (https://redis.io/topics/benchmarks) and provides a touch of local ACL’s (permissions on the local file descriptor).

Added this PR to keep feature parity between redis-input and output. logstash-plugins/logstash-input-redis#64

@brennentsmith
Copy link
Author

Just signed the CLA. Rebuild

@jsvd
Copy link
Member

jsvd commented Jan 18, 2018

I believe this PR should follow the same strategy as the input, where either you select path or hosts. I don't see much of a reason to load balance between hosts and a unix socket. Wdyt?

@jsvd jsvd self-requested a review January 19, 2018 16:54
@brennentsmith
Copy link
Author

What if someone wants to run multiple redis instances on their local VM, and use different unix sockets to connect to them? (similar to the in documentation example of 127.0.0.1:6379 + 127.0.0.2:6379)

I agree that it's not exactly a common scenario, but I'd argue to leave it up to the person configuring logstash to select the options they want, rather than pull functionality.

@jstoja
Copy link

jstoja commented Feb 10, 2018

To be honest, I was about to totally disagree with this, but actually many other softwares (most common being databases and webservers) allow you listen on both. So I'd say why not.

The issue there is with this is that the unix socket is not an host, and therefore should in my opinion be used from the path parameter. Having both IPs and paths seem wrong in the same parameter seem wrong to me.

@process0
Copy link
Contributor

process0 commented Jun 25, 2019

Are there plans to merge this? Having a unix socket for output is reasonable expectation, seeing that the corresponding input plugin supports it.

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

Successfully merging this pull request may close these issues.

5 participants