Move pipeline agg validation to coordinating node#53669
Move pipeline agg validation to coordinating node#53669nik9000 merged 5 commits intoelastic:masterfrom
Conversation
This moves the pipeline aggregation validation from the data node to the coordinating node so that we, eventually, can stop sending pipeline aggregations to the data nodes entirely. In fact, it moves it into the "request validation" stage so multiple errors can be accumulated and sent back to the requester for the entire request. We can't always take advantage of that, but it'll be nice for folks not to have to play whack-a-mole with validation. This is implemented by replacing `PipelineAggretionBuilder#validate` with: ``` protected abstract void validate(ValidationContext context); ``` The `ValidationContext` handles the accumulation of validation failures, provides access to the aggregation's siblings, and implements a few validation utility methods.
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
|
This'll probably change some errors from 500s to 400s. I believe that'll be a net positive because I don't imagine anyone is relying on 500 errors for bad aggregation configuration. |
polyfractal
left a comment
polyfractal
left a comment
There was a problem hiding this comment.
++ I like this. Left a few tiny notes and optional nits.
Should we add a note to the breaking changes doc? I'm onboard with the fact these should have been 4xx errors anyway, and it's unlikely someone was depending on these particular 5xx errors in their code...but it might be nice to note it in the docs anyway?
server/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java
Show resolved
Hide resolved
| orderedPipelineAggregators = resolvePipelineAggregatorOrder(pipelineAggregatorBuilders, aggregationBuilders); | ||
| } catch (IllegalArgumentException iae) { | ||
| context.addValidationError(iae.getMessage()); | ||
| return; |
There was a problem hiding this comment.
Should we allow the validations to keep running down the tree, so we can tell the user all the problems at once?
There was a problem hiding this comment.
I was tempted but I think the tree is pretty borked at this point and you'll end up with duplicate error messages all about the same thing. And I figured we were just returning a single error message right now so it probably isn't worse than it was before and we could do it later if we wanted it.
| } | ||
|
|
||
| @Override | ||
| public void validateParentAggSequentiallyOrdered(String type, String name) { |
There was a problem hiding this comment.
Ahh yes, this thing. Would be nice someday if we could get rid of these instanceofs with some kind of isSequential() method on the agg.
Battle for another day :)
| + " must be a multi-bucket aggregation for aggregation [" + name + "] found :" | ||
| + aggBuilder.get().getClass().getName() + " for buckets path: " + bucketsPaths[0]); | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Optional: should we remove this else statement and just leave the context error as the last statement in the method?
I always feel like else are trappy/unnecessary when it's just doing some kind of error or return statement. Less rightward drift if we remove.
Totally optional, this might just be a quirk I've picked up :)
There was a problem hiding this comment.
Or, re-arrange so there's an isPresent() == false check first, and an instanceof check next, which avoids nesting.
But yeah, optional pending your preferences :D
I think I have to do that as part of the backport, right? |
nik9000
force-pushed
the
pipeline_validate_builders
branch
from
2b846b9 to
c686fe0
Compare
|
@polyfractal I believe I've done the things you asked! Thanks so much for the review. |
🤦♂️ yes, yes I believe you're right. |
…c#53669) This moves the pipeline aggregation validation from the data node to the coordinating node so that we, eventually, can stop sending pipeline aggregations to the data nodes entirely. In fact, it moves it into the "request validation" stage so multiple errors can be accumulated and sent back to the requester for the entire request. We can't always take advantage of that, but it'll be nice for folks not to have to play whack-a-mole with validation. This is implemented by replacing `PipelineAggretionBuilder#validate` with: ``` protected abstract void validate(ValidationContext context); ``` The `ValidationContext` handles the accumulation of validation failures, provides access to the aggregation's siblings, and implements a few validation utility methods.
#54019) This moves the pipeline aggregation validation from the data node to the coordinating node so that we, eventually, can stop sending pipeline aggregations to the data nodes entirely. In fact, it moves it into the "request validation" stage so multiple errors can be accumulated and sent back to the requester for the entire request. We can't always take advantage of that, but it'll be nice for folks not to have to play whack-a-mole with validation. This is implemented by replacing `PipelineAggretionBuilder#validate` with: ``` protected abstract void validate(ValidationContext context); ``` The `ValidationContext` handles the accumulation of validation failures, provides access to the aggregation's siblings, and implements a few validation utility methods.
Adds a note to the pipeline aggregation docs for error status codes changed with #53669.
4 participants
This moves the pipeline aggregation validation from the data node to the
coordinating node so that we, eventually, can stop sending pipeline
aggregations to the data nodes entirely. In fact, it moves it into the
"request validation" stage so multiple errors can be accumulated and
sent back to the requester for the entire request. We can't always take
advantage of that, but it'll be nice for folks not to have to play
whack-a-mole with validation.
This is implemented by replacing
PipelineAggretionBuilder#validatewith:
The
ValidationContexthandles the accumulation of validation failures,provides access to the aggregation's siblings, and implements a few
validation utility methods.