Fix 'autoGeneratedTimestamp should not be set externally' error when retrying IndexRequest#86184
Conversation
|
Pinging @elastic/es-data-management (Team:Data Management) |
|
Hi @martijnvg, I've created a changelog YAML for you. |
…retrying IndexRequest We use `BulkProcessor` to index various features internally (ilm history, watcher history and more). When an index request is retried then resending that same request can cause the following error: ``` java.lang.IllegalArgumentException: autoGeneratedTimestamp should not be set externally at org.elasticsearch.action.bulk.TransportBulkAction.doInternalExecute(TransportBulkAction.java:223) ~[elasticsearch-7.16.1.jar:7.16.1] at org.elasticsearch.action.bulk.TransportBulkAction$1.doRun(TransportBulkAction.java:192) ~[elasticsearch-7.16.1.jar:7.16.1] at org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:26) [elasticsearch-7.16.1.jar:7.16.1] at org.elasticsearch.action.bulk.TransportBulkAction.doExecute(TransportBulkAction.java:200) [elasticsearch-7.16.1.jar:7.16.1] at org.elasticsearch.action.bulk.TransportBulkAction.doExecute(TransportBulkAction.java:91) [elasticsearch-7.16.1.jar:7.16.1] ``` This happens when index requests fail because of a retryable error (e.g. EsRejectedExecutionException). Prior to this change, the `BulkProcessor` (via `Retry`) then reuses the same instance. With this change, a copy is made of the original `IndexRequest`, so that the retry is performed with an unset `autoGeneratedTimestamp` field. Closes elastic#83927
1fa52b5 to
fc170e2
Compare
|
@martijnvg what's your thought on this solution versus doing something like adding a |
|
@dakrone I think I'm in favour of that instead of creating a new instance based on the original. The copying is error prone, especially if new fields are added in the future to IndexRequest. I will add the reset method and remove the IndexRequest best effort cloning. |
…amp_error_when_retryin_index_request
…p field instead of best effort cloning IndexRequest.
dakrone
left a comment
There was a problem hiding this comment.
LGTM assuming CI is happy. Do we have a way to add a test for this that doesn't bend over backwards in complexity?
I don't think so? This doesn't change behaviour or formats. The logic here is tested via |
We use
BulkProcessorto index various features internally (ilm history, watcher history and more).When an index request is retried then resending that same request can cause the following error:
This happens when index requests fail because of a retryable error (e.g. EsRejectedExecutionException).
Prior to this change, the
BulkProcessor(viaRetry) then reuses the same instance.With this change, the
autoGeneratedTimestampfield is unset via a reset method added toIndexRequestclass,which should avoid the IAE.
Closes #83927