From 1cefa2fbf00cd3ec351b876f282a33ad0bb68d68 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Mon, 28 Nov 2022 14:56:56 -0500 Subject: [PATCH 1/5] Handle any exception thrown while generating source --- .../java/org/elasticsearch/ingest/IngestService.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestService.java b/server/src/main/java/org/elasticsearch/ingest/IngestService.java index 95016b688db97..49bb3844844ae 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestService.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestService.java @@ -944,11 +944,11 @@ private void innerExecute( try { boolean ensureNoSelfReferences = ingestDocument.doNoSelfReferencesCheck(); indexRequest.source(ingestDocument.getSource(), indexRequest.getContentType(), ensureNoSelfReferences); - } catch (IllegalArgumentException ex) { - // An IllegalArgumentException can be thrown when an ingest - // processor creates a source map that is self-referencing. - // In that case, we catch and wrap the exception so we can - // include which pipeline failed. + } catch (Exception ex) { + // An IllegalArgumentException can be thrown when an ingest processor creates a source map that is self-referencing. + // Rarely, a ConcurrentModificationException can be thrown if a pipeline leaks a reference to a shared mutable + // collection, and another indexing thread modifies the shared reference while we're trying to ensure it has + // no self references. If anything goes wrong here, we want to know, but also cannot proceed with normal execution. handler.accept( new IllegalArgumentException( "Failed to generate the source document for ingest pipeline [" + pipeline.getId() + "]", From 0954d7baa87b503da7eb182ac06a0771d7f531f4 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Mon, 28 Nov 2022 15:05:20 -0500 Subject: [PATCH 2/5] Update docs/changelog/91981.yaml --- docs/changelog/91981.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/changelog/91981.yaml diff --git a/docs/changelog/91981.yaml b/docs/changelog/91981.yaml new file mode 100644 index 0000000000000..c34fd6959c8da --- /dev/null +++ b/docs/changelog/91981.yaml @@ -0,0 +1,5 @@ +pr: 91981 +summary: Handle any exception thrown while generating source for an `IngestDocument` +area: Ingest Node +type: bug +issues: [] From 03aadbe8c3c43fcad11f868f3f722d9976c7ab57 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Mon, 28 Nov 2022 15:22:21 -0500 Subject: [PATCH 3/5] Widen the wrapping exception It seems misleading to use IllegalArgumentException for the general case of "something, anything, went wrong here". --- .../src/main/java/org/elasticsearch/ingest/IngestService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestService.java b/server/src/main/java/org/elasticsearch/ingest/IngestService.java index 49bb3844844ae..fa1fce926cac4 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestService.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestService.java @@ -950,7 +950,7 @@ private void innerExecute( // collection, and another indexing thread modifies the shared reference while we're trying to ensure it has // no self references. If anything goes wrong here, we want to know, but also cannot proceed with normal execution. handler.accept( - new IllegalArgumentException( + new RuntimeException( "Failed to generate the source document for ingest pipeline [" + pipeline.getId() + "]", ex ) From 6d5c870dc2f3c86186227a048dc6ec4f5165ab4e Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Mon, 28 Nov 2022 16:05:19 -0500 Subject: [PATCH 4/5] Use two catch blocks The first catch block converts to an IllegalArgumentException which maps to a 400. The second block converts to a RuntimeException which maps to a 500. For bwc reasons, we don't want to change the first block, but at the same time it's not right to return a 400 for the second situation -- we don't know what happened and can't say for sure it's a problem with the request. --- .../org/elasticsearch/ingest/IngestService.java | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestService.java b/server/src/main/java/org/elasticsearch/ingest/IngestService.java index fa1fce926cac4..48a4b47132e42 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestService.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestService.java @@ -944,11 +944,21 @@ private void innerExecute( try { boolean ensureNoSelfReferences = ingestDocument.doNoSelfReferencesCheck(); indexRequest.source(ingestDocument.getSource(), indexRequest.getContentType(), ensureNoSelfReferences); - } catch (Exception ex) { + } catch (IllegalArgumentException ex) { // An IllegalArgumentException can be thrown when an ingest processor creates a source map that is self-referencing. - // Rarely, a ConcurrentModificationException can be thrown if a pipeline leaks a reference to a shared mutable + // In that case, we catch and wrap the exception, so we can include which pipeline failed. + handler.accept( + new IllegalArgumentException( + "Failed to generate the source document for ingest pipeline [" + pipeline.getId() + "]", + ex + ) + ); + return; + } catch (Exception ex) { + // If anything goes wrong here, we want to know, but also cannot proceed with normal execution. For example, + // *rarely*, a ConcurrentModificationException could be thrown if a pipeline leaks a reference to a shared mutable // collection, and another indexing thread modifies the shared reference while we're trying to ensure it has - // no self references. If anything goes wrong here, we want to know, but also cannot proceed with normal execution. + // no self references. handler.accept( new RuntimeException( "Failed to generate the source document for ingest pipeline [" + pipeline.getId() + "]", From 6a5bb43bd9f6a6181fec3f7ce6a3e66af532e80e Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Mon, 28 Nov 2022 16:09:09 -0500 Subject: [PATCH 5/5] Reword this comment slightly --- .../src/main/java/org/elasticsearch/ingest/IngestService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestService.java b/server/src/main/java/org/elasticsearch/ingest/IngestService.java index 48a4b47132e42..3362aae1b513a 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestService.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestService.java @@ -955,7 +955,7 @@ private void innerExecute( ); return; } catch (Exception ex) { - // If anything goes wrong here, we want to know, but also cannot proceed with normal execution. For example, + // If anything goes wrong here, we want to know, and cannot proceed with normal execution. For example, // *rarely*, a ConcurrentModificationException could be thrown if a pipeline leaks a reference to a shared mutable // collection, and another indexing thread modifies the shared reference while we're trying to ensure it has // no self references.