Skip to content

Add support for Redis unix sockets #64

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

Merged
merged 4 commits into from
Jan 19, 2018

Conversation

brennentsmith
Copy link
Contributor

@brennentsmith brennentsmith commented Jan 8, 2018

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).

We use Redis heavily at Speedtest.net as a local pre-buffer for logstash, and using unix sockets helps reduce the local overhead.

Corresponding logstash-output-redis PR: logstash-plugins/logstash-output-redis#57

@brennentsmith
Copy link
Contributor Author

Signed CLA. Rebuild

@jordansissel
Copy link
Contributor

Change looks pretty straight-forward.

I'd like to see mutual exclusion on path vs host and port -- if a user specifies both path and host, Logstash should fail.

@jordansissel
Copy link
Contributor

I'll test this (and your other PR on redis output) tomorrow. Thank you for your work :)

@brennentsmith
Copy link
Contributor Author

👍 Sounds good on the mutual exclusion - PR updated.

@brennentsmith
Copy link
Contributor Author

Rebuild. Travis seemed to have a networking issue.

@jordansissel
Copy link
Contributor

I restarted the failed redis job. (3/4 passed, 1 failed due to network issues)

@jstoja
Copy link

jstoja commented Jan 12, 2018

This is probably mergeable! :)

Copy link
Member

@jsvd jsvd left a comment

Choose a reason for hiding this comment

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

to reduce a minor breaking change subtlety in treating the default parameters I think we should keep the default value for host in the setting itself but make :path override it if set (and log a warn).

# The hostname of your Redis server
# This defaults to 127.0.0.1 if not specified.
# This and :path are mutually exclusive. Will raise an ArgumentError if both are specified.
config :host, :validate => :string
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 we can keep this default here, and simple have the :path option overwrite host/port like described in line 34

Copy link
Member

Choose a reason for hiding this comment

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

If :path is set we can just log a warn saying that path is set, so we're ignoring host/port

Copy link
Contributor Author

@brennentsmith brennentsmith Jan 17, 2018

Choose a reason for hiding this comment

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

Thanks @jsvd for the feedback. Ironically, that was actually the original logic of how this PR was constructed (see commit: 7c29668) Looks like I forgot to remove the comment on L34 in the subsequent commits 😆 .

@jordansissel - thoughts? I can see merits for both logic paths:

  1. Mutually exclusive
  • No ambiguity about which connection is being used
  • Defaults are no longer set at init, which does make the code more complex
  1. Override
  • Could potentially lead to confusion (especially if someone doesn't know what unix sockets are). Documentation can help assay this issue, but it's not as explicit as a mutex.
  • Defaults are set at init, making the code cleaner.

Frankly, I'm fine with either logic path - just let me know which pattern matches Logstash plugins best and I'll update the PR.

@jsvd
Copy link
Member

jsvd commented Jan 15, 2018

@brennentsmith thank you for the contribution, this is great!

@brennentsmith
Copy link
Contributor Author

@jsvd - logic updated as requested.

Copy link
Member

@jsvd jsvd left a comment

Choose a reason for hiding this comment

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

LGTM

@jsvd jsvd merged commit 59c8d61 into logstash-plugins:master Jan 19, 2018
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.

4 participants