-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Introduce downsampling configuration for data stream lifecycle #97041
Introduce downsampling configuration for data stream lifecycle #97041
Conversation
@elasticmachine update branch |
PARSER.declareField(ConstructingObjectParser.optionalConstructorArg(), (p, c) -> { | ||
if (p.currentToken() == XContentParser.Token.VALUE_NULL) { | ||
return Downsampling.NULL; | ||
} else { | ||
return new Downsampling(AbstractObjectParser.parseArray(p, c, Downsampling.Round::fromXContent)); | ||
} | ||
}, DOWNSAMPLING_FIELD, ObjectParser.ValueType.OBJECT_ARRAY_OR_NULL); |
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.
This approach LGTM, thanks Mary
// cc @masseyke
Pinging @elastic/es-data-management (Team:Data Management) |
Hi @gmarouli, I've created a changelog YAML for you. |
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.
Thanks for working on this Mary, this looks great
Left a couple of comments
* @param after is a TimeValue configuring how old (based on generation age) should a backing index be before downsampling | ||
* @param fixedInterval is a TimeValue configuring the interval that the backing index is going to be downsampled. | ||
*/ | ||
public record Round(TimeValue after, TimeValue fixedInterval) implements Writeable, ToXContentObject { |
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.
Could we use the existing DownsampleConfig
instead of just a TimeValue
for the fixedInterval
?
We'll later be able to use it in the DownsampleAction#Request
and implement the intervals validations similar to how we do it in https://github.com/elastic/elasticsearch/blob/main/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/downsample/TransportDownsampleAction.java#L527C43-L527C43
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.
Definitely, I was so focused on how to parse it I completely forgot to check for existing code. Thanks @andreidan
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 had to make some adjustments to the DownsamplingConfig to use it in DataLifecycle but I believe that they are not too invasive.
+ "." | ||
); | ||
} | ||
if (round.fixedInterval.compareTo(previous.fixedInterval) < 0) { |
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.
Shall we also introduce the validation that all fixed_intervals are required to be multiples of each other?
(This could be a follow-up as well)
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 will give it a try.
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 extrcted this validation so now it's shared.
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.
LGTM, this is great, thanks Mary
Thank you for the quick review and the sharp comments! 🚀 |
This change adds support for
downsampling
lifecycle config like the following:We will also support saying that explicitly do not want any downsampling (for templates):
And that we do not have a preference for downsampling:
Disclaimer
This proposal is an alternative implementation to #96848.
We propose to not add new functionality to
AbstractObjectParser.java
because our understanding is that the nullification needed for template composition is not something we want to have in general. We chose to expose the parsing of an array value, so this special array parsing can be contained within the DataLifecycle code.Part of: #93596