Fix retry handling - #138
Merged
Merged
Conversation
…etries When a bulk indexing request fails, we send another bulk request with only the records that failed as a retry. Because our `Transaction` object keeps track of all the items in the original batch, we have to map items in the _new_ batch, back to the old batch. This has never been done properly and is one cause of strange errors and bad retry logic in this repo.
Our logic for calculating status from a batch that was retried had a bug: it looked at the previous retry (or original attempt) status, and would only ever raise the overall batch status to an equal or higher value. So a 2xx could become 4xx or 5xx, but 5xx (failure) could never become 2xx (success). This lead to a pattern we've seen many times where once a retry happens, it continues several times and looks to actually fail. In reality, the second retry likely succeeded, but that information is lost by the `dbclient`. Now we correctly recalculate batch status, so if a retry is successful it will be recorded as such and no longer retried.
…imits configurable
Retries were fired synchronously and immediately, with no delay, up to 5
times. If a batch is failing because elasticsearch is overloaded (bulk
queue rejections, request timeouts), retrying instantly just adds more
load right when the cluster needs to drain, and the whole retry budget
can be burned through in milliseconds.
Add exponential backoff with jitter between retries, and make the retry
count and backoff bounds configurable via dbclient.retry.{max,baseDelay,
maxDelay} in pelias.json (defaults: 5 retries, 1s base, 30s cap - same
retry count as before).
Also log the specific record ids still failing when a batch is finally
dropped after exhausting retries, instead of just an opaque
'reached max retries' with no way to tell which records were affected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes 3 different but related issues with retry handling here. We've noticed the effects of these issues for years but never fully chased them down.
Fortunately the existing retry logic usually worked, but it would show a lot of noisy errors that turned out to be actual success cases.
First issue: incorrect accounting of individual records during retries
When retrying a batch, the logic in this repo correctly only retries the records in a batch that failed (Elasticsearch's bulk endpoint returns a status code for each record). However, it didn't keep track of them correctly, and errors in one record could be assigned to another record that actually succeeded.
Second issue: fix tracking of overall status across retries of a batch
That first bug actually didn't really matter, because there was an even more glaring bug in how retries worked. Once a batch failed, the code would never correctly detect that a retry succeeded. It would always interpret every retry as a failure and keep retrying.
This means most of the time, when you see a bunch of retries in a row during a Pelias import, a subsequent retry probably worked. I can definitely attest that this lead us to ignoring these errors because everything "seemed to work".
Third issue: retries were immediate with no delay whatsoever
In our experience the main reason for a batch indexing failure is an overloaded Elasticsearch cluster. Our retries were probably making this issue worse, because the retries happened immediately upon receiving response from Elasticsearch, even if other batch indexing requests were paused.
This is an easy fix, we add some exponential backoff and now retries should only continue after an overloaded cluster has had a chance to cool down. The retry settings are configurable via
pelias.json.