Retry logic does the wrong thing

[andrewvc]

Currently our retry logic is the following:

1. Retry [409, 429, 503] up max_attempts times per batch.
2. Retry network errors infinitely.

As @gingerwizard pointed out this all makes no sense. We would expect 429/503 to retry infinitely. These both mean that Elasticsearch is just too busy to do anything useful.

For the 409 use case there's no reason to retry on the ES output. We should let Elasticsearch handle that with retry_on_conflict.

The great thing about this is we'll need less code in this plugin for better performance / functionality!

[jordansissel]

+1 on your proposal.

[andrewvc]

I have a pr open that fixes it here: #327

@untergeek or @jordansissel if you wouldn't mind taking a look?

[andrewvc]

@gingerwizard this is your xmas present!

[imriz]

This is not a sane way. There should be a way for the user to define the maximum number of attempts.
For example, we had a situation where logstash processed a huge event, and tried to send it to ES.
ES barfed on it with the following log:

[2016-04-07 04:25:19,861][WARN ][http.netty               ] [elasticsearch003] Caught exception while handling client http traffic, closing connection [id: 0xb6f62f58, /1.1.1.1:50777 => /1.1.1.2:8080]
org.elasticsearch.common.netty.handler.codec.frame.TooLongFrameException: HTTP content length exceeded 104857600 bytes.

On this error, ES closes the connection brutally, and Logstash gets a network error.

What happens then is that logstash will effectively hang! it will continue to loop on this event forever.
This is not sane.

[andrewvc]

#327 was closed, so that fixes this mostly.

#523 will be merged soon and address the issue you found @imriz

[jordansissel]

This is not a sane way. There should be a way for the user to define the maximum number of attempts.

Logstash is designed towards never dropping data, so yes, this is "sane" (btw, "sane" is not the correct word to use, here). Dropping after a certain number of attempts is still dropping data, which Logstash should never do by default. We have other features coming soon that will allow you to configure the behavior on error.

[imriz]

Logstash is designed towards never dropping data, so yes, this is "sane" (btw, "sane" is not the correct word to use, here). Dropping after a certain number of attempts is still dropping data, which Logstash should never do by default.

Note that I didn't say "by default", I said let the user have a way to configure this.
Also, I would argue that in such scenario like I described, logstash might effectively lose logs, due to log rotate, source going away or many other reasons which will cause the backlog to vanish, since it might get stuck on that single message for a long long time.

[jordansissel]

Note that I didn't say "by default",

I'm not going to argue semantics with you. The last sentence in my comment indicated that we are working on such features already to allow configuration of this behavior, copied below for emphasis:

We have other features coming soon that will allow you to configure the behavior on error.

[imriz]

I'm not going to argue semantics with you. The last sentence in my comment indicated that we are working on such features already to allow configuration of this behavior, copied below for emphasis:

Didn't mean to argue, just thought this is the right place to discuss the semantics and implementation details of the retry logic, and to emphasis and clarify what I meant.

Anyway, thank you for your reply.

[jeeyoungk]

Any update on this?

[jsvd]

The initial issue was addressed by PR #327 which was merged and released.

Please open a new issue if you're seeing another unexpected behaviour or wish to discuss an alternative.