Don't check ES cluster health when flushing messages

[joschi]

Checking the ES cluster health before flushing messages was important while using the embedded Elasticsearch client node because it would block processing until the cluster is available and healthy (YELLOW or GREEN).

After migrating to an HTTP-based Elasticsearch client, this isn't necessary anymore. The client will simply fail to index the messages without blocking.

Additionally, this changeset only marks those messages as committed, which could be successfully indexed. Before this change, all messages of a batch were marked as committed and removed from the journal, whether they've been indexed or not.

[bernd]

@joschi The archive plugin needs to be adjusted for this.

[bernd]

I thought about and tested some failure scenarios with this PR.

Previously we checked if the write index alias exists before we wrote the messages into Elasticsearch. With this PR, we don't do that anymore because it's expensive. When I remove the graylog_deflector alias from the active write index, indexing new messages creates an index named graylog_deflector. Our periodical that checks the aliases then complains that the alias name is used by an actual index.

This breaks the system pretty bad. I am wondering if this is an issue in real life or if we can live with that. Not sure if switching aliases in Elasticsearch is an atomic operation of if this might happen during index rotation. We could extend our documentation and tell users to disable automatic index creation in Elasticsearch. Not sure if that breaks anything on our side, though.

Opinions?

I also thought about retrying bulk requests. Currently we are retrying a bulk request if a SocketTimeoutException is thrown. That doesn't make Graylog retry the bulk request if Elasticsearch is down and happily throws away the bulk request. So I guess we have to adjust the retry logic.

Also we are not checking if the HTTP request succeeded via BulkResult#isSucceeded().

Looking forward to your feedback. /cc @jochen @dennisoelkers @kroepke

[bernd]

Regarding the retry, I think it would be nice to get some log messages when retrying the build request. Like we do in the journalling message handler: https://github.com/Graylog2/graylog2-server/blob/master/graylog2-server/src/main/java/org/graylog2/shared/buffers/JournallingMessageHandler.java#L50-L56

[dennisoelkers]

This is not working the way it is supposed to. It just marks all messages up to and including the successfull message with the highest journal offset, including failed messages with lower journal offsets.

See this for the explanation what markJournalOffsetCommited does.

[kroepke]

@bernd We used to recommand disabling automatic index creation for that reason, but I'm not sure how long ago that was.
Switching aliases used to be atomic and I am not aware that this changed. It was one of the reasons we went with it as a write target solution in the first place, because it required no coordination.
OTOH I'm not overly happy with it because of the risks you mention.

Disabling automatic index creation and failing to have the deflector alias will very likely lose messages (depending on our retry strategy).
The only other solution I can think of is to always create the next index immediately and only switching the alias during rotation. That would minimize the chance of not having an index. But this probably requires changes all over the place, because that index should probably not be used in retention and search (and archiving)

Given all that, my vote would be to take the risk for the beta now and to throw more testing against it to see how it behaves under load.

[bernd]

LGTM 👍