[DSL Global Retention] Use data stream global retention metadata#106221
[DSL Global Retention] Use data stream global retention metadata#106221gmarouli merged 26 commits intoelastic:mainfrom
Conversation
|
Pinging @elastic/es-data-management (Team:Data Management) |
|
Known failure: #106457 |
docs/reference/data-streams/lifecycle/tutorial-manage-new-data-stream.asciidoc
Outdated
Show resolved
Hide resolved
| public static final ConstructingObjectParser<DataStreamLifecycle, Void> PARSER = new ConstructingObjectParser<>( | ||
| "lifecycle", | ||
| false, | ||
| true, |
There was a problem hiding this comment.
I'm a little confused by this change. I think it's because we don't expect (or allow) the user to pass in effective_retention or retention_determined_by, but we use this parser for round-trip serialization tests, which need to use that. Is that right? It might be worth a short comment in the code since the ConstructingObjectParser constructor's javadocs recommend against setting it to true (This should generally be set to true only when parsing responses from external systems, never when parsing requests from users).
There was a problem hiding this comment.
Oh it's more than just the round-trip serialization test -- I tried setting this to false to see what else was impacted, and a yaml rest test also failed when the xcontent was being deserialized by DataStreamLifecycle.fromXContent().
There was a problem hiding this comment.
Thank you for raising this topic @masseyke . I spent some more time looking around and I decided to change the approach for the following reasons:
- Well, javadoc is very specific for the use of
ignoreUnknownFields:
@param ignoreUnknownFields Should this parser ignore unknown fields? This should generally be set to true only when parsing responses from external systems, never when parsing requests from users.
DataStreamLifecycleis used for user input and usingignoreUnknownFieldsmeans that a user could add different fields includingeffective_retentionwhich will be ignored but the user won't know.- The problem here I think is that
DataStreamLifecycleis not only used for user input but it's also deserialised in the cluster state as XContent as part of the data stream.
As you see we do not have this problem with the rollover information, because they are only used when creating a response to the user. So, I decided to follow this approach. I added a flag, that is set only when we are creating a response to the user and it's false otherwise. This will ensure that the new and unexpected fields will only be present in the user responses and not during other serialisations.
There was a problem hiding this comment.
I requested another review because the approach changed and I would like your input. Thanks again for bringing this up! I think the original approach was not thought through properly.
There was a problem hiding this comment.
And uncovered a mistake on a payload... that's why the yaml was failing, or at least one of the reasons
There was a problem hiding this comment.
What you did makes sense to me. I like that we no longer ignore unknown fields.
|
It looks good to me. It would probably be good to have some yaml rest tests -- maybe you're waiting to add those until you've added a way to set global retention? |
masseyke
left a comment
There was a problem hiding this comment.
I had a few minor questions/comments, but it looks good to me.
Yes, indeed that would be in the final PR. |
|
Docs preview here: https://elasticsearch_bk_106221.docs-preview.app.elstc.co/diff |
In this PR we make use in the code the global retention metadata that we introduced in #106170. We use it in responses of
GETrequests when data stream lifecycle was included.Currently, there is no way to set the global retention for the user, so this PR is also marked as a
>non-issue, it adds two new fields but the global retention functionality cannot influence them yet.