Remove PROTOTYPE from regular aggregations #17389
Conversation
nik9000
added
>enhancement
review
:Analytics/Aggregations
|
@javanna another. |
| @@ -32,6 +32,7 @@ protected FilterAggregatorBuilder createTestAggregatorBuilder() { | |||
| // NORELEASE make RandomQueryBuilder work outside of the | |||
| // AbstractQueryTestCase | |||
| // builder.query(RandomQueryBuilder.createQuery(getRandom())); | |||
| // TODO the norelease above | |||
There was a problem hiding this comment.
These kind of scare me but I'm not sure the right way to address this. This PR is already too big.
nik9000
force-pushed
the
prototype_stomp_aggregations_2
branch
from
11d39f6
to
7508eb1
Compare
|
@javanna this is almost the same as the score function one but I did less test work along with it so it is more "pure". Still huge, sadly. |
|
Your comment in the score function one about keeping the read unction out of the parser is a good one. I can move the registration here too. |
|
I am totally tempted to leave this one to @colings86 . You are welcome Colin :) |
|
@colings86 this PR was from before I figured out that I had to do them smaller and more incrementally. So it is something of a mess. It was also before I hit on the idea that the parsers should function interfaces - so parsers are still forwarding the read method to the constructor. All and all it isn't where I'd like it to end up but it is a step. It is just in the wrong order compared to what I'd proposed for queries. I think maybe we should wait on this PR until after we've settled on the registration issue over in the query PR. |
|
I think we should leave this PR for now until we have removed the PROTOTYPE object from the query builders. By then we should have a complete plan of how best to do this and we can apply it consistently across the other parts of the codebase. I am concerned that if we try to scatter these changes around the codebase before completely one area we will end up with a lot of different ways of doing this rather than one consistent way everyone knows and can easily follow. |
This will let us migrate away from PROTOTYPES more easilly.
Always nice
This one should be easier to extend. It uses ObjectParser too!
This is finally paying off - replacing a whole class with 3 lines.
nik9000
force-pushed
the
prototype_stomp_aggregations_2
branch
from
7508eb1
to
d078027
Compare
|
@colings86 what do you want to do with this? I'm going to be ready to pick this up on Monday morning or so. I can keep going forwards, making this larger and large. Or I can abandon it and we can do it in stages like we did for the query parsers. I'm fine with whatever makes the review easiest for you. |
|
I would personally prefer abandoning this PR and doing this in stages since this PR will be very large and hard to review currently. It also means that any general problems arising from the refactoring can be sorted out quickly on the first small PR rather than having to make the fixes across all the aggregations because all the aggs get affected |
We still use PROTOTYPE in pipeline aggregations and the SignificanceHeuristicStreams, both of which can wait for other PRs. This one is large enough.
I decided to abandon doWriteTo/innerWriteTo in favor of all the classes just overriding writeTo. We were ending up with just an intense number of these. To the point where the security you get from forcing the implementer to define the method wasn't worth the complexity.