Adding settings to data streams by masseyke · Pull Request #126947 · elastic/elasticsearch

masseyke · 2025-04-16T18:47:32Z

This adds a Settings object to each DataStream. This Settings object is serialized into the cluster state, and exposed via a getSettings(), getEffectiveSettings(), and getEffectiveIndexTemplate() methods . Right now it is not used by anything, but will be in subsequent PRs.

…/elasticsearch into adding-settings-to-data-streams

elasticsearchmachine · 2025-04-17T20:37:44Z

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

lukewhiting

A couple of comments to clarify but nothing major :-) Logic looks good and tests are robust 👍🏻

lukewhiting · 2025-04-22T08:44:36Z

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

+    private static Settings mergeSettings(Settings originalSettings, Settings newSettings) {
+        if (newSettings == null || Settings.EMPTY.equals(newSettings)) {
+            return Objects.requireNonNullElse(originalSettings, Settings.EMPTY);
+        } else if (originalSettings == null || Settings.EMPTY.equals(originalSettings)) {
+            return newSettings;
+        }
+        Settings.Builder settingsBuilder = Settings.builder().put(originalSettings).put(newSettings);
+        for (String settingName : new HashSet<>(settingsBuilder.keys())) {
+            if (settingsBuilder.get(settingName) == null) {
+                settingsBuilder.remove(settingName);
+            }
+        }
+        return settingsBuilder.build();
+    }


I'm not sure but this feels like the wrong place for this kind of method... Could the this and mergeSettingsIntoTemplate(...) be part of the template builder or Settings object for better re-use / encapsulation?

Yeah I agree. It makes me a little nervous to move them because now their behavior is not clearly just meant for data streams. See what you think of the latest commit.

I think I do prefer it with the new layout... Is there a risk if things other than datastreams use it?

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

lukewhiting · 2025-04-22T08:49:57Z

Also, apologies for the delay on the review here. Easter holidays in the UK made it a 4 day weekend.

rjernst

I only reviewed the settings infra changes.

One question about the feature in general: is there validation that settings passed in are valid index settings? Should there be an explicit list of supported settings, or filters for eg private settings? I imagine we don't want all index settings to be supported here?

rjernst · 2025-04-22T19:50:37Z

server/src/main/java/org/elasticsearch/common/settings/Settings.java

+     * from the returned object.
+     */
+    public Settings merge(Settings newSettings) {
+        if (newSettings == null || Settings.EMPTY.equals(newSettings)) {


why support null? seems any callers could pass empty settings?

Yeah I can change it to throw NPE if the caller passes null if that's what you mean.

rjernst · 2025-04-22T20:33:10Z

server/src/main/java/org/elasticsearch/common/settings/Settings.java

+        if (newSettings == null || Settings.EMPTY.equals(newSettings)) {
+            return this;
+        }
+        Settings.Builder settingsBuilder = Settings.builder().put(this).put(newSettings);


Rather than merging and then removing nulls, could we loop over the new settings and decide whether to put or remove?

var builder = Settings.builder.put(this); for (String key : newSettings.keys()) { String rawValue = newSettings.get(key); if (rawValue == null) { builder.remove(key); } else { builder.put(key, rawValue); } }

rjernst · 2025-04-22T20:36:15Z

server/src/test/java/org/elasticsearch/common/settings/SettingsTests.java

    }

+    public void testMerge() {
+        {


nit: having many blocks in a single test makes understanding what failed more difficult to understand. Prefer to have each block as a separate named test method, eg testMergeEmpty, testMergeNullValue, etc.

masseyke · 2025-04-22T22:17:14Z

I only reviewed the settings infra changes.

One question about the feature in general: is there validation that settings passed in are valid index settings? Should there be an explicit list of supported settings, or filters for eg private settings? I imagine we don't want all index settings to be supported here?

I assume you're talking about in DataStream? That limitation is coming in a follow-up PR (the one that adds the transport action to set the settings). And we're initially limiting it to just 2 settings (lifecycle name and number of shards), but others will probably follow. But yes it will always be a fairly small whitelist.

rjernst

Looks good, minus a few additional comments.

rjernst · 2025-04-22T23:18:04Z

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

+     * returned unchanged. This method never changes this object.
+     */
+    public ComposableIndexTemplate mergeSettings(Settings settings) {
+        if (settings == null || Settings.EMPTY.equals(settings)) {


nit: null could also be removed here?

rjernst · 2025-04-22T23:26:41Z

server/src/main/java/org/elasticsearch/common/settings/Settings.java

+        if (Settings.EMPTY.equals(newSettings)) {
+            return this;
+        }
+        Settings.Builder builder = Settings.builder().put(this).put(newSettings);


the put(newSettings) can be removed since individual settings are added below, right?

Oops, yes. That was a mistake!

lukewhiting

I think I like the new layout more. Looks good to me bar Ryan's outstanding points 👍🏻

…eSettings

Adding settings to data streams

e93ea67

masseyke added >non-issue :StorageEngine/Data streams Data streams and their lifecycles v9.1.0 labels Apr 16, 2025

masseyke and others added 10 commits April 16, 2025 13:48

removing unused method

cf2b2c2

fixing ProjectMetadataTests

bec18b3

removing invalid MetadataTests tests

9cb64f2

[CI] Auto commit changes from spotless

781f9c6

adding a unit test

7432824

merging main

73ff2a1

Merge branch 'adding-settings-to-data-streams' of github.com:masseyke…

925f01b

…/elasticsearch into adding-settings-to-data-streams

Adding back a constructor that serverless depends on

c7ffd3e

merging main

083a437

Merge branch 'main' into adding-settings-to-data-streams

c6398d6

masseyke marked this pull request as ready for review April 17, 2025 20:37

elasticsearchmachine added the Team:Data Management (obsolete) DO NOT USE. This team no longer exists. label Apr 17, 2025

masseyke requested review from dakrone, jbaiera and lukewhiting April 17, 2025 20:37

masseyke added 4 commits April 18, 2025 07:52

merging main

0fda41e

making getEffectiveSettings more robust

9d09df7

merging main

b98d4a8

merging main

39e41c0

masseyke mentioned this pull request Apr 21, 2025

Using DataStream::getEffectiveSettings for rollover masseyke/elasticsearch#11

Draft

lukewhiting requested changes Apr 22, 2025

View reviewed changes

masseyke added 2 commits April 22, 2025 13:40

moving merge methods out of DataStream

a39ffbc

merging main

3cf1def

masseyke requested a review from a team as a code owner April 22, 2025 18:41

masseyke requested a review from lukewhiting April 22, 2025 18:42

rjernst reviewed Apr 22, 2025

View reviewed changes

masseyke added 2 commits April 22, 2025 17:36

updates to Settings::merge from code review

d85b4d7

merging main

ad9238e

masseyke requested a review from rjernst April 22, 2025 22:38

splitting up unit tests for code review

5df3d4f

rjernst approved these changes Apr 22, 2025

View reviewed changes

lukewhiting approved these changes Apr 23, 2025

View reviewed changes

masseyke added 5 commits April 23, 2025 07:57

fixing Settings::merge

a9998f5

removing support for null settings from ComposableIndexTemplate::merg…

641c645

…eSettings

Merge branch 'main' into adding-settings-to-data-streams

8d7875d

Merge branch 'main' into adding-settings-to-data-streams

b1d5944

Merge branch 'main' into adding-settings-to-data-streams

3521f8e

masseyke merged commit ee2d2f3 into elastic:main Apr 23, 2025
16 of 17 checks passed

This was referenced Apr 23, 2025

Using DataStream::getEffectiveSettings #127282

Merged

Adding transport actions for getting and updating data stream settings #127417

Merged

masseyke mentioned this pull request May 7, 2025

Adding rest actions to get and set data stream settings #127858

Merged

joegallo pushed a commit that referenced this pull request Jun 10, 2025

Adding settings to data streams (#126947)

7f0917f

joegallo mentioned this pull request Jun 10, 2025

[8.19] Backport data stream settings #129213

Merged

joegallo pushed a commit that referenced this pull request Jun 11, 2025

Adding settings to data streams (#126947)

a3fe59f

Conversation

masseyke commented Apr 16, 2025

Uh oh!

elasticsearchmachine commented Apr 17, 2025

Uh oh!

lukewhiting left a comment

Choose a reason for hiding this comment

Uh oh!

lukewhiting Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

lukewhiting commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rjernst 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!

masseyke commented Apr 22, 2025

Uh oh!

rjernst 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!

lukewhiting left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lukewhiting Apr 22, 2025 •

edited

Loading

lukewhiting commented Apr 22, 2025 •

edited

Loading