[Rollup] Move toBuilders() methods out of rollup config objects #32585
Conversation
tlrx
added
review
v7.0.0
>refactoring
:StorageEngine/Rollup
|
Pinging @elastic/es-search-aggs |
There was a problem hiding this comment.
For this one, I think it makes sense to keep it inside the config builders. They create values source for a composite aggregation , that's the only thing they need to do. @polyfractal WDYT ?
|
Urgh, I'm conflicted on all three of these. I generally like asking the config object to transform itself into different representations, rather than relying on external objects to transform the config object. E.g. if there's a change to the config (like if we decided to support That said, I understand not wanting to expose these to the user. I don't know enough about how the HLRC works right now... do we have to use the same config objects in both server and HLRC? Can we have a "user version" of the config and our internal version? Won't the HLRC already have access to the composite VS builders, since a user would need them to create a composite agg? |
tlrx
force-pushed
the
move-toabuilders-methods-out-of-rollup-config-objects
branch
from
1482f85 to
c530b9a
Compare
|
With the recent discussions around the HLRC maybe it's interesting to move the toBuilders methods from the configuration objects. What do you think @polyfractal ? |
tlrx
force-pushed
the
move-toabuilders-methods-out-of-rollup-config-objects
branch
from
c530b9a to
ed04fac
Compare
|
Thanks @polyfractal ! |
* master: Adjust BWC version on mapping version Token API supports the client_credentials grant (#33106) Build: forked compiler max memory matches jvmArgs (#33138) Introduce mapping version to index metadata (#33147) SQL: Enable aggregations to create a separate bucket for missing values (#32832) Fix grammar in contributing docs SECURITY: Fix Compile Error in ReservedRealmTests (#33166) APM server monitoring (#32515) Support only string `format` in date, root object & date range (#28117) [Rollup] Move toBuilders() methods out of rollup config objects (#32585) Fix forbiddenapis on java 11 (#33116) Apply publishing to genreate pom (#33094) Have circuit breaker succeed on unknown mem usage Do not lose default mapper on metadata updates (#33153) Fix a mappings update test (#33146) Reload Secure Settings REST specs & docs (#32990) Refactor CachingUsernamePassword realm (#32646)
* 6.x: Introduce mapping version to index metadata (#33147) Move non duplicated actions back into xpack core (#32952) HLRC: Create server agnostic request and response (#32912) Build: forked compiler max memory matches jvmArgs (#33138) * Added breaking change section for GROUP BY behavior: now it considers null or empty values as a separate group/bucket. Previously, they were ignored. * This is part of backporting of #32832 SQL: Enable aggregations to create a separate bucket for missing values (#32832) [TEST] version guard for reload rest-api-spec Fix grammar in contributing docs APM server monitoring (#32515) Support only string `format` in date, root object & date range (#28117) [Rollup] Move toBuilders() methods out of rollup config objects (#32585) Accept Gradle build scan agreement (#30645) Fix forbiddenapis on java 11 (#33116) Run forbidden api checks with runtimeJavaVersion (#32947) Apply publishing to genreate pom (#33094) Fix a mappings update test (#33146) Reload Secure Settings REST specs & docs (#32990) Refactor CachingUsernamePassword realm (#32646)
This pull request removes the
toBuilders()methods from the rollup configuration objects. This way they do not rely on aggregation value sources anymore and do not expose atoBuilders()method that is tighlty coupled to the rollup indexer.