Skip to content

Commit 8ff5ac8

Browse files
authored
DeprecationInfoAction refactoring (#121181) (#121638)
This refactoring was motivated by the following issues with the current state of the code: - The `TransformDeprecationChecker` is listed as plugin checker, but later we remove is from the `plugin_settings` and add it to the `cluster_settings`. This made me consider that the checker might be dealing with transform deprecation warnings but if they are listed under the `cliuster_settings`, it fits better to be part of `ClusterDeprecationChecker`. - The `DeprecationInfo` is a data class, but it has a method `from` which constructs an `DeprecationInfo.Response` instance. However, this is not a simple factory class but it actually runs all the checks and it also tries to assert that it is not executed on a transport thread. Considering this, I thought it might fit better to the `TransportDeprecationInfoAction`, this way all the logic is in one place and all the checkers are wired and used in the same class. - Constructing the node settings deprecation issues requires to merge the deprecation warnings of the individual nodes. We considered bringing together the execution of the remote request and the construction of the response in a new class called `NodeDeprecationChecker` that resembles the patterns of the other Checker classes. - Reinstated the `PLUGIN_CHECKERS` even if we have only one check, so other developers can easier add their plugin checks. - Finally, we noticed that the way we synthesise the remote requests is difficult to read and maintain because each call is nested under the previous one. We propose in this PR a different pattern that uses the `RefCountingListener` to combine the different remote calls and store their results in a container class named `PrecomputedData` - **Bonus**: Removed the `LegacyIndexTemplateDeprecationChecker.java` which was not used.
1 parent 8f79947 commit 8ff5ac8

27 files changed

+1185
-1151
lines changed

qa/rolling-upgrade/src/javaRestTest/java/org/elasticsearch/upgrades/SourceModeRollingUpgradeIT.java

+5-16
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818
import java.util.List;
1919
import java.util.Map;
2020

21-
import static org.hamcrest.Matchers.containsString;
2221
import static org.hamcrest.Matchers.equalTo;
2322

2423
public class SourceModeRollingUpgradeIT extends AbstractRollingUpgradeTestCase {
@@ -83,20 +82,10 @@ public void testConfigureStoredSourceWhenIndexIsCreatedLegacy() throws IOExcepti
8382
private void assertDeprecationWarningForTemplate(String templateName) throws IOException {
8483
var request = new Request("GET", "/_migration/deprecations");
8584
var response = entityAsMap(client().performRequest(request));
86-
if (response.containsKey("templates")) {
87-
// Check the newer version of the deprecation API that contains the templates section
88-
Map<?, ?> issuesByTemplate = (Map<?, ?>) response.get("templates");
89-
assertThat(issuesByTemplate.containsKey(templateName), equalTo(true));
90-
var templateIssues = (List<?>) issuesByTemplate.get(templateName);
91-
assertThat(((Map<?, ?>) templateIssues.getFirst()).get("message"), equalTo(SourceFieldMapper.DEPRECATION_WARNING));
92-
} else {
93-
// Bwc version with 8.18 until https://github.com/elastic/elasticsearch/pull/120505/ gets backported, clean up after backport
94-
var nodeSettings = (Map<?, ?>) ((List<?>) response.get("node_settings")).getFirst();
95-
assertThat(nodeSettings.get("message"), equalTo(SourceFieldMapper.DEPRECATION_WARNING));
96-
assertThat(
97-
(String) nodeSettings.get("details"),
98-
containsString(SourceFieldMapper.DEPRECATION_WARNING + " Affected component templates: [" + templateName + "]")
99-
);
100-
}
85+
assertThat(response.containsKey("templates"), equalTo(true));
86+
Map<?, ?> issuesByTemplate = (Map<?, ?>) response.get("templates");
87+
assertThat(issuesByTemplate.containsKey(templateName), equalTo(true));
88+
var templateIssues = (List<?>) issuesByTemplate.get(templateName);
89+
assertThat(((Map<?, ?>) templateIssues.getFirst()).get("message"), equalTo(SourceFieldMapper.DEPRECATION_WARNING));
10190
}
10291
}

x-pack/plugin/deprecation/src/main/java/module-info.java

+1
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
requires org.apache.logging.log4j;
1414
requires org.apache.logging.log4j.core;
1515
requires log4j2.ecs.layout;
16+
requires org.apache.lucene.core;
1617

1718
exports org.elasticsearch.xpack.deprecation to org.elasticsearch.server;
1819
exports org.elasticsearch.xpack.deprecation.logging to org.elasticsearch.server;
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License
4+
* 2.0; you may not use this file except in compliance with the Elastic License
5+
* 2.0.
6+
*/
7+
8+
package org.elasticsearch.xpack.deprecation;
9+
10+
import org.apache.logging.log4j.LogManager;
11+
import org.apache.logging.log4j.Logger;
12+
import org.elasticsearch.cluster.ClusterState;
13+
import org.elasticsearch.common.TriConsumer;
14+
import org.elasticsearch.xcontent.NamedXContentRegistry;
15+
import org.elasticsearch.xpack.core.deprecation.DeprecationIssue;
16+
import org.elasticsearch.xpack.core.transform.transforms.TransformConfig;
17+
18+
import java.io.IOException;
19+
import java.util.ArrayList;
20+
import java.util.List;
21+
22+
/**
23+
* Cluster-specific deprecation checks, this is used to populate the {@code cluster_settings} field
24+
*/
25+
public class ClusterDeprecationChecker {
26+
27+
private static final Logger logger = LogManager.getLogger(ClusterDeprecationChecker.class);
28+
private final List<TriConsumer<ClusterState, List<TransformConfig>, List<DeprecationIssue>>> CHECKS = List.of(
29+
this::checkTransformSettings
30+
);
31+
private final NamedXContentRegistry xContentRegistry;
32+
33+
ClusterDeprecationChecker(NamedXContentRegistry xContentRegistry) {
34+
this.xContentRegistry = xContentRegistry;
35+
}
36+
37+
public List<DeprecationIssue> check(ClusterState clusterState, List<TransformConfig> transformConfigs) {
38+
List<DeprecationIssue> allIssues = new ArrayList<>();
39+
CHECKS.forEach(check -> check.apply(clusterState, transformConfigs, allIssues));
40+
return allIssues;
41+
}
42+
43+
private void checkTransformSettings(
44+
ClusterState clusterState,
45+
List<TransformConfig> transformConfigs,
46+
List<DeprecationIssue> allIssues
47+
) {
48+
for (var config : transformConfigs) {
49+
try {
50+
allIssues.addAll(config.checkForDeprecations(xContentRegistry));
51+
} catch (IOException e) {
52+
logger.warn("failed to check transformation settings for '" + config.getId() + "'", e);
53+
}
54+
}
55+
}
56+
}

x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/DataStreamDeprecationChecker.java

+20-3
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,13 @@
1818
import java.util.HashMap;
1919
import java.util.List;
2020
import java.util.Map;
21+
import java.util.Objects;
2122
import java.util.Set;
2223
import java.util.function.BiFunction;
2324
import java.util.stream.Collectors;
2425

2526
import static java.util.Map.entry;
2627
import static java.util.Map.ofEntries;
27-
import static org.elasticsearch.xpack.deprecation.DeprecationInfoAction.filterChecks;
2828

2929
/**
3030
* Checks the data streams for deprecation warnings.
@@ -44,10 +44,24 @@ public DataStreamDeprecationChecker(IndexNameExpressionResolver indexNameExpress
4444

4545
/**
4646
* @param clusterState The cluster state provided for the checker
47+
* @param request not used yet in these checks
48+
* @param precomputedData not used yet in these checks
4749
* @return the name of the data streams that have violated the checks with their respective warnings.
4850
*/
4951
@Override
50-
public Map<String, List<DeprecationIssue>> check(ClusterState clusterState, DeprecationInfoAction.Request request) {
52+
public Map<String, List<DeprecationIssue>> check(
53+
ClusterState clusterState,
54+
DeprecationInfoAction.Request request,
55+
TransportDeprecationInfoAction.PrecomputedData precomputedData
56+
) {
57+
return check(clusterState);
58+
}
59+
60+
/**
61+
* @param clusterState The cluster state provided for the checker
62+
* @return the name of the data streams that have violated the checks with their respective warnings.
63+
*/
64+
public Map<String, List<DeprecationIssue>> check(ClusterState clusterState) {
5165
List<String> dataStreamNames = indexNameExpressionResolver.dataStreamNames(
5266
clusterState,
5367
IndicesOptions.LENIENT_EXPAND_OPEN_CLOSED_HIDDEN
@@ -58,7 +72,10 @@ public Map<String, List<DeprecationIssue>> check(ClusterState clusterState, Depr
5872
Map<String, List<DeprecationIssue>> dataStreamIssues = new HashMap<>();
5973
for (String dataStreamName : dataStreamNames) {
6074
DataStream dataStream = clusterState.metadata().dataStreams().get(dataStreamName);
61-
List<DeprecationIssue> issuesForSingleDataStream = filterChecks(DATA_STREAM_CHECKS, c -> c.apply(dataStream, clusterState));
75+
List<DeprecationIssue> issuesForSingleDataStream = DATA_STREAM_CHECKS.stream()
76+
.map(c -> c.apply(dataStream, clusterState))
77+
.filter(Objects::nonNull)
78+
.toList();
6279
if (issuesForSingleDataStream.isEmpty() == false) {
6380
dataStreamIssues.put(dataStreamName, issuesForSingleDataStream);
6481
}

x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/Deprecation.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@
3333
import java.util.function.Predicate;
3434
import java.util.function.Supplier;
3535

36-
import static org.elasticsearch.xpack.deprecation.DeprecationChecks.SKIP_DEPRECATIONS_SETTING;
36+
import static org.elasticsearch.xpack.deprecation.TransportDeprecationInfoAction.SKIP_DEPRECATIONS_SETTING;
3737
import static org.elasticsearch.xpack.deprecation.logging.DeprecationIndexingComponent.DEPRECATION_INDEXING_FLUSH_INTERVAL;
3838

3939
/**

x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/DeprecationChecks.java

-107
This file was deleted.

0 commit comments

Comments
 (0)