-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
[Rest Api Compatibility] MovAvgPipelineAggregation #82828
Conversation
But only when using compatible-with=7.
foo/bar paths are unusual for these tests, while foo.bar directories are very usual.
Pinging @elastic/es-analytics-geo (Team:Analytics) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good on the REST compat front, @not-napoleon - can you review the agg portion ?
one nit: can you add a comment to the Builder that it can be removed once support for compatibility with v7 is removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For an aggregation that's intended to fail at parse time, this looks fine.
@@ -0,0 +1,30 @@ | |||
--- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have never gotten a good answer for why we put BASIC-style line numbers on yml tests, but given that we do, 10 is already in use for histogram
.
import java.io.IOException; | ||
import java.util.Map; | ||
|
||
public class MovAvgPipelineAggregationBuilder extends AbstractPipelineAggregationBuilder<MovAvgPipelineAggregationBuilder> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also @deprecate
this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍, c1d7a35
Related to #51816 / #68905.
The
moving_avg
pipeline aggregation was deprecated in #29594 (6.4.0+) and removed via #39328, this PR adds back a nicer error message (but only in the presence of the relevantcompatible-with=7
headers).Quick action shot:
I expect that the vast majority of direct users of
moving_avg
have migrated over the last ~3.5 years of it being deprecated, and if you were using it indirectly via Kibana's Moving Avg then you've already been migrated tomoving_fn
for quite some time (see elastic/kibana#24702).Side note: funnily enough, because
moving_avg
andmoving_fn
are so close together in terms of edit distance, 8.0.0 without the compat header still suggests the latter if you present it with the former. You still get an error, it's the same error you'd get for a typo likemovnig_fn
.