Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions docs/index.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ This plugin supports the following configuration options plus the <<plugins-{typ
| <<plugins-{type}s-{plugin}-data_type>> |<<string,string>>, one of `["list", "channel", "pattern_channel"]`|Yes
| <<plugins-{type}s-{plugin}-db>> |<<number,number>>|No
| <<plugins-{type}s-{plugin}-host>> |<<string,string>>|No
| <<plugins-{type}s-{plugin}-path>> |<<string,string>>|No
| <<plugins-{type}s-{plugin}-key>> |<<string,string>>|Yes
| <<plugins-{type}s-{plugin}-password>> |<<password,password>>|No
| <<plugins-{type}s-{plugin}-port>> |<<number,number>>|No
Expand Down Expand Up @@ -89,9 +90,19 @@ The Redis database number.

* Value type is <<string,string>>
* Default value is `"127.0.0.1"`
* Path and Host are mutually exclusive - Logstash will throw an error if both are specified.

The hostname of your Redis server.

id="plugins-{type}s-{plugin}-path"]
===== `path`

* Value type is <<string,string>>
* There is no default value for this setting.
* Path and Host are mutually exclusive - Logstash will throw an error if both are specified.

The unix socket path of your Redis server.

[id="plugins-{type}s-{plugin}-key"]
===== `key`

Expand Down
38 changes: 32 additions & 6 deletions lib/logstash/inputs/redis.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,19 @@ module LogStash module Inputs class Redis < LogStash::Inputs::Threadable

default :codec, "json"

# The hostname of your Redis server.
config :host, :validate => :string, :default => "127.0.0.1"
# 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.


# The port to connect on.
config :port, :validate => :number, :default => 6379

# The unix socket path to connect on. Will override host and port if defined.
# There is no unix socket path by default.
# This and :host are mutually exclusive. Will raise an ArgumentError if both are specified.
config :path, :validate => :string

# The Redis database number.
config :db, :validate => :number, :default => 0

Expand Down Expand Up @@ -68,7 +75,15 @@ def new_redis_instance
end

def register
@redis_url = "redis://#{@password}@#{@host}:#{@port}/#{@db}"
if [email protected]? && [email protected]?
raise ArgumentError.new(
"Host and Path cannot be specified simultaneously. These options are exclusive."
)
elsif @host.nil? && @path.nil?
@host = "127.0.0.1"
end

@redis_url = @path.nil? ? "redis://#{@password}@#{@host}:#{@port}/#{@db}" : "#{@password}@#{@path}/#{@db}"

@redis_builder ||= method(:internal_redis_builder)

Expand Down Expand Up @@ -114,13 +129,24 @@ def is_list_type?

# private
def redis_params
{
:host => @host,
:port => @port,
if @path.nil?
connectionParams = {
:host => @host,
:port => @port
}
else
connectionParams = {
:path => @path
}
end

baseParams = {
:timeout => @timeout,
:db => @db,
:password => @password.nil? ? nil : @password.value
}

return connectionParams.merge(baseParams)
end

# private
Expand Down