Always retry full-request failures #523
Conversation
|
Alternate approach. Maybe we don't retry 400s, but instead detect the large requests AOT. |
|
If we assume 400 could mean too-large, we could just retry with a smaller request on 400s? (Detecting too-large requires we know the maximum size of a request body in Elasticsearch before we send the request, which may not be practical to obtain? I'm open to whatever) |
|
@jordansissel this only happens if there is a single event larger than the limit. We already attempt to split up requests in this case, but we obviously cannot subdivide a single event. |
|
The more I think about it the more I think we need a new option In the absence of the too-large-request scenario we should retry 400s indefinitely because that would indicate either:
In either case they wouldn't want to start dropping events in those cases. A stall would be appropriate. |
++ |
andrewvc
force-pushed
the
fix-retry-behavior
branch
from
430c1ce to
b66df6c
Compare
andrewvc
changed the title
andrewvc
force-pushed
the
fix-retry-behavior
branch
from
b66df6c to
9067315
Compare
|
@jordansissel OK, so I've changed the logic and updated the description per our discussion. Can I get a review from yourself? |
|
@jordansissel is there a plugin-api change I need to make now that we're using the |
andrewvc
force-pushed
the
fix-retry-behavior
branch
2 times, most recently
from
82d814c to
b4341de
Compare
| @@ -88,7 +93,7 @@ class LogStash::Outputs::ElasticSearch < LogStash::Outputs::Base | |||
| # - A sprintf style string to change the action based on the content of the event. The value `%{[foo]}` | |||
| # would use the foo field for the action | |||
| # | |||
| # For more details on actions, check out the http://www.elastic.co/guide/en/elasticsearch/reference/current/docs-bulk.html[Elasticsearch bulk API documentation] | |||
| # For more details on actions, check out the http://www.elastic.co/guide/en/elasticsearch/reference/current/docs-bulk.html[Elasticsearch bthey are:ulk API documentation] | |||
There was a problem hiding this comment.
typo inserting 'they are:' in the middle of the word 'bulk' ?
There was a problem hiding this comment.
Ah, clearly a complete ViM failure :)
| # | ||
| # This plugin will try to send appropriately sized requests, using multiple | ||
| # requests per batch if the events are very large, but if a single event | ||
| # is larger than this value sending even that single request will break. |
There was a problem hiding this comment.
"will break" --- it will break what?
| # or other things that could account for a slight difference in the HTTTP request, as our size | ||
| # check only happens on the request body. | ||
| # | ||
| # This is specified as a number now, but we should move it to the new 'bytes' type |
There was a problem hiding this comment.
"This is specified ..." will show up in the docs. I recommend not putting this comment here.
There was a problem hiding this comment.
I'll put in a note saying that we will change it in the future.
andrewvc
force-pushed
the
fix-retry-behavior
branch
from
7200f34 to
6e5b4d6
Compare
|
@jordansissel can we move this forward to LGTM? |
| # or other things that could account for a slight difference in the HTTTP request, as our size | ||
| # check only happens on the request body. | ||
| # | ||
| # This is specified as a number now, but we will be moved to the new 'bytes' type |
There was a problem hiding this comment.
I think this is a TODO item and we don't want ti showing up in the docs (which it would, in its current location in a comment above this)
|
Once the comment-that-looks-like-a-todo-item thing is removed, LGTM :) |
andrewvc
force-pushed
the
fix-retry-behavior
branch
from
6e5b4d6 to
95330c1
Compare
andrewvc
dismissed
jordansissel’s stale review
Addressed your final point Jordan! Thanks for the LGTM!
andrewvc
force-pushed
the
fix-retry-behavior
branch
2 times, most recently
from
fdf3cd7 to
95330c1
Compare
We now always retry full-request failures. It makes no sense to stop retrying a bulk request because the bulk API should always return 200. In any other situation we may lose data. Sometimes a full request can legitimately fail if there is a too-large request size from a too-large event. To fix this we now check before sending requests if the size is too large, and have added a new max_request_size option to configure this. Fixes logstash-plugins#522
andrewvc
force-pushed
the
fix-retry-behavior
branch
from
95330c1 to
36b9e77
Compare
|
I for the life of me can't repro these travis failures. They ONLY happen on 5.1.1, and they definitely don't happen locally for me :(. The lack of SSH into travis makes this even more perplexing. I tried some debugging last week, and it appeared that ES wasn't accepting TCP connections anymore on travis! Bad travis, bad! @jsvd @jordansissel any ideas here? |
|
The failure can be seen here: https://travis-ci.org/logstash-plugins/logstash-output-elasticsearch/jobs/185633956 |
We now always retry full-request failures. It makes no sense to stop retrying a bulk request because the bulk API should always return 200. In any other situation we may lose data. Sometimes a full request can legitimately fail if there is a too-large request size from a too-large event. To fix this we now check before sending requests if the size is too large, and have added a new max_request_size option to configure this.
Fixes #522
Fixes #321