Skip to content

Commit 52456cb

Browse files
nielsbaumannielsbauman
authored
Add small optimizations to PUT _component_template API (#135644)
By moving component template normalization to the transport action, we can perform an equality check on the component template before creating a cluster state update task. This reduces the load on the master/cluster state update thread for no-op update requests. We still need to perform the equality check in the cluster state update task, as the cluster state could have been updated after we performed the check in the transport action. But avoiding a cluster state update task altogether in some cases is worth adding the check to the transport action. Additionally, by making some small refactorings in `MetadataIndexTemplateService#addComponentTemplate` we avoid building some objects unnecessarily and improve readability.
1 parent 66e9e81 commit 52456cb

File tree

10 files changed

+674
-630
lines changed

10 files changed

+674
-630
lines changed

‎docs/changelog/135644.yaml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 135644
2+
summary: Add small optimizations to `PUT _component_template` API
3+
area: Indices APIs
4+
type: enhancement
5+
issues: []

‎server/src/internalClusterTest/java/org/elasticsearch/indices/template/ComposableTemplateIT.java‎

Lines changed: 542 additions & 7 deletions
Large diffs are not rendered by default.

‎server/src/main/java/org/elasticsearch/action/admin/indices/template/put/TransportPutComponentTemplateAction.java‎

Lines changed: 18 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,10 @@
1818
import org.elasticsearch.cluster.block.ClusterBlockException;
1919
import org.elasticsearch.cluster.block.ClusterBlockLevel;
2020
import org.elasticsearch.cluster.metadata.ComponentTemplate;
21-
import org.elasticsearch.cluster.metadata.IndexMetadata;
2221
import org.elasticsearch.cluster.metadata.MetadataIndexTemplateService;
23-
import org.elasticsearch.cluster.metadata.Template;
2422
import org.elasticsearch.cluster.project.ProjectResolver;
2523
import org.elasticsearch.cluster.project.ProjectStateRegistry;
2624
import org.elasticsearch.cluster.service.ClusterService;
27-
import org.elasticsearch.common.settings.IndexScopedSettings;
28-
import org.elasticsearch.common.settings.Settings;
2925
import org.elasticsearch.common.util.concurrent.EsExecutors;
3026
import org.elasticsearch.core.FixForMultiProject;
3127
import org.elasticsearch.injection.guice.Inject;
@@ -39,7 +35,6 @@
3935
public class TransportPutComponentTemplateAction extends AcknowledgedTransportMasterNodeAction<PutComponentTemplateAction.Request> {
4036

4137
private final MetadataIndexTemplateService indexTemplateService;
42-
private final IndexScopedSettings indexScopedSettings;
4338
private final ProjectResolver projectResolver;
4439

4540
@Inject
@@ -49,7 +44,6 @@ public TransportPutComponentTemplateAction(
4944
ThreadPool threadPool,
5045
MetadataIndexTemplateService indexTemplateService,
5146
ActionFilters actionFilters,
52-
IndexScopedSettings indexScopedSettings,
5347
ProjectResolver projectResolver
5448
) {
5549
super(
@@ -62,7 +56,6 @@ public TransportPutComponentTemplateAction(
6256
EsExecutors.DIRECT_EXECUTOR_SERVICE
6357
);
6458
this.indexTemplateService = indexTemplateService;
65-
this.indexScopedSettings = indexScopedSettings;
6659
this.projectResolver = projectResolver;
6760
}
6861

@@ -71,46 +64,36 @@ protected ClusterBlockException checkBlock(PutComponentTemplateAction.Request re
7164
return state.blocks().globalBlockedException(projectResolver.getProjectId(), ClusterBlockLevel.METADATA_WRITE);
7265
}
7366

74-
public static ComponentTemplate normalizeComponentTemplate(
75-
ComponentTemplate componentTemplate,
76-
IndexScopedSettings indexScopedSettings
77-
) {
78-
Template template = componentTemplate.template();
79-
// Normalize the index settings if necessary
80-
if (template.settings() != null) {
81-
Settings.Builder builder = Settings.builder().put(template.settings()).normalizePrefix(IndexMetadata.INDEX_SETTING_PREFIX);
82-
Settings settings = builder.build();
83-
indexScopedSettings.validate(settings, true);
84-
template = Template.builder(template).settings(settings).build();
85-
componentTemplate = new ComponentTemplate(
86-
template,
87-
componentTemplate.version(),
88-
componentTemplate.metadata(),
89-
componentTemplate.deprecated(),
90-
componentTemplate.createdDateMillis().orElse(null),
91-
componentTemplate.modifiedDateMillis().orElse(null)
92-
);
93-
}
94-
95-
return componentTemplate;
96-
}
97-
9867
@Override
9968
protected void masterOperation(
10069
Task task,
10170
final PutComponentTemplateAction.Request request,
10271
final ClusterState state,
10372
final ActionListener<AcknowledgedResponse> listener
104-
) {
105-
ComponentTemplate componentTemplate = normalizeComponentTemplate(request.componentTemplate(), indexScopedSettings);
106-
var projectId = projectResolver.getProjectId();
73+
) throws Exception {
74+
final var project = projectResolver.getProjectMetadata(state);
75+
final ComponentTemplate componentTemplate = indexTemplateService.normalizeComponentTemplate(request.componentTemplate());
76+
final ComponentTemplate existingTemplate = project.componentTemplates().get(request.name());
77+
if (existingTemplate != null) {
78+
if (request.create()) {
79+
listener.onFailure(new IllegalArgumentException("component template [" + request.name() + "] already exists"));
80+
return;
81+
}
82+
// We have an early return here in case the component template already exists and is identical in content. We still need to do
83+
// this check in the cluster state update task in case the cluster state changed since this check.
84+
if (componentTemplate.contentEquals(existingTemplate)) {
85+
listener.onResponse(AcknowledgedResponse.TRUE);
86+
return;
87+
}
88+
}
89+
10790
indexTemplateService.putComponentTemplate(
10891
request.cause(),
10992
request.create(),
11093
request.name(),
11194
request.masterNodeTimeout(),
11295
componentTemplate,
113-
projectId,
96+
project.id(),
11497
listener
11598
);
11699
}

‎server/src/main/java/org/elasticsearch/action/admin/indices/template/reservedstate/ReservedComposableIndexTemplateAction.java‎

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818
import org.elasticsearch.cluster.metadata.MetadataIndexTemplateService;
1919
import org.elasticsearch.cluster.metadata.ProjectId;
2020
import org.elasticsearch.cluster.metadata.ProjectMetadata;
21-
import org.elasticsearch.common.settings.IndexScopedSettings;
2221
import org.elasticsearch.common.util.set.Sets;
2322
import org.elasticsearch.reservedstate.ReservedClusterStateHandler;
2423
import org.elasticsearch.reservedstate.ReservedProjectStateHandler;
@@ -56,14 +55,9 @@ public class ReservedComposableIndexTemplateAction
5655
public static final String COMPOSABLE_PREFIX = "composable_index_template:";
5756

5857
private final MetadataIndexTemplateService indexTemplateService;
59-
private final IndexScopedSettings indexScopedSettings;
6058

61-
public ReservedComposableIndexTemplateAction(
62-
MetadataIndexTemplateService indexTemplateService,
63-
IndexScopedSettings indexScopedSettings
64-
) {
59+
public ReservedComposableIndexTemplateAction(MetadataIndexTemplateService indexTemplateService) {
6560
this.indexTemplateService = indexTemplateService;
66-
this.indexScopedSettings = indexScopedSettings;
6761
}
6862

6963
@Override
@@ -154,11 +148,7 @@ public TransformState transform(ProjectId projectId, ComponentsAndComposables so
154148

155149
// 1. create or update component templates (composable templates depend on them)
156150
for (var request : components) {
157-
ComponentTemplate template = TransportPutComponentTemplateAction.normalizeComponentTemplate(
158-
request.componentTemplate(),
159-
indexScopedSettings
160-
);
161-
151+
ComponentTemplate template = indexTemplateService.normalizeComponentTemplate(request.componentTemplate());
162152
project = indexTemplateService.addComponentTemplate(project, false, request.name(), template);
163153
}
164154

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

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -188,12 +188,23 @@ public boolean equals(Object obj) {
188188
return false;
189189
}
190190
ComponentTemplate other = (ComponentTemplate) obj;
191+
return contentEquals(other)
192+
&& Objects.equals(createdDateMillis, other.createdDateMillis)
193+
&& Objects.equals(modifiedDateMillis, other.modifiedDateMillis);
194+
}
195+
196+
/**
197+
* Check whether the content of this component template is equal to another component template. Can be used to determine if a template
198+
* already exists.
199+
*/
200+
public boolean contentEquals(ComponentTemplate other) {
201+
if (other == null) {
202+
return false;
203+
}
191204
return Objects.equals(template, other.template)
192205
&& Objects.equals(version, other.version)
193206
&& Objects.equals(metadata, other.metadata)
194-
&& Objects.equals(deprecated, other.deprecated)
195-
&& Objects.equals(createdDateMillis, other.createdDateMillis)
196-
&& Objects.equals(modifiedDateMillis, other.modifiedDateMillis);
207+
&& Objects.equals(deprecated, other.deprecated);
197208
}
198209

199210
@Override

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

Lines changed: 66 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -294,25 +294,26 @@ public ProjectMetadata execute(ProjectMetadata currentProject) throws Exception
294294
);
295295
}
296296

297-
// Public visible for testing
297+
/**
298+
* Add the given component template to the project. If {@code create} is true, we will fail if there exists a component template with
299+
* the same name. If a component template with the same name exists, but the content is identical, no change will be made.
300+
* This method will perform all necessary validation but assumes that the component template has already been normalized (see
301+
* {@link #normalizeComponentTemplate(ComponentTemplate)}.
302+
*/
298303
public ProjectMetadata addComponentTemplate(
299304
final ProjectMetadata project,
300305
final boolean create,
301306
final String name,
302307
final ComponentTemplate template
303-
) throws Exception {
304-
final ComponentTemplate existing = project.componentTemplates().get(name);
305-
if (create && existing != null) {
306-
throw new IllegalArgumentException("component template [" + name + "] already exists");
307-
}
308-
309-
CompressedXContent mappings = template.template().mappings();
310-
CompressedXContent wrappedMappings = wrapMappingsIfNecessary(mappings, xContentRegistry);
311-
312-
// We may need to normalize index settings, so do that also
313-
Settings finalSettings = template.template().settings();
314-
if (finalSettings != null) {
315-
finalSettings = Settings.builder().put(finalSettings).normalizePrefix(IndexMetadata.INDEX_SETTING_PREFIX).build();
308+
) throws IOException {
309+
final ComponentTemplate existingTemplate = project.componentTemplates().get(name);
310+
if (existingTemplate != null) {
311+
if (create) {
312+
throw new IllegalArgumentException("component template [" + name + "] already exists");
313+
}
314+
if (template.contentEquals(existingTemplate)) {
315+
return project;
316+
}
316317
}
317318

318319
// Collect all the composable (index) templates that use this component template, we'll use
@@ -325,9 +326,9 @@ public ProjectMetadata addComponentTemplate(
325326
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
326327

327328
// if we're updating a component template, let's check if it's part of any V2 template that will yield the CT update invalid
328-
if (create == false && finalSettings != null) {
329+
if (create == false && template.template().settings() != null) {
329330
// if the CT is specifying the `index.hidden` setting it cannot be part of any global template
330-
if (IndexMetadata.INDEX_HIDDEN_SETTING.exists(finalSettings)) {
331+
if (IndexMetadata.INDEX_HIDDEN_SETTING.exists(template.template().settings())) {
331332
List<String> globalTemplatesThatUseThisComponent = new ArrayList<>();
332333
for (Map.Entry<String, ComposableIndexTemplate> entry : templatesUsingComponent.entrySet()) {
333334
ComposableIndexTemplate templateV2 = entry.getValue();
@@ -351,47 +352,27 @@ public ProjectMetadata addComponentTemplate(
351352
}
352353
}
353354

354-
final Template finalTemplate = Template.builder(template.template()).settings(finalSettings).mappings(wrappedMappings).build();
355-
final long now = instantSource.instant().toEpochMilli();
356-
final ComponentTemplate finalComponentTemplate;
357-
if (existing == null) {
358-
finalComponentTemplate = new ComponentTemplate(
359-
finalTemplate,
360-
template.version(),
361-
template.metadata(),
362-
template.deprecated(),
363-
now,
364-
now
365-
);
366-
} else {
367-
final ComponentTemplate templateToCompareToExisting = new ComponentTemplate(
368-
finalTemplate,
369-
template.version(),
370-
template.metadata(),
371-
template.deprecated(),
372-
existing.createdDateMillis().orElse(null),
373-
existing.modifiedDateMillis().orElse(null)
374-
);
375-
if (templateToCompareToExisting.equals(existing)) {
376-
return project;
377-
}
378-
finalComponentTemplate = new ComponentTemplate(
379-
finalTemplate,
380-
template.version(),
381-
template.metadata(),
382-
template.deprecated(),
383-
existing.createdDateMillis().orElse(null),
384-
now
385-
);
386-
}
355+
final Long now = instantSource.instant().toEpochMilli();
356+
final Long createdDateMillis = existingTemplate == null ? now : existingTemplate.createdDateMillis().orElse(null);
357+
final ComponentTemplate finalComponentTemplate = new ComponentTemplate(
358+
template.template(),
359+
template.version(),
360+
template.metadata(),
361+
template.deprecated(),
362+
createdDateMillis,
363+
now
364+
);
387365

388-
validateTemplate(finalSettings, wrappedMappings, indicesService);
366+
// These two validation checks are only scoped to the component template itself (and don't depend on any other entities in the
367+
// cluster state) and could thus be done in the transport action. However, since we're parsing mappings here, we shouldn't be doing
368+
// it directly on the transport thread. Instead, we should fork to a different threadpool (management/generic).
369+
validateTemplate(finalComponentTemplate.template().settings(), finalComponentTemplate.template().mappings(), indicesService);
389370
validate(name, finalComponentTemplate.template(), List.of(), null);
390371

391372
ProjectMetadata projectWithComponentTemplateAdded = ProjectMetadata.builder(project).put(name, finalComponentTemplate).build();
392373
// Validate all composable index templates that use this component template
393374
if (templatesUsingComponent.isEmpty() == false) {
394-
Exception validationFailure = null;
375+
IllegalArgumentException validationFailure = null;
395376
for (Map.Entry<String, ComposableIndexTemplate> entry : templatesUsingComponent.entrySet()) {
396377
final String composableTemplateName = entry.getKey();
397378
final ComposableIndexTemplate composableTemplate = entry.getValue();
@@ -425,10 +406,42 @@ public ProjectMetadata addComponentTemplate(
425406
.addWarningHeaderIfDataRetentionNotEffective(globalRetentionSettings.get(false), false);
426407
}
427408

428-
logger.info("{} component template [{}]", existing == null ? "adding" : "updating", name);
409+
logger.info("{} component template [{}]", existingTemplate == null ? "adding" : "updating", name);
429410
return projectWithComponentTemplateAdded;
430411
}
431412

413+
/**
414+
* Normalize the given component template by trying to normalize settings and wrapping mappings if necessary. Returns the same instance
415+
* if nothing needs to be done.
416+
*/
417+
public ComponentTemplate normalizeComponentTemplate(final ComponentTemplate componentTemplate) throws IOException {
418+
Template template = componentTemplate.template();
419+
// Normalize the index settings if necessary
420+
Settings prefixedSettings = null;
421+
if (template.settings() != null) {
422+
prefixedSettings = template.settings().maybeNormalizePrefix(IndexMetadata.INDEX_SETTING_PREFIX);
423+
}
424+
// TODO: theoretically, we could avoid parsing the mappings once by combining this wrapping with the mapping validation later on,
425+
// but that refactoring will be non-trivial as we currently don't seem to have methods available to merge already-parsed mappings;
426+
// we only allow merging mappings from CompressedXContent.
427+
CompressedXContent wrappedMappings = MetadataIndexTemplateService.wrapMappingsIfNecessary(template.mappings(), xContentRegistry);
428+
429+
// No need to build a new component template if we didn't change anything.
430+
// We can check for reference equality since `maybeNormalizePrefix` and `wrapMappingsIfNecessary` return the same instance if
431+
// nothing needs to be done.
432+
if (prefixedSettings == template.settings() && wrappedMappings == template.mappings()) {
433+
return componentTemplate;
434+
}
435+
return new ComponentTemplate(
436+
Template.builder(template).settings(prefixedSettings).mappings(wrappedMappings).build(),
437+
componentTemplate.version(),
438+
componentTemplate.metadata(),
439+
componentTemplate.deprecated(),
440+
componentTemplate.createdDateMillis().orElse(null),
441+
componentTemplate.modifiedDateMillis().orElse(null)
442+
);
443+
}
444+
432445
/**
433446
* Mappings in templates don't have to include <code>_doc</code>, so update the mappings to include this single type if necessary
434447
*
@@ -2048,7 +2061,7 @@ private static void validateCompositeTemplate(
20482061
}
20492062

20502063
public static void validateTemplate(Settings validateSettings, CompressedXContent mappings, IndicesService indicesService)
2051-
throws Exception {
2064+
throws IOException {
20522065
// Hard to validate settings if they're non-existent, so used empty ones if none were provided
20532066
Settings settings = validateSettings;
20542067
if (settings == null) {

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -898,6 +898,19 @@ public Settings merge(Settings newSettings) {
898898
return builder.build();
899899
}
900900

901+
/**
902+
* Checks if all settings start with the specified prefix and renames any that do not. Returns the current instance if nothing needs
903+
* to be done. See {@link Builder#normalizePrefix(String)} for more info.
904+
*/
905+
public Settings maybeNormalizePrefix(String prefix) {
906+
for (String key : settings.keySet()) {
907+
if (key.startsWith(prefix) == false && key.endsWith("*") == false) {
908+
return builder().put(this).normalizePrefix(prefix).build();
909+
}
910+
}
911+
return this;
912+
}
913+
901914
/**
902915
* A builder allowing to put different settings and then {@link #build()} an immutable
903916
* settings implementation. Use {@link Settings#builder()} in order to

‎server/src/main/java/org/elasticsearch/node/NodeConstruction.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1096,7 +1096,7 @@ public Map<String, String> queryFields() {
10961096
clusterService,
10971097
rerouteService,
10981098
buildReservedClusterStateHandlers(reservedStateHandlerProviders, settingsModule),
1099-
buildReservedProjectStateHandlers(reservedStateHandlerProviders, settingsModule, metadataIndexTemplateService),
1099+
buildReservedProjectStateHandlers(reservedStateHandlerProviders, metadataIndexTemplateService),
11001100
pluginsService.loadSingletonServiceProvider(RestExtension.class, RestExtension::allowAll),
11011101
incrementalBulkService,
11021102
projectResolver
@@ -1697,12 +1697,11 @@ private List<ReservedClusterStateHandler<?>> buildReservedClusterStateHandlers(
16971697

16981698
private List<ReservedProjectStateHandler<?>> buildReservedProjectStateHandlers(
16991699
List<? extends ReservedStateHandlerProvider> handlers,
1700-
SettingsModule settingsModule,
17011700
MetadataIndexTemplateService templateService
17021701
) {
17031702
List<ReservedProjectStateHandler<?>> reservedStateHandlers = new ArrayList<>();
17041703

1705-
reservedStateHandlers.add(new ReservedComposableIndexTemplateAction(templateService, settingsModule.getIndexScopedSettings()));
1704+
reservedStateHandlers.add(new ReservedComposableIndexTemplateAction(templateService));
17061705

17071706
// add all reserved state handlers from plugins
17081707
handlers.forEach(h -> reservedStateHandlers.addAll(h.projectHandlers()));

0 commit comments

Comments
 (0)