Deprecate serializing PipelineAggregators #54926
Conversation
`PipelineAggregator`s are only sent across the wire for backwards compatibility with 7.7.0. `PipelineAggregator` needs to continue to implement `NamedWriteable` for backwards compatibility but pipeline aggregations created after 7.7.0 need not implement any of the methods in that interface because we'll never attempt to call them. So this creates implementations in `PipelineAggregator` (the base class) that just throw exceptions.
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
|
This is the last item for #53742 that I plan to backport to |
| bucketsPaths = in.readStringArray(); | ||
| metadata = in.readMap(); | ||
| } else { | ||
| throw new IllegalArgumentException("[" + getClass() + "] is not supported on versions before 7.8.0"); |
There was a problem hiding this comment.
Hmm, I think these exception messages are a bit confusing. If I understand correctly, this path theoretically should never happen because e.g. InternalAggregations won't serialize pipelines in 7.8+, so read/write on the aggregator 7.8+ should never be invoked.
I think we should probably change the message to "Cannot (de)serialize pipeline aggregator from stream because pipelines are not serialized after 7.8" or something to that effect. That way if a user does see this, it won't confuse them; the current message may make them think the aggregation itself is no longer supported (due to the node being 7.8+ to reach that statement, but the message saying "before 7.8").
I think we need to throw a 5xx error too, because if we get here we are firmly in the server-side going pear shaped and not the fault of a client/user. Perhaps IllegalStateException?
|
@polyfractal I pushed an update to the exception type and message in one spot. The other spots I think are actually correct because they'll happen for pipeline aggregations that never implemented serialization because they aren't supported before 7.8.0. |
There was a problem hiding this comment.
LGTM. I was going to quibble over the other IllegalArgExceptions... but I guess it's a matter of perspective. If you have a heterogeneous cluster, it's technically valid and invalid to request a new 7.8 agg. So could be viewed as a server issue (can't de/serialize), or that the user requested something bad.
👍 to keeping IllegalArg for the rest.
| bucketsPaths = in.readStringArray(); | ||
| metadata = in.readMap(); | ||
| } else { | ||
| throw new IllegalStateException("Cannot deserialize pipeline [" + getClass() + "] because it 7.8.0"); |
There was a problem hiding this comment.
Typo: "because it 7.8.0" :)
`PipelineAggregator`s are only sent across the wire for backwards compatibility with 7.7.0. `PipelineAggregator` needs to continue to implement `NamedWriteable` for backwards compatibility but pipeline aggregations created after 7.7.0 need not implement any of the methods in that interface because we'll never attempt to call them. So this creates implementations in `PipelineAggregator` (the base class) that just throw exceptions.
`PipelineAggregator`s are only sent across the wire for backwards compatibility with 7.7.0. `PipelineAggregator` needs to continue to implement `NamedWriteable` for backwards compatibility but pipeline aggregations created after 7.7.0 need not implement any of the methods in that interface because we'll never attempt to call them. So this creates implementations in `PipelineAggregator` (the base class) that just throw exceptions.
PipelineAggregators are only sent across the wire for backwardscompatibility with 7.7.0.
PipelineAggregatorneeds to continue toimplement
NamedWriteablefor backwards compatibility but pipelineaggregations created after 7.7.0 need not implement any of the methods
in that interface because we'll never attempt to call them. So this
creates implementations in
PipelineAggregator(the base class) thatjust throw exceptions.