Added support for routing parameters. #65

cdahlqvist · 2015-02-27T21:40:18Z

I have added support for routing when indexing documents.

jordansissel · 2015-02-27T22:33:07Z

Code looks good, I think. I haven't tested it.

cdahlqvist · 2015-02-27T23:34:17Z

I copied the parameter description from PR #64 to my code as it is better and more descriptive.

masteinhauser · 2015-03-02T16:01:11Z

Ah, I didn't realize there was more work which was needed. Though, I definitely missed adding a test! I'll close out #64 in support of this.

masteinhauser · 2015-03-03T21:23:05Z

Is there anything else needed for this PR to be reviewed, tested and merged?

It looks like the tests failed due to a CLA issue but it also seems like the QA process is not updated for the new plugins repo organization.

jordansissel · 2015-03-03T21:24:38Z

@masteinhauser time. We'll tend to this soon. We are focusing on other priorities at this time.

masteinhauser · 2015-03-03T21:25:35Z

Ah, understood. I will package and keep my own internally. Thanks!

purbon · 2015-03-13T18:12:44Z

spec/outputs/elasticsearch_spec.rb

@@ -73,6 +73,75 @@
    end
  end

+  describe "ship lots of events w/ default index_type and fixed routing key", :elasticsearch => true do
+    # Generate a random index name
+    index = 10.times.collect { rand(10).to_s }.join("")


This should go into a let variable (more info at https://relishapp.com/rspec/rspec-core/v/3-2/docs/helper-methods/let-and-let )

…ults.

jordansissel · 2015-03-16T21:51:27Z

@talevy can you take over review on this? @purbon is going on vacation (WOO) and I'd like to help move this forward for our friend @cdahlqvist :)

cdahlqvist · 2015-03-20T18:07:26Z

@talevy Added test for dynamic routing parameter as discussed.

talevy · 2015-03-20T18:23:51Z

@cdahlqvist thanks!

I'll try to check it out and test locally soon and get back to you!

talevy · 2015-03-24T22:07:40Z

@cdahlqvist why did you change all the insist to expect. can we leave that alone for now? Plus, I cannot run tests as is with that

talevy · 2015-03-24T22:15:57Z

@cdahlqvist I also see that there are a few result['count'] retrievals for verifying number of responses. This field is only found in _count requests and not _search. to verify count of a search response you may want to look at result['hits']['hits'].size.

talevy · 2015-03-24T22:20:53Z

spec/outputs/elasticsearch_spec.rb

@@ -56,7 +56,7 @@
        response.read_body { |chunk| data << chunk }
        result = LogStash::Json.load(data)
        count = result["count"]
-        insist { count } == event_count
+        expect { count } == event_count


we have traditionally been using insist for such assertions. I think we should keep it with insist for the sake of keeping this PR's main goal to add the _routing functionality

talevy · 2015-03-24T22:29:12Z

spec/outputs/elasticsearch_spec.rb

+      data = ""
+      response.read_body { |chunk| data << chunk }
+      result = LogStash::Json.load(data)
+      count = result["count"]


result["count"] should be result['hits']['hits'].size

talevy · 2015-03-24T22:46:06Z

I also notice that many of the tests for routing simply check document counts from request with routing parameters. you may simply want to call the count api, and check count, instead of calling search

cdahlqvist · 2015-03-25T10:50:17Z

That is a very good point. Will modify to instead use the count API and correct the syntax.

cdahlqvist · 2015-03-26T07:22:26Z

@talevy Have cleaned up the routing tests and made them instead use the _count API. Did however not change back to using 'insist' as @purbon preferred the use of 'expect'.

talevy · 2015-03-26T19:30:26Z

we can go ahead and change insist to expect in a different PR, if you do not mind. Plus, I cannot seem to be able to run the tests with expect

cdahlqvist · 2015-03-26T20:42:45Z

@talevy Changed back to using expect. Hopefully it will work properly now.

cdahlqvist · 2015-03-30T04:26:06Z

@talevy Have merged changes from master.

talevy · 2015-03-30T23:36:11Z

@cdahlqvist looks good! but, I am having trouble having the whole test-suite pass... your tests pass. other tests pass. but when I run it all together, things fail. I am trying to debug it. once that is resolved, I will merge this in!

talevy · 2015-04-03T20:10:57Z

so master is ready. but I cant seem to get your tests to pass. they conflict and cause this:

Apr 03, 2015 1:04:35 PM org.apache.http.impl.execchain.RetryExec execute
INFO: I/O exception (java.net.SocketException) caught when processing request to {}->http://127.0.0.1:9200: Bad file descriptor

talevy · 2015-04-03T20:36:42Z

https://github.com/cdahlqvist/logstash-output-elasticsearch/blob/master/spec/outputs/elasticsearch_spec.rb#L701-L704

in the lines above. the mocked actions need to be updated to include the :_routing param.

for example:

let(:action1) { ["index", {:_id=>nil, :_index=>"logstash-2014.11.17", :_type=>"logs"}, event1] }

should be updated to:

let(:action1) { ["index", {:_id=>nil, :_routing=>nil, :_index=>"logstash-2014.11.17", :_type=>"logs"}, event1] }

same goes for :action2

iemem15 · 2015-04-05T22:37:17Z

Hi, When is going to be available to use it?

cdahlqvist · 2015-04-06T07:38:20Z

@talevy Have added the routing parameter.

talevy · 2015-04-06T17:39:44Z

cool! tests are looking good @cdahlqvist!

can you go ahead and update your master from remote and git rebase master so we can merge this in?

cdahlqvist · 2015-04-07T10:04:40Z

@talevy Done.

talevy · 2015-04-07T18:16:08Z

Thank you!

LGTM

Added support for routing parameters.

crickeys · 2015-04-15T23:34:27Z

Does this currently work for UPDATES in the downloadable or YUM versions of logstash? I'm using logstash-1.4.2-1_2c0f5a1.noarch

I'm having issues with updating parent/child documents. I can't seem to add any routing information when doing an UPDATE action. I keep getting :message=>"Unknown setting 'routing' for elasticsearch", :level=>:error}

I've also tried logstash-1.5.0.rc2-1.noarch and same message. I'm new to logstash/elasticsearch so forgive me if this is obvious.

talevy · 2015-04-15T23:42:35Z

@crickeys we are still focusing on cleaning up a few pipeline details for another RC3 release in the next week or 2. so it will be in there. For now, this feature is only compatible with development master of Logstash

Christian Dahlqvist added 2 commits February 27, 2015 21:03

Added support for routing parameters.

35245f8

Updated routing test description.

d93cce4

Updated parameter description.

1952908

masteinhauser mentioned this pull request Mar 2, 2015

Support routing option for documents #64

Closed

wiibaa mentioned this pull request Mar 13, 2015

Add Routing-Paramter to Elasticsearch-Output elastic/logstash#2814

Closed

purbon reviewed Mar 13, 2015
View reviewed changes

Christian Dahlqvist added 2 commits March 14, 2015 09:37

Added routing tests for different protocols. Corrected parameter defa…

cecd8cf

…ults.

Replaced use if insist with expect.

451094d

jordansissel assigned talevy Mar 16, 2015

Added test for dynamic routing parameter

464ab14

talevy reviewed Mar 24, 2015
View reviewed changes

Cleaned up and corrected the routing tests.

8c5d2f4

Corrected tests and changed back to using expect.

5739a32

Merge remote-tracking branch 'upstream/master'

b642a7c

Added routing parameter to actions

0d86892

Merge remote-tracking branch 'upstream/master'

60c8c8f

talevy added a commit that referenced this pull request Apr 7, 2015

Merge pull request #65 from cdahlqvist/master

63ff11d

Added support for routing parameters.

talevy merged commit 63ff11d into logstash-plugins:master Apr 7, 2015

morganchristiansson mentioned this pull request Aug 25, 2016

Large logs filling up disks elastic/logstash#5831

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for routing parameters. #65

Added support for routing parameters. #65

cdahlqvist commented Feb 27, 2015

jordansissel commented Feb 27, 2015

cdahlqvist commented Feb 27, 2015

masteinhauser commented Mar 2, 2015

masteinhauser commented Mar 3, 2015

jordansissel commented Mar 3, 2015

masteinhauser commented Mar 3, 2015

purbon Mar 13, 2015

jordansissel commented Mar 16, 2015

cdahlqvist commented Mar 20, 2015

talevy commented Mar 20, 2015

talevy commented Mar 24, 2015

talevy commented Mar 24, 2015

talevy Mar 24, 2015

talevy Mar 24, 2015

talevy commented Mar 24, 2015

cdahlqvist commented Mar 25, 2015

cdahlqvist commented Mar 26, 2015

talevy commented Mar 26, 2015

cdahlqvist commented Mar 26, 2015

cdahlqvist commented Mar 30, 2015

talevy commented Mar 30, 2015

talevy commented Apr 3, 2015

talevy commented Apr 3, 2015

iemem15 commented Apr 5, 2015

cdahlqvist commented Apr 6, 2015

talevy commented Apr 6, 2015

cdahlqvist commented Apr 7, 2015

talevy commented Apr 7, 2015

crickeys commented Apr 15, 2015

talevy commented Apr 15, 2015

Added support for routing parameters. #65

Added support for routing parameters. #65

Conversation

cdahlqvist commented Feb 27, 2015

jordansissel commented Feb 27, 2015

cdahlqvist commented Feb 27, 2015

masteinhauser commented Mar 2, 2015

masteinhauser commented Mar 3, 2015

jordansissel commented Mar 3, 2015

masteinhauser commented Mar 3, 2015

purbon Mar 13, 2015

Choose a reason for hiding this comment

jordansissel commented Mar 16, 2015

cdahlqvist commented Mar 20, 2015

talevy commented Mar 20, 2015

talevy commented Mar 24, 2015

talevy commented Mar 24, 2015

talevy Mar 24, 2015

Choose a reason for hiding this comment

talevy Mar 24, 2015

Choose a reason for hiding this comment

talevy commented Mar 24, 2015

cdahlqvist commented Mar 25, 2015

cdahlqvist commented Mar 26, 2015

talevy commented Mar 26, 2015

cdahlqvist commented Mar 26, 2015

cdahlqvist commented Mar 30, 2015

talevy commented Mar 30, 2015

talevy commented Apr 3, 2015

talevy commented Apr 3, 2015

iemem15 commented Apr 5, 2015

cdahlqvist commented Apr 6, 2015

talevy commented Apr 6, 2015

cdahlqvist commented Apr 7, 2015

talevy commented Apr 7, 2015

crickeys commented Apr 15, 2015

talevy commented Apr 15, 2015