Excluding system data streams from global and factory retention by masseyke · Pull Request #108038 · elastic/elasticsearch

masseyke · 2024-04-29T18:37:35Z

We have the notion of global and factory retention for data streams, where we declare the default and maximum retention for all data streams. However, we don't want system data streams to be constrained by these. For example, some system data streams will need to have unlimited retention. This PR makes it so that system data streams are not affected by global or factory data stream retention.

elasticsearchmachine · 2024-04-29T18:38:00Z

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

…ithub.com:masseyke/elasticsearch into exclude-system-data-streams-from-global-retention

nielsbauman

I know this is just a draft, but I placed some comments anyway, as I was afraid I'd forget them otherwise 😄. Hopefully, there's not too much overlap with what you were already planning on doing but just hadn't come around to yet.

docs/changelog/108038.yaml

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

...ain/java/org/elasticsearch/action/datastreams/lifecycle/ExplainIndexDataStreamLifecycle.java

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

nielsbauman · 2024-04-30T07:15:47Z

server/src/test/java/org/elasticsearch/cluster/metadata/ComposableIndexTemplateTests.java

+            for (String label : rolloverConfiguration.resolveRolloverConditions(
+                lifecycle.getEffectiveDataRetention(globalRetention, randomBoolean())
+            ).getConditions().keySet()) {


Maybe we can extract the rollover conditions to a variable to make this a bit more readable? Same goes for the other changes in tests.

server/src/test/java/org/elasticsearch/cluster/metadata/ComposableIndexTemplateTests.java

server/src/test/java/org/elasticsearch/cluster/metadata/DataStreamLifecycleTests.java

…ithub.com:masseyke/elasticsearch into exclude-system-data-streams-from-global-retention

… retention for calcuating rollover for display

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

nielsbauman · 2024-05-02T18:05:49Z

...ain/java/org/elasticsearch/action/datastreams/lifecycle/ExplainIndexDataStreamLifecycle.java

+        if (in.getTransportVersion().onOrAfter(TransportVersions.NO_LIFECYCLE_FOR_SYSTEM_DATA_STREAMS)) {
+            this.isSystemDataStream = in.readBoolean();
+        } else {
+            this.isSystemDataStream = false;
+        }


Nit:

Suggested change

if (in.getTransportVersion().onOrAfter(TransportVersions.NO_LIFECYCLE_FOR_SYSTEM_DATA_STREAMS)) {

this.isSystemDataStream = in.readBoolean();

} else {

this.isSystemDataStream = false;

}

this.isSystemDataStream = in.getTransportVersion().onOrAfter(TransportVersions.NO_LIFECYCLE_FOR_SYSTEM_DATA_STREAMS)) && in.readBoolean();

I kind of like the simplicity of the if/else here. That's just a matter of personal preference, right? The outcome is the same either way?

I'd argue that the one-liner is just as simple 😛. But it's definitely just personal preference as I've seen both variations and the outcome is the same. I'm fine with leaving it like this if that's your preference.

server/src/test/java/org/elasticsearch/cluster/metadata/DataStreamLifecycleTests.java

elasticsearchmachine · 2024-05-02T22:05:57Z

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

nielsbauman

Some last comments form my side, after those we'll be good to go :)

nielsbauman · 2024-05-03T06:47:54Z

...ava/org/elasticsearch/datastreams/lifecycle/UpdateDataStreamGlobalRetentionServiceTests.java

+                assertThat(dataStream.newEffectiveRetention(), equalTo(globalRetention.getMaxRetention()));
+            } else {
+                assertThat(affectedDataStreams.size(), is(1));
+                var dataStream = affectedDataStreams.get(1);


This is going to give an IndexOutOfBoundsException.

Oops. Not sure how I missed that!

nielsbauman · 2024-05-03T07:01:08Z

...alClusterTest/java/org/elasticsearch/datastreams/lifecycle/ExplainDataStreamLifecycleIT.java

+    public void testSystemExplainLifecycle() throws Exception {
+        /*
+         * This test makes sure that for system data streams, we correctly ignore the global retention when calling
+         * ExplainDataStreamLifecycle. It is very simila to testExplainLifecycle, but only focuses on the retention for a system index.


Nit:

Suggested change

* ExplainDataStreamLifecycle. It is very simila to testExplainLifecycle, but only focuses on the retention for a system index.

* ExplainDataStreamLifecycle. It is very similar to testExplainLifecycle, but only focuses on the retention for a system index.

nielsbauman · 2024-05-03T07:13:33Z

server/src/main/java/org/elasticsearch/TransportVersions.java

    public static final TransportVersion SECURITY_ROLE_MAPPINGS_IN_CLUSTER_STATE = def(8_647_00_0);
    public static final TransportVersion ESQL_REQUEST_TABLES = def(8_648_00_0);
    public static final TransportVersion ROLE_REMOTE_CLUSTER_PRIVS = def(8_649_00_0);
+    public static final TransportVersion NO_LIFECYCLE_FOR_SYSTEM_DATA_STREAMS = def(8_650_00_0);


Nit:

Suggested change

public static final TransportVersion NO_LIFECYCLE_FOR_SYSTEM_DATA_STREAMS = def(8_650_00_0);

public static final TransportVersion NO_GLOBAL_RETENTION_FOR_SYSTEM_DATA_STREAMS = def(8_650_00_0);

nielsbauman · 2024-05-03T07:13:55Z

...alClusterTest/java/org/elasticsearch/datastreams/lifecycle/ExplainDataStreamLifecycleIT.java

+            assertThat(
+                explainIndex.getLifecycle().getDataStreamRetention(),
+                equalTo(TimeValue.timeValueDays(SYSTEM_DATA_STREAM_RETENTION_DAYS))
+            );


Isn't this test missing a request that actually sets a global retention? Without such a request, this assertion becomes rather trivial, I think. But maybe I'm missing something.

Oops. Good catch!

nielsbauman

LGTM! Thanks for iterating on this Keith :)

Excluding system data streams from global and factory retention

959c4c8

masseyke added >enhancement :StorageEngine/Data streams Data streams and their lifecycles v8.15.0 labels Apr 29, 2024

Update docs/changelog/108038.yaml

892b36a

masseyke added 3 commits April 29, 2024 14:28

fixing calculation

e28c0a4

improving tests

5ec38d1

Merge branch 'exclude-system-data-streams-from-global-retention' of g…

232c442

…ithub.com:masseyke/elasticsearch into exclude-system-data-streams-from-global-retention

nielsbauman reviewed Apr 30, 2024

View reviewed changes

masseyke added 3 commits April 30, 2024 11:28

adding tests

40ff495

cleaning up resources after testSystemDataStreamRetention

ea71ebc

code review feedback

df52058

masseyke added >non-issue and removed >enhancement labels Apr 30, 2024

masseyke added 4 commits April 30, 2024 16:03

Delete docs/changelog/108038.yaml

c47801d

merging main

f9137cb

Merge branch 'exclude-system-data-streams-from-global-retention' of g…

0fa423a

…ithub.com:masseyke/elasticsearch into exclude-system-data-streams-from-global-retention

templates never show effective_retention, and should never use global…

81427b8

… retention for calcuating rollover for display

nielsbauman reviewed May 1, 2024

View reviewed changes

server/src/main/java/org/elasticsearch/cluster/metadata/DataStreamLifecycle.java Outdated Show resolved Hide resolved

masseyke added 5 commits May 1, 2024 13:29

merging main

f82d9e7

cleanup

ba7b91b

cleanup

4344dd4

minor cleanup

87fc681

merging main

8c5a93f

masseyke mentioned this pull request May 1, 2024

Removing global retention functionality for templates #108170

Merged

masseyke added 2 commits May 2, 2024 11:06

merging main

59dd38b

removing ignoreGlobalRetention parameter

2f15fc5

nielsbauman reviewed May 2, 2024

View reviewed changes

adding tests

475550b

merging main

66b557a

masseyke marked this pull request as ready for review May 2, 2024 22:05

masseyke requested a review from nielsbauman May 2, 2024 22:05

elasticsearchmachine added the Team:Data Management (obsolete) DO NOT USE. This team no longer exists. label May 2, 2024

masseyke requested a review from dakrone May 2, 2024 22:08

nielsbauman reviewed May 3, 2024

View reviewed changes

code review feedback

ea2be97

masseyke requested a review from nielsbauman May 3, 2024 13:08

nielsbauman approved these changes May 3, 2024

View reviewed changes

masseyke merged commit a561958 into elastic:main May 3, 2024

masseyke deleted the exclude-system-data-streams-from-global-retention branch May 3, 2024 14:09

	* ExplainDataStreamLifecycle. It is very simila to testExplainLifecycle, but only focuses on the retention for a system index.
	* ExplainDataStreamLifecycle. It is very similar to testExplainLifecycle, but only focuses on the retention for a system index.

	public static final TransportVersion NO_LIFECYCLE_FOR_SYSTEM_DATA_STREAMS = def(8_650_00_0);
	public static final TransportVersion NO_GLOBAL_RETENTION_FOR_SYSTEM_DATA_STREAMS = def(8_650_00_0);

Conversation

masseyke commented Apr 29, 2024

Uh oh!

elasticsearchmachine commented Apr 29, 2024

Uh oh!

nielsbauman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticsearchmachine commented May 2, 2024

Uh oh!

nielsbauman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nielsbauman left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants