Handle error conditions when simulating ingest pipelines with verbosity enabled #63327
Conversation
danhermann
added
>bug
:Data Management/Ingest Node
|
Pinging @elastic/es-core-features (:Core/Features/Ingest) |
| if (e instanceof ElasticsearchException) { | ||
| ElasticsearchException elasticsearchException = (ElasticsearchException) e; | ||
| //else do nothing, let the tracking processors throw the exception while recording the path up to the failure | ||
| if (elasticsearchException.getCause() instanceof IllegalStateException) { |
There was a problem hiding this comment.
This combination of if statements allowed cases of ElasticsearchException with causes other than those of type IllegalStateException to fall through with invoking the failure handler. I believe the original intention was to catch cyclical pipelines so I believe the correct behavior is to check for both conditions at once and allow any other exception scenarios to fall through to the "now that we know there are no cycles" condition.
There was a problem hiding this comment.
Additionally, it seems brittle to check for pipeline cycles based only on the class of the inner exception when there's nothing that would prevent other processors from throwing exceptions of type IllegalStateException that have nothing to do with pipeline cycles.
There was a problem hiding this comment.
This was briefly discussed a while back #34155 (comment) ..IIRC it had to do with amount of overhead to introduce a custom exception for an edge case.
|
@elasticmachine update branch |
|
@elasticmachine run elasticsearch-ci/1 |
There was a problem hiding this comment.
LGTM - thanks for fixing this. However a REST test would be a welcome addition.
| if (e instanceof ElasticsearchException) { | ||
| ElasticsearchException elasticsearchException = (ElasticsearchException) e; | ||
| //else do nothing, let the tracking processors throw the exception while recording the path up to the failure | ||
| if (elasticsearchException.getCause() instanceof IllegalStateException) { |
There was a problem hiding this comment.
This was briefly discussed a while back #34155 (comment) ..IIRC it had to do with amount of overhead to introduce a custom exception for an edge case.
|
@elasticmachine update branch |
|
I added a REST test and narrowed the conditions used to identify the pipeline cycle error. |
Fixes #63199.