Skip to content

Remove serialization from pipeline aggregator #55026

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

Merged
merged 2 commits into from
Apr 14, 2020

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Apr 9, 2020

master doesn't need to talk to Elasticsearch versions before 7.8.0 so
PipelineAggregator doesn't need to be writable at all in master. New
pipeline aggregations don't need to worry about serializing
PipelineAggregator at all so this drops all of it.

For the most part we don't need to worry about serialization of
PipelineAggregator at all any more. When backporting a change to an
aggregator that is serialized to previous versions of Elasticsearch it
should be fairly simple to pick a value to serialize. And the compiler
should tell you that you need to do it. In many cases you this'll be a
noop. Hopefully all cases.

Closes #53742

`master` doesn't need to talk to Elasticsearch versions before 7.8.0 so
`PipelineAggregator` doesn't need to be writable *at all* in master. New
pipeline aggregations don't need to worry about serializing
`PipelineAggregator` at all so this drops all of it.

For the most part we don't need to worry about serialization of
`PipelineAggregator` at all any more. When backporting a change to an
aggregator that is serialized to previous versions of Elasticsearch it
*should* be fairly simple to pick a value to serialize. And the compiler
*should* tell you that you need to do it. In many cases you this'll be a
noop. *Hopefully* all cases.

Closes elastic#53742
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (:Analytics/Aggregations)

@nik9000 nik9000 mentioned this pull request Apr 9, 2020
3 tasks
Copy link
Contributor

@polyfractal polyfractal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these serialization changes hurt my brain, but I think this LGTM! Not sure I'd catch any tricky behavior in review but I didn't see any obvious foot-guns.

Also yay deletions :)

@nik9000 nik9000 merged commit af11ec5 into elastic:master Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pipeline aggregations are weird
4 participants