Allow for the data lifecycle to be explicitly nullified #95979

gmarouli · 2023-05-10T09:17:28Z

In this PR we allow a template to have a lifecycle set to null. This can enable a user to explicitly signal that they do not want to use a lifecycle in a composable template even if the component templates have lifecycle defined.

In other words, we now support the following:

"template": {
   "lifecycle": null
}

The above works in the following way:

When used in a component template, then it's the equivalent of a missing lifecycle.
When used in an index template, then it means that the user does not want any lifecycle defined in the data streams created by this template.

For example:

# Add component template with lifecycle
PUT _component_template/my-template
{
  "template": {
    "settings": {
      "number_of_shards": 1
    },
    "lifecycle": {
      "data_retention": "10d"
    }
  }
}

# Simulate index template with the null lifecycle
POST /_index_template/_simulate?include_defaults
{
  "index_patterns": ["my-data-*"],
  "composed_of": ["my-template"],
  "priority": 10,
  "template": {
    "lifecycle": null
  },
  "data_stream": {
  }
}

# Response
{
  "template": {
    "settings": {
      "index": {
        "number_of_shards": "1",
        "routing": {
          "allocation": {
            "include": {
              "_tier_preference": "data_hot"
            }
          }
        }
      }
    },
    "aliases": {},
    "lifecycle": null
  },
  "overlapping": []
}

Part of: #93596

elasticsearchmachine · 2023-05-10T09:28:51Z

Pinging @elastic/es-data-management (Team:Data Management)

elasticsearchmachine · 2023-05-10T09:28:52Z

Hi @gmarouli, I've created a changelog YAML for you.

… change

gmarouli · 2023-05-10T12:29:21Z

Bumping the compatibility version of the failed tests. Since DLM is still experimental, I think we do not need to handle backwards compatibility.

dakrone · 2023-05-10T17:26:02Z

When used in a component template, then it's the equivalent of a missing lifecycle.

How would a user specify from a component template that the data stream should be unmanaged by DLM?

gmarouli · 2023-05-11T10:07:32Z

Converted it to draft to add support for data_retention: null

gmarouli · 2023-05-11T10:10:48Z

Bumping the compatibility version of the failed tests. Since DLM is still experimental, I think we do not need to handle backwards compatibility.

When used in a component template, then it's the equivalent of a missing lifecycle.

How would a user specify from a component template that the data stream should be unmanaged by DLM?

I am also doubting that part to be honest. Right now it is not possible. A component template might not have lifecycle defined which means that it is not managed by DLM but it can never override another one that has. Only the index template can.

Should we change it and treat the same way? @dakrone is that what you had in mind?

dakrone · 2023-05-15T20:26:22Z

How would a user specify from a component template that the data stream should be unmanaged by DLM?

I am also doubting that part to be honest. Right now it is not possible. A component template might not have lifecycle defined which means that it is not managed by DLM but it can never override another one that has. Only the index template can.

Should we change it and treat the same way? @dakrone is that what you had in mind?

It's a little hard to reason about with prose, maybe a table will help us decide (the values in the table are the lifecycle config, assuming that A is before B in the composed_of list):

Component A	Component B	Composable Z	Final result
	"lifecycle": {}		"lifecycle": {}
"lifecycle": {}	"lifecycle": {"retention": "30d"}		"lifecycle": {"retention": "30d"}
"lifecycle": {"retention": "30d"}	"lifecycle": {"retention": null}		"lifecycle": {}
"lifecycle": {"retention": "30d"}	"lifecycle": null		"lifecycle": {}
"lifecycle": {"retention": "30d"}	"lifecycle": {"retention": "45d"}	"lifecycle": {}	"lifecycle": {}
"lifecycle": {"retention": "30d"}	"lifecycle": {"retention": "45d"}	"lifecycle": null
"lifecycle": {}	"lifecycle": {"retention": "45d"}	"lifecycle": {"retention": "15d"}	"lifecycle": {"retention": "15d"}
"lifecycle": {}	"lifecycle": {"retention": "45d"}	"lifecycle": {"retention": null}	"lifecycle": {}

Hopefully that helps. Also, this is just some thoughts for discussion, not necessarily the way we have to decide. Either way, I think having the table is helpful to make sure we have the same thing in mind. What do you think?

gmarouli · 2023-05-16T13:52:26Z

@dakrone thank you! This is indeed really helpful. I am going to make a table of the current composition in which explicit null values are not supported and then we can see how would this change when we introduce them:

Current behavior:
We keep the latest configuration, missing values represent that we have no opinion, so we "inherit" what the previous template has dictated.

Table 1

Component A	Component B	Composable Z	Final result
	"lifecycle": {}		"lifecycle": {}
"lifecycle": {}	"lifecycle": {"retention": "30d"}		"lifecycle": {"retention": "30d"}
"lifecycle": {"retention": "30d"}	"lifecycle": {"retention": "45d"}	"lifecycle": {}	"lifecycle": {"retention": "45d"}
"lifecycle": {}	"lifecycle": {"retention": "45d"}	"lifecycle": {"retention": "15d"}	"lifecycle": {"retention": "15d"}

Proposal # 1:
We treat a lifecycle template in the same way no matter if it is defined in a component template or in an index template.

a missing value represents no opinion, so we inherit the previous templates' opinions
a null value means we explicitly do not want this set.

This means we extend the table above with the following cases:

Table 2a

Component A	Component B	Composable Z	Final result
"lifecycle": {"retention": "30d"}	"lifecycle": {"retention": null}		"lifecycle": {}
"lifecycle": {}	"lifecycle": {"retention": "45d"}	"lifecycle": {"retention": null}	"lifecycle": {}
"lifecycle": {"retention": "30d"}	"lifecycle": {"retention": "45d"}	"lifecycle": null
"lifecycle": {"retention": "30d"}	"lifecycle": null
"lifecycle": {"retention": "30d"}	"lifecycle": null	"lifecycle": {"retention": "15d"}	"lifecycle": {"retention": "15d"}

Proposal # 2:
We treat lifecycle: null differently depending if we are a component template or an index template.

a missing always value represents no opinion, so we inherit the previous templates' opinions
a null value in component template represents no opinion, so we inherit the previous templates' opinions
a null value in an index template represents that we explicitly do not want this data stream managed.

This means we extend the table above with the following cases:

Table 2b

Component A	Component B	Composable Z	Final result
"lifecycle": {"retention": "30d"}	"lifecycle": {"retention": null}		"lifecycle": {}
"lifecycle": {}	"lifecycle": {"retention": "45d"}	"lifecycle": {"retention": null}	"lifecycle": {}
"lifecycle": {"retention": "30d"}	"lifecycle": {"retention": "45d"}	"lifecycle": null
"lifecycle": {"retention": "30d"}	"lifecycle": null		"lifecycle": {}
"lifecycle": {"retention": "30d"}	"lifecycle": null	"lifecycle": {"retention": "15d"}	"lifecycle": {"retention": "15d"}

My recommendation:
Strictly driven from the explanation of the behavior and not from the way it might be used, I favor proposal # 1 because the null values behave consistently across lifecycle, data_retention irrespective of where it's used (component template or index template). I believe it's easier to comprehend that null means ignore everything else we have seen so far.

@dakrone & @masseyke what do you think? Does this way of thinking stand?

dakrone · 2023-05-16T14:00:49Z

Thanks for presenting the proposals like this Mary — I favor proposal # 1 also. I think consistency is good to have no matter what kind of template.

masseyke · 2023-05-16T22:17:40Z

Your Table 1 has some conditions that aren't in Table 2a or 2b. I assume since null is not involved in those, the behavior would stay the same, right?
I prefer proposal 1 over proposal 2 as well. I think I'm confused by the last row in table 2a though (I probably ought to look at this in the morning instead of in the evening) -- Component B explicitly makes the lifecycle null, so doesn't that mean the final result would be null?

gmarouli · 2023-05-17T10:14:03Z

Your Table 1 has some conditions that aren't in Table 2a or 2b. I assume since null is not involved in those, the behavior would stay the same, right?

Exactly, table 1 remains the same between the two proposals and depicts how composition works right now that we do not allow the null value.

I prefer proposal 1 over proposal 2 as well. I think I'm confused by the last row in table 2a though (I probably ought to look at this in the morning instead of in the evening) -- Component B explicitly makes the lifecycle null, so doesn't that mean the final result would be null?

No worries, I can explain, there is precendence between the component template and the index template, so it works like this:

Component template A: We start with this template
Component template B: We "compose" it with A, which means add new fields or overwrite existing ones, in this case B overwrites the lifecycle defined in A and would render the data stream unmanaged if it wasn't for
Index template Z: we take the template from this one and we "compose" it with the result from A&B. Since that was a null lifecycle, there is nothing to merge and we overwrite it by the lifecycle defined in this template.

Does this make sense? If we do not do that any lifecycle:null in the list would render a data stream unmanaged which I think is too aggressive as an approach.

masseyke · 2023-05-17T14:29:38Z

here is precendence between the component template and the index template

Oh OK -- I had thought that in proposal 1 you were removing that precedence.

gmarouli · 2023-05-17T14:58:27Z

@elasticmachine update branch

gmarouli · 2023-05-30T07:06:23Z

@elasticmachine run elasticsearch-ci/bwc

andreidan

Thanks for iterating on this Mary. This is shaping up nicely.

Left a few more comments

server/src/main/java/org/elasticsearch/cluster/metadata/DataLifecycle.java

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java

server/src/main/java/org/elasticsearch/cluster/metadata/Template.java

gmarouli · 2023-05-30T12:03:37Z

Waiting for: #96428

gmarouli · 2023-05-30T12:52:28Z

@elasticmachine update branch

gmarouli · 2023-05-31T08:01:22Z

@elasticmachine update branch

andreidan

Thanks for iterating on this Mary.

Left a final batch of comments and this is almost ready 🚀

server/src/main/java/org/elasticsearch/cluster/metadata/DataLifecycle.java

andreidan · 2023-06-01T09:06:26Z

server/src/main/java/org/elasticsearch/cluster/metadata/Template.java

@@ -180,7 +206,15 @@ public void writeTo(StreamOutput out) throws IOException {
            out.writeMap(this.aliases, StreamOutput::writeString, (stream, aliasMetadata) -> aliasMetadata.writeTo(stream));
        }
        if (out.getTransportVersion().onOrAfter(TransportVersion.V_8_8_0) && DataLifecycle.isEnabled()) {
-            out.writeOptionalWriteable(lifecycle);
+            if (out.getTransportVersion().onOrAfter(TransportVersion.V_8_500_007)) {
+                boolean isExplicitNull = NO_LIFECYCLE.equals(lifecycle);


We shouldn't use equals here but ==

Should we also add a unit test that fails if equals is used here?

andreidan · 2023-06-01T09:12:33Z

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java

@@ -1508,7 +1508,7 @@ public static DataLifecycle resolveLifecycle(ComposableIndexTemplate template, M
    public static DataLifecycle composeDataLifecycles(List<DataLifecycle> lifecycles) {
        DataLifecycle.Builder builder = null;
        for (DataLifecycle current : lifecycles) {
-            if (current.equals(Template.NO_LIFECYCLE)) {
+            if (current == Template.NO_LIFECYCLE) {


Should we add a test that fails if equals is used here?

Or maybe better just implement equals as follows:

@Override public boolean equals(Object o) { return this == o; }

Do you mean to throw an exception is someone is using equals

No, just a unit test that uses a DataLifecycle that is equals to Template.NO_LIFECYCLE (e.g the INFINITE_RETENTION one) and we compose it using composeDataLifecycles and assert the correct result

This test would fail if we change the code to

if (current.equals(Template.NO_LIFECYCLE)) {

Hope that makes sense

I think it does, let me try

Ok, I am back, because both equals and == behave the same in this case, I cannot really make a unit test fail, unless you had in mind to mock DataLifecycle to ensure that equals is not called, but I am not sure we should go that far. If I am not mistaken, it's nicer to have == but since both are correct, I don't think we should go to such lengths. What do you think?

PS: the equals fails because they are not the same class type.

the equals fails because they are not the same class type

Ah, of course - the method override at instantiation time gives us this.
If we remove that override and somehow end up using equals the testLifecycleComposition test will fail (using == in the meantime makes us resilient to the method override removal)

I'm good with this 🚀

andreidan

LGTM, thanks for implementing this

gmarouli · 2023-06-01T14:17:08Z

@elasticmachine update branch

These keys allow using an explicit `null` as the value to "unset" the configuration when merging multiple component templates. As related to the merging tables seen in elastic/elasticsearch#95979 (comment) Relates also to the discussion in elastic#2049

Allow for the data lifecycle to be explicitly nullified

1045538

elasticsearchmachine added the v8.9.0 label May 10, 2023

gmarouli requested review from dakrone and masseyke May 10, 2023 09:28

gmarouli added >enhancement :Data Management/DLM labels May 10, 2023

gmarouli marked this pull request as ready for review May 10, 2023 09:28

elasticsearchmachine added the Team:Data Management Meta label for data/management team label May 10, 2023

Update docs/changelog/95979.yaml

396ea5d

Bump compatibility version of DLM to account for the lifecycle format…

bf85b5b

… change

gmarouli marked this pull request as draft May 11, 2023 07:58

Merge branch 'main' into dlm-nullify-retention

68b5256

gmarouli mentioned this pull request May 11, 2023

[Meta] Data Lifecycle Management #93596

Closed

19 tasks

Merge branch 'main' into dlm-nullify-retention

309a374

Merge with main and composing lifecycle components

63625d9

Introduce "nullyfiable" retention

bbe9101

Refactor test and add extra cases

19d69e7

elasticmachine and others added 3 commits May 29, 2023 18:53

Merge branch 'main' into dlm-nullify-retention

a2f6bf2

Remove the nullified flag from the DataLifecycle

713cfd7

Implement nullified lifecycle in the template level

dad5e59

gmarouli requested a review from andreidan May 29, 2023 19:07

Handle backwards compatibility

2165fe3

andreidan reviewed May 30, 2023

View reviewed changes

gmarouli added 3 commits May 30, 2023 13:56

Review improvements

467ac2f

Merge

a1458e9

Update new telemetry code

404a874

gmarouli requested a review from andreidan May 30, 2023 11:50

Merge branch 'main' into dlm-nullify-retention

378805b

Merge branch 'main' into dlm-nullify-retention

ecf8a7f

andreidan reviewed Jun 1, 2023

View reviewed changes

gmarouli added 3 commits June 1, 2023 16:37

Add Nullable annotation

bfbaf19

Merge branch 'main' into dlm-nullify-retention

8db0537

Change equals to ==

9730ec3

gmarouli requested a review from andreidan June 1, 2023 13:57

andreidan approved these changes Jun 1, 2023

View reviewed changes

Merge branch 'main' into dlm-nullify-retention

a74625f

gmarouli merged commit 8363e8c into elastic:main Jun 1, 2023

dakrone mentioned this pull request Jun 27, 2023

Allow nullable lifecycle and data_retention template values elastic/elasticsearch-specification#2160

Open

gmarouli deleted the dlm-nullify-retention branch August 20, 2024 06:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow for the data lifecycle to be explicitly nullified #95979

Allow for the data lifecycle to be explicitly nullified #95979

gmarouli commented May 10, 2023 •

edited

Loading

elasticsearchmachine commented May 10, 2023

elasticsearchmachine commented May 10, 2023

gmarouli commented May 10, 2023

dakrone commented May 10, 2023

gmarouli commented May 11, 2023 •

edited

Loading

gmarouli commented May 11, 2023

dakrone commented May 15, 2023

gmarouli commented May 16, 2023

dakrone commented May 16, 2023 •

edited

Loading

masseyke commented May 16, 2023

gmarouli commented May 17, 2023

masseyke commented May 17, 2023

gmarouli commented May 17, 2023

gmarouli commented May 30, 2023

andreidan left a comment

gmarouli commented May 30, 2023

gmarouli commented May 30, 2023

gmarouli commented May 31, 2023

andreidan left a comment

andreidan Jun 1, 2023

andreidan Jun 1, 2023

gmarouli Jun 1, 2023

andreidan Jun 1, 2023

gmarouli Jun 1, 2023

gmarouli Jun 1, 2023

andreidan Jun 1, 2023 •

edited

Loading

andreidan left a comment

gmarouli commented Jun 1, 2023

Allow for the data lifecycle to be explicitly nullified #95979

Allow for the data lifecycle to be explicitly nullified #95979

Conversation

gmarouli commented May 10, 2023 • edited Loading

elasticsearchmachine commented May 10, 2023

elasticsearchmachine commented May 10, 2023

gmarouli commented May 10, 2023

dakrone commented May 10, 2023

gmarouli commented May 11, 2023 • edited Loading

gmarouli commented May 11, 2023

dakrone commented May 15, 2023

gmarouli commented May 16, 2023

dakrone commented May 16, 2023 • edited Loading

masseyke commented May 16, 2023

gmarouli commented May 17, 2023

masseyke commented May 17, 2023

gmarouli commented May 17, 2023

gmarouli commented May 30, 2023

andreidan left a comment

Choose a reason for hiding this comment

gmarouli commented May 30, 2023

gmarouli commented May 30, 2023

gmarouli commented May 31, 2023

andreidan left a comment

Choose a reason for hiding this comment

andreidan Jun 1, 2023

Choose a reason for hiding this comment

andreidan Jun 1, 2023

Choose a reason for hiding this comment

gmarouli Jun 1, 2023

Choose a reason for hiding this comment

andreidan Jun 1, 2023

Choose a reason for hiding this comment

gmarouli Jun 1, 2023

Choose a reason for hiding this comment

gmarouli Jun 1, 2023

Choose a reason for hiding this comment

andreidan Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

andreidan left a comment

Choose a reason for hiding this comment

gmarouli commented Jun 1, 2023

gmarouli commented May 10, 2023 •

edited

Loading

gmarouli commented May 11, 2023 •

edited

Loading

dakrone commented May 16, 2023 •

edited

Loading

andreidan Jun 1, 2023 •

edited

Loading