-
Notifications
You must be signed in to change notification settings - Fork 24.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add deprecation info and warnings for an empty TIER_PREFERENCE #79305
Changes from 6 commits
3f800c4
7cfc8f1
b3e7ee9
cfbd76e
76090df
675fd52
89c2527
14f1984
2a16334
2f5d9cd
0812b95
0065a23
cc28dab
a963286
156775d
2b11293
c10b720
ac950dc
902f998
9a25c04
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,7 @@ | |
import org.elasticsearch.cluster.routing.RoutingTable; | ||
import org.elasticsearch.cluster.routing.allocation.AllocationService; | ||
import org.elasticsearch.cluster.routing.allocation.DataTier; | ||
import org.elasticsearch.cluster.routing.allocation.DataTierTests; | ||
import org.elasticsearch.cluster.routing.allocation.allocator.BalancedShardsAllocator; | ||
import org.elasticsearch.cluster.routing.allocation.decider.AllocationDeciders; | ||
import org.elasticsearch.cluster.routing.allocation.decider.MaxRetryAllocationDecider; | ||
|
@@ -1102,6 +1103,16 @@ null, Settings.EMPTY, IndexScopedSettings.DEFAULT_SCOPED_SETTINGS, randomShardLi | |
"as it offers superior or equivalent performance to [simplefs]."); | ||
} | ||
|
||
public void testDeprecateNoTierPreference() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if you should bake this into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair point, I think I've accomplished what you're getting at via 2b11293. |
||
request = new CreateIndexClusterStateUpdateRequest("create index", "test", "test"); | ||
ClusterState clusterState = DataTierTests.clusterStateWithoutAllDataRoles(); | ||
aggregateIndexSettings(clusterState, request, Settings.EMPTY, | ||
null, Settings.EMPTY, IndexScopedSettings.DEFAULT_SCOPED_SETTINGS, randomShardLimitService(), | ||
Collections.emptySet(), false); | ||
assertWarnings("[test] creating index with an empty [index.routing.allocation.include._tier_preference] setting, " + | ||
"and TODO TODO TODO"); | ||
} | ||
|
||
private IndexTemplateMetadata addMatchingTemplate(Consumer<IndexTemplateMetadata.Builder> configurator) { | ||
IndexTemplateMetadata.Builder builder = templateMetadataBuilder("template1", "te*"); | ||
configurator.accept(builder); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ | |
package org.elasticsearch.cluster.routing.allocation; | ||
|
||
import org.elasticsearch.Version; | ||
import org.elasticsearch.cluster.ClusterState; | ||
import org.elasticsearch.cluster.node.DiscoveryNode; | ||
import org.elasticsearch.cluster.node.DiscoveryNodeRole; | ||
import org.elasticsearch.cluster.node.DiscoveryNodes; | ||
|
@@ -27,7 +28,9 @@ | |
import java.util.Map; | ||
import java.util.Set; | ||
import java.util.concurrent.atomic.AtomicInteger; | ||
import java.util.stream.Collectors; | ||
|
||
import static org.elasticsearch.cluster.routing.allocation.DataTier.ALL_DATA_TIERS; | ||
import static org.elasticsearch.cluster.routing.allocation.DataTier.DATA_COLD; | ||
import static org.elasticsearch.cluster.routing.allocation.DataTier.DATA_HOT; | ||
import static org.elasticsearch.cluster.routing.allocation.DataTier.DATA_WARM; | ||
|
@@ -139,6 +142,35 @@ public void testGetPreferredTiersConfiguration() { | |
assertThat(exception.getMessage(), is("invalid data tier [no_tier]")); | ||
} | ||
|
||
public static ClusterState clusterStateWithoutAllDataRoles() { | ||
Set<DiscoveryNodeRole> allDataRoles = new HashSet<>(DiscoveryNodeRole.BUILT_IN_ROLES).stream() | ||
.filter(role -> ALL_DATA_TIERS.contains(role.roleName())).collect(Collectors.toSet()); | ||
|
||
DiscoveryNodes.Builder discoBuilder = DiscoveryNodes.builder(); | ||
List<DiscoveryNode> nodesList = org.elasticsearch.core.List.of( | ||
newNode(0, org.elasticsearch.core.Map.of(), org.elasticsearch.core.Set.of(DiscoveryNodeRole.MASTER_ROLE)), | ||
newNode(1, org.elasticsearch.core.Map.of(), org.elasticsearch.core.Set.of(DiscoveryNodeRole.DATA_ROLE)), | ||
newNode(2, org.elasticsearch.core.Map.of(), allDataRoles), | ||
newNode(3, org.elasticsearch.core.Map.of(), org.elasticsearch.core.Set.of(DiscoveryNodeRole.DATA_FROZEN_NODE_ROLE)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps add one with nearly all roles, something like:
You can then validate that you get the node out but no need to validate the roles (just the id/name). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, thanks for the suggestion, 0065a23 |
||
); | ||
for (DiscoveryNode node : nodesList) { | ||
discoBuilder = discoBuilder.add(node); | ||
} | ||
discoBuilder.localNodeId(randomFrom(nodesList).getId()); | ||
discoBuilder.masterNodeId(randomFrom(nodesList).getId()); | ||
|
||
return ClusterState.builder(ClusterState.EMPTY_STATE).nodes(discoBuilder.build()).build(); | ||
} | ||
|
||
public void testDataNodesWithoutAllDataRoles() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: can we move this test method before the helper method above? That feels like a more natural reading order. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure thing, 2a16334 |
||
ClusterState clusterState = clusterStateWithoutAllDataRoles(); | ||
Set<DiscoveryNode> nodes = DataTier.dataNodesWithoutAllDataRoles(clusterState); | ||
assertEquals(1, nodes.size()); | ||
DiscoveryNode node = nodes.iterator().next(); | ||
assertEquals("name_3", node.getName()); | ||
assertEquals(org.elasticsearch.core.Set.of(DiscoveryNodeRole.DATA_FROZEN_NODE_ROLE), node.getRoles()); | ||
} | ||
|
||
private static DiscoveryNodes buildDiscoveryNodes() { | ||
int numNodes = randomIntBetween(3, 15); | ||
DiscoveryNodes.Builder discoBuilder = DiscoveryNodes.builder(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,9 +13,9 @@ | |
import org.elasticsearch.common.settings.Setting; | ||
import org.elasticsearch.common.settings.Settings; | ||
import org.elasticsearch.license.XPackLicenseState; | ||
import org.elasticsearch.xpack.core.deprecation.DeprecationIssue; | ||
import org.elasticsearch.transport.TransportService; | ||
import org.elasticsearch.xpack.core.XPackSettings; | ||
import org.elasticsearch.xpack.core.deprecation.DeprecationIssue; | ||
|
||
import java.util.Arrays; | ||
import java.util.Collections; | ||
|
@@ -138,24 +138,25 @@ private DeprecationChecks() { | |
).collect(Collectors.toList()); | ||
} | ||
|
||
static List<Function<IndexMetadata, DeprecationIssue>> INDEX_SETTINGS_CHECKS = | ||
static List<IndexDeprecationCheck<ClusterState, IndexMetadata, DeprecationIssue>> INDEX_SETTINGS_CHECKS = | ||
Collections.unmodifiableList(Arrays.asList( | ||
IndexDeprecationChecks::oldIndicesCheck, | ||
IndexDeprecationChecks::tooManyFieldsCheck, | ||
IndexDeprecationChecks::chainedMultiFieldsCheck, | ||
IndexDeprecationChecks::deprecatedDateTimeFormat, | ||
IndexDeprecationChecks::translogRetentionSettingCheck, | ||
IndexDeprecationChecks::fieldNamesDisabledCheck, | ||
IndexDeprecationChecks::checkIndexDataPath, | ||
IndexDeprecationChecks::indexingSlowLogLevelSettingCheck, | ||
IndexDeprecationChecks::searchSlowLogLevelSettingCheck, | ||
IndexDeprecationChecks::storeTypeSettingCheck, | ||
IndexDeprecationChecks::checkIndexRoutingRequireSetting, | ||
IndexDeprecationChecks::checkIndexRoutingIncludeSetting, | ||
IndexDeprecationChecks::checkIndexRoutingExcludeSetting, | ||
IndexDeprecationChecks::checkIndexMatrixFiltersSetting, | ||
IndexDeprecationChecks::checkGeoShapeMappings, | ||
IndexDeprecationChecks::frozenIndexSettingCheck | ||
(clusterState, indexMetadata) -> IndexDeprecationChecks.oldIndicesCheck(indexMetadata), | ||
(clusterState, indexMetadata) -> IndexDeprecationChecks.tooManyFieldsCheck(indexMetadata), | ||
(clusterState, indexMetadata) -> IndexDeprecationChecks.chainedMultiFieldsCheck(indexMetadata), | ||
(clusterState, indexMetadata) -> IndexDeprecationChecks.deprecatedDateTimeFormat(indexMetadata), | ||
(clusterState, indexMetadata) -> IndexDeprecationChecks.translogRetentionSettingCheck(indexMetadata), | ||
(clusterState, indexMetadata) -> IndexDeprecationChecks.fieldNamesDisabledCheck(indexMetadata), | ||
(clusterState, indexMetadata) -> IndexDeprecationChecks.checkIndexDataPath(indexMetadata), | ||
(clusterState, indexMetadata) -> IndexDeprecationChecks.indexingSlowLogLevelSettingCheck(indexMetadata), | ||
(clusterState, indexMetadata) -> IndexDeprecationChecks.searchSlowLogLevelSettingCheck(indexMetadata), | ||
(clusterState, indexMetadata) -> IndexDeprecationChecks.storeTypeSettingCheck(indexMetadata), | ||
(clusterState, indexMetadata) -> IndexDeprecationChecks.checkIndexRoutingRequireSetting(indexMetadata), | ||
(clusterState, indexMetadata) -> IndexDeprecationChecks.checkIndexRoutingIncludeSetting(indexMetadata), | ||
(clusterState, indexMetadata) -> IndexDeprecationChecks.checkIndexRoutingExcludeSetting(indexMetadata), | ||
(clusterState, indexMetadata) -> IndexDeprecationChecks.checkIndexMatrixFiltersSetting(indexMetadata), | ||
(clusterState, indexMetadata) -> IndexDeprecationChecks.checkGeoShapeMappings(indexMetadata), | ||
(clusterState, indexMetadata) -> IndexDeprecationChecks.frozenIndexSettingCheck(indexMetadata), | ||
IndexDeprecationChecks::emptyDataTierPreferenceCheck | ||
)); | ||
|
||
/** | ||
|
@@ -174,4 +175,9 @@ static <T> List<DeprecationIssue> filterChecks(List<T> checks, Function<T, Depre | |
public interface NodeDeprecationCheck<A, B, C, D, R> { | ||
R apply(A first, B second, C third, D fourth); | ||
} | ||
|
||
@FunctionalInterface | ||
public interface IndexDeprecationCheck<A, B, R> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure I follow why it is beneficial to have this be generic, can we not just make There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ohhhhh, yup, I see it now. I tried to mirror what was in place for the |
||
R apply(A first, B second); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,20 +8,22 @@ | |
|
||
|
||
import org.elasticsearch.Version; | ||
import org.elasticsearch.cluster.ClusterState; | ||
import org.elasticsearch.cluster.metadata.IndexMetadata; | ||
import org.elasticsearch.cluster.metadata.MappingMetadata; | ||
import org.elasticsearch.cluster.routing.allocation.DataTier; | ||
import org.elasticsearch.common.joda.JodaDeprecationPatterns; | ||
import org.elasticsearch.common.settings.Setting; | ||
import org.elasticsearch.common.settings.Settings; | ||
import org.elasticsearch.index.IndexModule; | ||
import org.elasticsearch.index.IndexSettings; | ||
import org.elasticsearch.index.IndexingSlowLog; | ||
import org.elasticsearch.index.mapper.GeoShapeFieldMapper; | ||
import org.elasticsearch.index.engine.frozen.FrozenEngine; | ||
import org.elasticsearch.xpack.core.deprecation.DeprecationIssue; | ||
import org.elasticsearch.index.SearchSlowLog; | ||
import org.elasticsearch.index.SlowLogLevel; | ||
import org.elasticsearch.index.engine.frozen.FrozenEngine; | ||
import org.elasticsearch.index.mapper.GeoShapeFieldMapper; | ||
import org.elasticsearch.search.SearchModule; | ||
import org.elasticsearch.xpack.core.deprecation.DeprecationIssue; | ||
|
||
import java.util.ArrayList; | ||
import java.util.Collections; | ||
|
@@ -438,4 +440,17 @@ static DeprecationIssue frozenIndexSettingCheck(IndexMetadata indexMetadata) { | |
} | ||
return null; | ||
} | ||
|
||
static DeprecationIssue emptyDataTierPreferenceCheck(ClusterState clusterState, IndexMetadata indexMetadata) { | ||
if (DataTier.dataNodesWithoutAllDataRoles(clusterState).isEmpty() == false) { | ||
final List<String> tierPreference = DataTier.parseTierList(DataTier.TIER_PREFERENCE_SETTING.get(indexMetadata.getSettings())); | ||
if (tierPreference.isEmpty()) { | ||
return new DeprecationIssue(DeprecationIssue.Level.CRITICAL, | ||
"some message here", | ||
"https://www.elastic.co/some/url/here.html", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should ensure this URL points to a good page describing the mandatory tier preference migration. I think just getting some page link to tier preference in for now is fine though, we can then iterate on the docs for this and update the link. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 902f998, good suggestion, I've added a link to the data tiers page for now, because there's some discussion of the setting there (incidentally, some discussion that will need to get updated, in light of the changes from this and the other related PRs). |
||
"some details here", false, null); | ||
} | ||
} | ||
return null; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
INDICES
is fine here, though we could also considerSETTINGS
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍, 14f1984