Native only #1
Native only #1
Conversation
|
Meta question - is |
|
@jordansissel that is a mistake, no idea how I put that in there, will back out that change. |
|
@jordansissel removed the dir. Thanks for catching that |
| when "transport", "node"; "9300-9305" | ||
| end | ||
| end | ||
| @port ||= "9300-9305" |
There was a problem hiding this comment.
now that we have no conditions on this default, we can move this to the top
config :port, :default => "9300-9305"
|
@talevy would either of you mind taking a look at this change set? I just backported the latest stuff around upserting. |
| @@ -11,146 +11,143 @@ | |||
| let(:max_retries) { 3 } | |||
|
|
|||
| def mock_actions_with_response(*resp) | |||
| LogStash::Outputs::Elasticsearch::Protocols::HTTPClient | |||
| LogStash::Outputs::Elasticsearch::Protocols::TransportClient | |||
There was a problem hiding this comment.
2 things.
- I don't fully understand why, but mocking both Node and Transport Clients caused problems to me in the past. Since Transport client is of type NodeClient, mocking the NodeClient sufficed.
- we might as well update to the newer convention in rspec for any_instance mocking like so:
allow_any_instance_of(LogStash::Outputs::Elasticsearch::Protocols::NodeClient).to receive(:bulk).and_return(*resp)
There was a problem hiding this comment.
good catch, fixed!
On Thu, Aug 20, 2015 at 2:36 PM, Tal Levy notifications@github.com wrote:
In spec/integration/outputs/retry_spec.rb
#1 (comment)
:@@ -11,146 +11,143 @@
let(:max_retries) { 3 }def mock_actions_with_response(*resp)
- LogStash::Outputs::Elasticsearch::Protocols::HTTPClient
- LogStash::Outputs::Elasticsearch::Protocols::TransportClient
2 things.
I don't fully understand why, but mocking both Node and Transport
Clients caused problems to me in the past. Since Transport client is of
type NodeClient, mocking the NodeClient sufficed.
2.we might as well update to the newer convention in rspec for
any_instance mocking like so:allow_any_instance_of(LogStash::Outputs::Elasticsearch::Protocols::NodeClient).to receive(:bulk).and_return(*resp)
—
Reply to this email directly or view it on GitHub
https://github.com/logstash-plugins/logstash-output-elasticsearch_native/pull/1/files#r37571763
.
|
This also pulls elastic/logstash#3735 (jar dependencies) via cherry pick |
|
This is currently broken pending mkristian/jar-dependencies#26 (figure out how to get the jar deps working) |
|
|
||
| # execute the request and get the response, if it fails, we'll get an exception. | ||
| request.get | ||
| raise "Could not index template!" unless response.isAcknowledged |
There was a problem hiding this comment.
I'm still investigating whether this is appropriate for error handling
|
Should there be all those .jar files checked into git? Seems like a recipe for fast git repo size growth. Thoughts? |
| # which uses HTTP instead. This output is, in-fact, sometimes slower, and never faster than that one. | ||
| # Additionally, upgrading your Elasticsearch cluster may require you to simultaneously update this | ||
| # plugin for any protocol level changes. The HTTP client has far fewer of these issues and is | ||
| # generally just easier to work with. |
There was a problem hiding this comment.
"is generally just easier"
Maybe rephrase "may be easier to work with due to wider familiarity with http" ?
|
Finished reviewing the code, but didn't review the specs yet. Left a few comments. I'll run the tests soon and report back. :) |
| # All protocols will use bulk requests when talking to Elasticsearch. | ||
| # | ||
| # The default `protocol` setting under java/jruby is "node". The default | ||
| # `protocol` on non-java rubies is "http" | ||
| config :protocol, :validate => [ "node", "transport", "http" ] | ||
| config :protocol, :validate => [ "node", "transport"], :default => "node" |
There was a problem hiding this comment.
Hmmm, maybe this should default to transport?
There was a problem hiding this comment.
@andrewvc I am leaning towards default to transport too. node requires setting network_host. Maybe this will really deter folks from using node
There was a problem hiding this comment.
+1 default to transport. We require (basically) hosts anyway and further that the main benefit of node for logstash (by intention, anyway) was multicast discovery which is disabled in 2.0
|
@andrewvc is |
|
@suyograo this must be an ES2.0 change, because it just won't join the cluster without that set. We could do some detection by attempting to get the hostname (which usually works for AWS), but it's probably best to just make it explicit. |
|
@andrewvc lets make it explicit for now. |
|
i tested this PR and it works for both transport and node |
| # The name/address of the host to bind to for Elasticsearch clustering. Equivalent to the Elasticsearch option 'network.host' | ||
| # option. | ||
| # This MUST be set for either protocol to work (node or transport)! The internal Elasticsearch node | ||
| # will bind to this ip. This ip MUST be reachable by all nodes in the Elasticsearch cluster |
There was a problem hiding this comment.
This ip MUST be reachable by all nodes in the Elasticsearch cluster
Only if protocol => node, right?
There was a problem hiding this comment.
Not in ES2.0! (surprisingly)
andrewvc
force-pushed
the
native_only
branch
from
f7adab2 to
603056c
Compare
|
LGTM :) |
Removal of HTTP stuff