New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Limiting the number of nested pipelines that can be executed #105428
Conversation
Hi @masseyke, I've created a changelog YAML for you. |
@@ -40,6 +41,9 @@ | |||
*/ | |||
public class ManyNestedPipelinesIT extends ESIntegTestCase { | |||
private final int manyPipelinesCount = randomIntBetween(2, 50); | |||
private final int tooManyPipelinesCount = 102; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be set to 101 if we decide to merge #105427.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, let's get that one merged in and then update this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Pinging @elastic/es-data-management (Team:Data Management) |
@@ -51,6 +51,8 @@ public final class IngestDocument { | |||
|
|||
private static final String PIPELINE_CYCLE_ERROR_MESSAGE = "Cycle detected for pipeline: "; | |||
static final String TIMESTAMP = "timestamp"; | |||
// This is the maximum number of nested pipelines that can be within a pipeline. If there are more, we bail out with an error | |||
private static final int MAX_PIPELINES = Integer.parseInt(System.getProperty("ingest.max_pipelines", "100")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A very minor nit, but I think es.ingest.max_pipelines
would be just a little better.
@@ -829,7 +831,12 @@ public void executePipeline(Pipeline pipeline, BiConsumer<IngestDocument, Except | |||
return; | |||
} | |||
|
|||
if (executedPipelines.add(pipeline.getId())) { | |||
if (executedPipelines.size() >= MAX_PIPELINES) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just thinking aloud to myself in a comment: if we have already executed X pipelines, and X is the max, then we should not execute X+1 pipelines because then we'd be executing more pipelines than the max. So there's not an off by one error here, and I'm in agreement with this logic. For the record I was considering if this if was misplaced and might have been better after the executedPipelines.add(...)
call below, but this is right and good.
…#105428) Limiting the number of nested pipelines that can be executed within a single pipeline to 100
#105533) Limiting the number of nested pipelines that can be executed within a single pipeline to 100
This limits the number of nested pipelines that can be executed from within a single pipeline to 100 (configurable via a system property called
es.ingest.max_pipelines
).