Skip to content
Merged
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import org.elasticsearch.common.settings.IndexScopedSettings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.set.Sets;
import org.elasticsearch.core.Assertions;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.core.Tuple;
Expand Down Expand Up @@ -818,7 +819,7 @@ void validateIndexTemplateV2(ProjectMetadata projectMetadata, String name, Compo
var templateToValidate = indexTemplate.toBuilder().template(Template.builder(finalTemplate).settings(finalSettings)).build();

validate(name, templateToValidate, additionalSettings);
validateDataStreamsStillReferenced(projectMetadata, name, templateToValidate);
maybeValidateDataStreamsStillReferenced(projectMetadata, name, templateToValidate);
validateLifecycle(componentTemplates, name, templateToValidate, globalRetentionSettings.get(false));
validateDataStreamOptions(componentTemplates, name, templateToValidate, globalRetentionSettings.get(true));

Expand Down Expand Up @@ -944,6 +945,43 @@ static void validateDataStreamOptions(
}
}

/**
* Maybe runs {@link #validateDataStreamsStillReferenced} if it looks like the new composite template could change data stream coverage.
*/
private static void maybeValidateDataStreamsStillReferenced(
ProjectMetadata project,
String templateName,
ComposableIndexTemplate newTemplate
) {
final ComposableIndexTemplate existing = project.templatesV2().get(templateName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't believe I'm saying this (usually I'm a big fan of conserving precious characters), but perhaps existingTemplate for symmetry with newTemplate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh, I renamed the variable in another place on another branch, but forgot this one. Agreed and will do!

final Settings existingSettings = Optional.ofNullable(existing)
.map(ComposableIndexTemplate::template)
.map(Template::settings)
.orElse(Settings.EMPTY);
final Settings newSettings = Optional.ofNullable(newTemplate)
.map(ComposableIndexTemplate::template)
.map(Template::settings)
.orElse(Settings.EMPTY);
// We check whether anything relevant has changed that could affect data stream coverage and return early if not.
// These checks are based on the implementation of findV2Template and the data stream template check in this method.
// If we're adding a new template, we do the full check in case this template's priority changes coverage.
if (existing != null
Copy link
Contributor

@joegallo joegallo Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This a nit, but it's throwing me off: the use of Objects.equals here is inconsistent. I'd rather it's either used exactly and only where it is 100% necessary for the code to be correct, or that it's just used for all the stanzas without thinking. As it is right now, the Objects.equals around the two != null checks seems unnecessary.

If you're truly neutral on the only-where-necessary versus always front, I slightly prefer always, because then instead of a mix of a.equals(b) and a == b and Object.equals(a, b), we'd just have only Object.equals(a, b) four times.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This crossed my mind too! I preferred Object.equals for the != null checks because I felt like (existingTemplate.getDataStreamTemplate() != null) == (newTemplate.getDataStreamTemplate() != null) would be annoying to read and I didn't know how our spotless configuration would format it. Although the boolean boxing is unfortunate, I figured it wasn't that problematic. I agree on the consistency, though. I'll push a commit to sync them.

&& existing.indexPatterns().equals(newTemplate.indexPatterns())
&& Objects.equals(existingSettings.get(IndexMetadata.SETTING_INDEX_HIDDEN), newSettings.get(IndexMetadata.SETTING_INDEX_HIDDEN))
&& Objects.equals(existing.getDataStreamTemplate() != null, newTemplate.getDataStreamTemplate() != null)
&& existing.priorityOrZero() == newTemplate.priorityOrZero()) {
if (Assertions.ENABLED) {
try {
validateDataStreamsStillReferenced(project, templateName, newTemplate);
} catch (IllegalArgumentException e) {
assert false : "Data stream reference validation took a shortcut but the full check failed: " + e.getMessage();
}
}
return;
}
validateDataStreamsStillReferenced(project, templateName, newTemplate);
}

/**
* Validate that by changing or adding {@code newTemplate}, there are
* no unreferenced data streams. Note that this scenario is still possible
Expand All @@ -955,18 +993,16 @@ private static void validateDataStreamsStillReferenced(
String templateName,
ComposableIndexTemplate newTemplate
) {
final Set<String> dataStreams = project.dataStreams()
.entrySet()
.stream()
.filter(entry -> entry.getValue().isSystem() == false)
.map(Map.Entry::getKey)
.collect(Collectors.toSet());

Function<Map<String, ComposableIndexTemplate>, Set<String>> findUnreferencedDataStreams = composableTemplates -> {
final Set<String> unreferenced = new HashSet<>();
// For each data stream that we have, see whether it's covered by a different
// template (which is great), or whether it's now uncovered by any template
for (String dataStream : dataStreams) {
for (var dataStreamEntry : project.dataStreams().entrySet()) {
// Exclude system data streams
if (dataStreamEntry.getValue().isSystem()) {
continue;
}
final String dataStream = dataStreamEntry.getKey();
final String matchingTemplate = findV2Template(project, composableTemplates.entrySet(), dataStream, false, false);
if (matchingTemplate == null) {
unreferenced.add(dataStream);
Expand Down