Skip to content

Revert enrich cache lookup optimisation#85000

Merged
martijnvg merged 4 commits intoelastic:7.17from
martijnvg:revert_cache_lookup_optz
Mar 16, 2022
Merged

Revert enrich cache lookup optimisation#85000
martijnvg merged 4 commits intoelastic:7.17from
martijnvg:revert_cache_lookup_optz

Conversation

@martijnvg
Copy link
Copy Markdown
Member

This PR reverts the optimisation that was added via #77259.

This optimisation cleverly ensures no duplicate searches happen if multiple threads concurrently execute the same search.
However there are issues with the implementation that cause issues like #84781. The optimisation make use of CompletableFuture and in this case we don't check whether the result has completed exceptionally. Which causes the callback not being invoked and this leads to bulk request not being completed and hanging around.

The ingest framework due to its asynchronous nature is already complex and adding CompletableFuture into the mix makes debugging these issues very time consuming. This is the main reason why we like to revert this commit.

(this PR will be forward ported to 8.1 and master branches)

@martijnvg martijnvg added >bug :Distributed/Ingest Node Execution or management of Ingest Pipelines v8.2.0 v8.1.1 v7.17.2 labels Mar 15, 2022
@martijnvg martijnvg requested a review from jbaiera March 15, 2022 17:03
@elasticmachine elasticmachine added the Team:Data Management (obsolete) DO NOT USE. This team no longer exists. label Mar 15, 2022
@elasticmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

Hi @martijnvg, I've created a changelog YAML for you.

Copy link
Copy Markdown
Member

@jbaiera jbaiera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a shame. I took some time to see how messy the code gets to handle the corner cases. The code got quite difficult to read very quickly, and I don't think I even had half of the cases covered.

Make it so (pending green CI).

@martijnvg martijnvg force-pushed the revert_cache_lookup_optz branch from bb76d57 to 9bfebec Compare March 16, 2022 12:16
because stats get skewed due to handler in EnrichCache being invoked twice.
@martijnvg martijnvg merged commit c31dbf3 into elastic:7.17 Mar 16, 2022
@martijnvg martijnvg added >non-issue and removed >bug labels Mar 16, 2022
martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request Mar 16, 2022
Forwardporting elastic#85000 to master branch.

This PR reverts the optimisation that was added via elastic#77259.

This optimisation cleverly ensures no duplicate searches happen if multiple threads concurrently execute the same search.
However there are issues with the implementation that cause issues like elastic#84781. The optimisation make use of CompletableFuture and in this case we don't check whether the result has completed exceptionally. Which causes the callback not being invoked and this leads to bulk request not being completed and hanging around.

The ingest framework due to its asynchronous nature is already complex and adding CompletableFuture into the mix makes debugging these issues very time consuming. This is the main reason why we like to revert this commit.
martijnvg added a commit that referenced this pull request Mar 16, 2022
Forwardporting #85000 to master branch.

This PR reverts the optimisation that was added via #77259.

This optimisation cleverly ensures no duplicate searches happen if multiple threads concurrently execute the same search.
However there are issues with the implementation that cause issues like #84781. The optimisation make use of CompletableFuture and in this case we don't check whether the result has completed exceptionally. Which causes the callback not being invoked and this leads to bulk request not being completed and hanging around.

The ingest framework due to its asynchronous nature is already complex and adding CompletableFuture into the mix makes debugging these issues very time consuming. This is the main reason why we like to revert this commit.
martijnvg added a commit that referenced this pull request Mar 16, 2022
Forward port #85000 (Revert enrich cache lookup optimisation ) and #84838 (CompoundProcessor should also catch exceptions when executing a processor) to 8.1 branch.

Revert enrich cache lookup optimisation (#85028)

Forwardporting #85000 to master branch.
This PR reverts the optimisation that was added via #77259.
This optimisation cleverly ensures no duplicate searches happen if multiple threads concurrently execute the same search. However there are issues with the implementation that cause issues like #84781. The optimisation make use of CompletableFuture and in this case we don't check whether the result has completed exceptionally. Which causes the callback not being invoked and this leads to bulk request not being completed and hanging around.

The ingest framework due to its asynchronous nature is already complex and adding CompletableFuture into the mix makes debugging these issues very time consuming. This is the main reason why we like to revert this commit.

* CompoundProcessor should also catch exceptions when executing a processor (#84838) (#85035)

Currently, CompoundProcessor does not catch Exception and
if a processor throws an error and a method higher in the
call stack doesn't catch the exception then pipeline execution
stalls and bulk requests may not complete.

Usually these exceptions are caught by IngestService#executePipelines(...) method,
but when a processor executes async (for example: enrich processor) and the thread
that executes enrich is no longer the original write thread then there is no logic
that deals with failing pipeline execution and cleaning resources up. This then leads
to memory leaks.

Closes #84781

Also change how 'pipeline doesn't exist' error is thrown in TrackingResultProcessor.

With the change to CompoundProcessor thrown exceptions are caught and delegated to handler.
SimulateExecutionService in verbose mode ignores exceptions delegated to its handler, since
it assumes that processorResultList contains the result (successful or not successful) of every
processor in the pipeline.

In case TrackingResultProcessor for PipelineProcessor couldn't find the mentioned pipeline then
it just throws an error without updating the processorResultList. This commit addresses that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Distributed/Ingest Node Execution or management of Ingest Pipelines >non-issue Team:Data Management (obsolete) DO NOT USE. This team no longer exists. v7.17.2 v8.1.1 v8.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants