From feb7268e3bf9140dd5d0af3374f15210373df707 Mon Sep 17 00:00:00 2001 From: Martijn van Groningen Date: Tue, 12 Oct 2021 18:02:15 +0200 Subject: [PATCH 1/8] Reuse previous indices lookup in IndexMetadataUpdater#applyChanges(...) The IndexMetadataUpdater#applyChanges(...) method builds a new metadata instance, but only primary term or insync allocations may be updated. No new indices, aliases or data streams are added, so re-building indices lookup is not necessary. In clusters with many indices the cost of building indices lookup is non-neglectable and should be avoided in this case. Closes #78980 --- .../cluster/metadata/Metadata.java | 23 ++++++++++++++++--- .../allocation/IndexMetadataUpdater.java | 2 +- .../cluster/ClusterChangedEventTests.java | 2 +- .../allocation/ThrottlingAllocationTests.java | 2 +- .../DatafeedConfigAutoUpdaterTests.java | 2 +- ...stractJobPersistentTasksExecutorTests.java | 2 +- .../security/test/SecurityTestUtils.java | 2 +- ...TransformPersistentTasksExecutorTests.java | 2 +- 8 files changed, 27 insertions(+), 10 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java b/server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java index c0b5ce151fd13..7b6ef62aee209 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java @@ -1020,7 +1020,11 @@ public static Builder builder() { } public static Builder builder(Metadata metadata) { - return new Builder(metadata); + return builder(metadata, false); + } + + public static Builder builder(Metadata metadata, boolean reuseIndicesLookup) { + return new Builder(metadata, reuseIndicesLookup); } public static class Builder { @@ -1037,6 +1041,7 @@ public static class Builder { private final ImmutableOpenMap.Builder indices; private final ImmutableOpenMap.Builder templates; private final ImmutableOpenMap.Builder customs; + private final SortedMap previousIndicesLookup; public Builder() { clusterUUID = UNKNOWN_CLUSTER_UUID; @@ -1044,9 +1049,10 @@ public Builder() { templates = ImmutableOpenMap.builder(); customs = ImmutableOpenMap.builder(); indexGraveyard(IndexGraveyard.builder().build()); // create new empty index graveyard to initialize + previousIndicesLookup = null; } - public Builder(Metadata metadata) { + Builder(Metadata metadata, boolean reuseIndicesLookup) { this.clusterUUID = metadata.clusterUUID; this.clusterUUIDCommitted = metadata.clusterUUIDCommitted; this.coordinationMetadata = metadata.coordinationMetadata; @@ -1057,6 +1063,11 @@ public Builder(Metadata metadata) { this.indices = ImmutableOpenMap.builder(metadata.indices); this.templates = ImmutableOpenMap.builder(metadata.templates); this.customs = ImmutableOpenMap.builder(metadata.customs); + if (reuseIndicesLookup) { + previousIndicesLookup = metadata.getIndicesLookup(); + } else { + previousIndicesLookup = null; + } } public Builder put(IndexMetadata.Builder indexMetadataBuilder) { @@ -1539,8 +1550,14 @@ public Metadata build(boolean builtIndicesLookupEagerly) { SortedMap indicesLookup; if (builtIndicesLookupEagerly) { - indicesLookup = Collections.unmodifiableSortedMap(buildIndicesLookup(dataStreamMetadata, indices)); + if (previousIndicesLookup != null) { + assert previousIndicesLookup.equals(buildIndicesLookup(dataStreamMetadata, indices)); + indicesLookup = previousIndicesLookup; + } else { + indicesLookup = Collections.unmodifiableSortedMap(buildIndicesLookup(dataStreamMetadata, indices)); + } } else { + assert previousIndicesLookup != null : "previous indicesLookup provided, but builtIndicesLookupEagerly=false"; indicesLookup = null; } diff --git a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/IndexMetadataUpdater.java b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/IndexMetadataUpdater.java index 48fc1ffbe6d3e..bc0da376de98f 100644 --- a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/IndexMetadataUpdater.java +++ b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/IndexMetadataUpdater.java @@ -114,7 +114,7 @@ public Metadata applyChanges(Metadata oldMetadata, RoutingTable newRoutingTable) if (indexMetadataBuilder != null) { if (metadataBuilder == null) { - metadataBuilder = Metadata.builder(oldMetadata); + metadataBuilder = Metadata.builder(oldMetadata, true); } metadataBuilder.put(indexMetadataBuilder); } diff --git a/server/src/test/java/org/elasticsearch/cluster/ClusterChangedEventTests.java b/server/src/test/java/org/elasticsearch/cluster/ClusterChangedEventTests.java index 234a3f8f96e07..f8505cfd26d4c 100644 --- a/server/src/test/java/org/elasticsearch/cluster/ClusterChangedEventTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/ClusterChangedEventTests.java @@ -354,7 +354,7 @@ private static ClusterState createNonInitializedState(final int numNodes, final private static ClusterState nextState(final ClusterState previousState, List customMetadataList) { final ClusterState.Builder builder = ClusterState.builder(previousState); builder.stateUUID(UUIDs.randomBase64UUID()); - Metadata.Builder metadataBuilder = new Metadata.Builder(previousState.metadata()); + Metadata.Builder metadataBuilder = Metadata.builder(previousState.metadata()); for (ObjectObjectCursor customMetadata : previousState.metadata().customs()) { if (customMetadata.value instanceof TestCustomMetadata) { metadataBuilder.removeCustom(customMetadata.key); diff --git a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/ThrottlingAllocationTests.java b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/ThrottlingAllocationTests.java index 81e387473f694..c60d00558cf17 100644 --- a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/ThrottlingAllocationTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/ThrottlingAllocationTests.java @@ -334,7 +334,7 @@ private ClusterState createRecoveryStateAndInitializeAllocations( final TestSnapshotsInfoService snapshotsInfoService ) { DiscoveryNode node1 = newNode("node1"); - Metadata.Builder metadataBuilder = new Metadata.Builder(metadata); + Metadata.Builder metadataBuilder = Metadata.builder(metadata); RoutingTable.Builder routingTableBuilder = RoutingTable.builder(); Snapshot snapshot = new Snapshot("repo", new SnapshotId("snap", "randomId")); Set snapshotIndices = new HashSet<>(); diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/datafeed/DatafeedConfigAutoUpdaterTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/datafeed/DatafeedConfigAutoUpdaterTests.java index 998be8f6181e9..a986cc7ab9ea3 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/datafeed/DatafeedConfigAutoUpdaterTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/datafeed/DatafeedConfigAutoUpdaterTests.java @@ -179,7 +179,7 @@ public void testIsAbleToRun() { final ClusterState clusterState = csBuilder.build(); assertThat(updater.isAbleToRun(clusterState), is(true)); - metadata = new Metadata.Builder(clusterState.metadata()); + metadata = Metadata.builder(clusterState.metadata()); routingTable = new RoutingTable.Builder(clusterState.routingTable()); if (randomBoolean()) { routingTable.remove(MlConfigIndex.indexName()); diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/task/AbstractJobPersistentTasksExecutorTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/task/AbstractJobPersistentTasksExecutorTests.java index dfecf00676514..0030c1ddf41b1 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/task/AbstractJobPersistentTasksExecutorTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/task/AbstractJobPersistentTasksExecutorTests.java @@ -55,7 +55,7 @@ public void testVerifyIndicesPrimaryShardsAreActive() { MlMetaIndex.indexName(), MlConfigIndex.indexName()).size()); - metadata = new Metadata.Builder(cs.metadata()); + metadata = Metadata.builder(cs.metadata()); routingTable = new RoutingTable.Builder(cs.routingTable()); String indexToRemove = randomFrom(resolver.concreteIndexNames(cs, IndicesOptions.lenientExpandOpen(), ".ml-anomalies-shared", diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/test/SecurityTestUtils.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/test/SecurityTestUtils.java index b45d05def523a..9717c93b5ddbe 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/test/SecurityTestUtils.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/test/SecurityTestUtils.java @@ -83,7 +83,7 @@ public static RoutingTable buildIndexRoutingTable(Index index) { */ public static Metadata addAliasToMetadata(Metadata metadata, String indexName) { AliasMetadata aliasMetadata = AliasMetadata.newAliasMetadataBuilder(SECURITY_MAIN_ALIAS).build(); - Metadata.Builder metadataBuilder = new Metadata.Builder(metadata); + Metadata.Builder metadataBuilder = Metadata.builder(metadata); IndexMetadata indexMetadata = metadata.index(indexName); metadataBuilder.put(IndexMetadata.builder(indexMetadata).putAlias(aliasMetadata)); return metadataBuilder.build(); diff --git a/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/transforms/TransformPersistentTasksExecutorTests.java b/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/transforms/TransformPersistentTasksExecutorTests.java index 1df06e3329440..bda9c8b497867 100644 --- a/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/transforms/TransformPersistentTasksExecutorTests.java +++ b/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/transforms/TransformPersistentTasksExecutorTests.java @@ -194,7 +194,7 @@ public void testVerifyIndicesPrimaryShardsAreActive() { TransformPersistentTasksExecutor.verifyIndicesPrimaryShardsAreActive(cs, TestIndexNameExpressionResolver.newInstance()).size() ); - metadata = new Metadata.Builder(cs.metadata()); + metadata = Metadata.builder(cs.metadata()); routingTable = new RoutingTable.Builder(cs.routingTable()); String indexToRemove = TransformInternalIndexConstants.LATEST_INDEX_NAME; if (randomBoolean()) { From dc92929d8debebe2aeb1eadf9ad5ca6720d08f77 Mon Sep 17 00:00:00 2001 From: Martijn van Groningen Date: Wed, 13 Oct 2021 10:45:42 +0200 Subject: [PATCH 2/8] iter --- .../cluster/metadata/Metadata.java | 82 ++++++++++++++----- .../allocation/IndexMetadataUpdater.java | 2 +- 2 files changed, 61 insertions(+), 23 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java b/server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java index 7b6ef62aee209..072f8a8de1922 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java @@ -1020,11 +1020,7 @@ public static Builder builder() { } public static Builder builder(Metadata metadata) { - return builder(metadata, false); - } - - public static Builder builder(Metadata metadata, boolean reuseIndicesLookup) { - return new Builder(metadata, reuseIndicesLookup); + return new Builder(metadata); } public static class Builder { @@ -1041,7 +1037,8 @@ public static class Builder { private final ImmutableOpenMap.Builder indices; private final ImmutableOpenMap.Builder templates; private final ImmutableOpenMap.Builder customs; - private final SortedMap previousIndicesLookup; + + private SortedMap previousIndicesLookup; public Builder() { clusterUUID = UNKNOWN_CLUSTER_UUID; @@ -1052,7 +1049,7 @@ public Builder() { previousIndicesLookup = null; } - Builder(Metadata metadata, boolean reuseIndicesLookup) { + Builder(Metadata metadata) { this.clusterUUID = metadata.clusterUUID; this.clusterUUIDCommitted = metadata.clusterUUIDCommitted; this.coordinationMetadata = metadata.coordinationMetadata; @@ -1063,18 +1060,17 @@ public Builder() { this.indices = ImmutableOpenMap.builder(metadata.indices); this.templates = ImmutableOpenMap.builder(metadata.templates); this.customs = ImmutableOpenMap.builder(metadata.customs); - if (reuseIndicesLookup) { - previousIndicesLookup = metadata.getIndicesLookup(); - } else { - previousIndicesLookup = null; - } + previousIndicesLookup = metadata.getIndicesLookup(); } public Builder put(IndexMetadata.Builder indexMetadataBuilder) { // we know its a new one, increment the version and store indexMetadataBuilder.version(indexMetadataBuilder.version() + 1); IndexMetadata indexMetadata = indexMetadataBuilder.build(); - indices.put(indexMetadata.getIndex().getName(), indexMetadata); + IndexMetadata previous = indices.put(indexMetadata.getIndex().getName(), indexMetadata); + if (unsetPreviousIndicesLookup(previous, indexMetadata)) { + previousIndicesLookup = null; + } return this; } @@ -1086,10 +1082,33 @@ public Builder put(IndexMetadata indexMetadata, boolean incrementVersion) { if (incrementVersion) { indexMetadata = IndexMetadata.builder(indexMetadata).version(indexMetadata.getVersion() + 1).build(); } - indices.put(indexMetadata.getIndex().getName(), indexMetadata); + IndexMetadata previous = indices.put(indexMetadata.getIndex().getName(), indexMetadata); + if (unsetPreviousIndicesLookup(previous, indexMetadata)) { + previousIndicesLookup = null; + } return this; } + boolean unsetPreviousIndicesLookup(IndexMetadata previous, IndexMetadata current) { + if (previous == null) { + return true; + } + + if (previous.getAliases().equals(current.getAliases()) == false) { + return true; + } + + if (previous.isHidden() != current.isHidden()) { + return true; + } + + if (previous.getState() != current.getState()) { + return true; + } + + return false; + } + public IndexMetadata get(String index) { return indices.get(index); } @@ -1108,16 +1127,22 @@ public IndexMetadata getSafe(Index index) { } public Builder remove(String index) { + previousIndicesLookup = null; + indices.remove(index); return this; } public Builder removeAllIndices() { + previousIndicesLookup = null; + indices.clear(); return this; } public Builder indices(ImmutableOpenMap indices) { + previousIndicesLookup = null; + this.indices.putAll(indices); return this; } @@ -1198,6 +1223,8 @@ public Builder removeIndexTemplate(String name) { } public DataStream dataStream(String dataStreamName) { + previousIndicesLookup = null; + DataStreamMetadata dataStreamMetadata = (DataStreamMetadata) customs.get(DataStreamMetadata.TYPE); if (dataStreamMetadata != null) { return dataStreamMetadata.dataStreams().get(dataStreamName); @@ -1207,11 +1234,15 @@ public DataStream dataStream(String dataStreamName) { } public Builder dataStreams(Map dataStreams, Map dataStreamAliases) { + previousIndicesLookup = null; + this.customs.put(DataStreamMetadata.TYPE, new DataStreamMetadata(dataStreams, dataStreamAliases)); return this; } public Builder put(DataStream dataStream) { + previousIndicesLookup = null; + Objects.requireNonNull(dataStream, "it is invalid to add a null data stream"); Map existingDataStreams = Optional.ofNullable((DataStreamMetadata) this.customs.get(DataStreamMetadata.TYPE)) @@ -1228,6 +1259,8 @@ public Builder put(DataStream dataStream) { } public boolean put(String aliasName, String dataStream, Boolean isWriteDataStream, String filter) { + previousIndicesLookup = null; + Map existingDataStream = Optional.ofNullable((DataStreamMetadata) this.customs.get(DataStreamMetadata.TYPE)) .map(dsmd -> new HashMap<>(dsmd.dataStreams())) @@ -1266,6 +1299,8 @@ public boolean put(String aliasName, String dataStream, Boolean isWriteDataStrea } public Builder removeDataStream(String name) { + previousIndicesLookup = null; + Map existingDataStreams = Optional.ofNullable((DataStreamMetadata) this.customs.get(DataStreamMetadata.TYPE)) .map(dsmd -> new HashMap<>(dsmd.dataStreams())) @@ -1302,6 +1337,8 @@ public Builder removeDataStream(String name) { } public boolean removeDataStreamAlias(String aliasName, String dataStreamName, boolean mustExist) { + previousIndicesLookup = null; + Map dataStreamAliases = Optional.ofNullable((DataStreamMetadata) this.customs.get(DataStreamMetadata.TYPE)) .map(dsmd -> new HashMap<>(dsmd.getDataStreamAliases())) @@ -1549,16 +1586,17 @@ public Metadata build(boolean builtIndicesLookupEagerly) { ImmutableOpenMap indices = this.indices.build(); SortedMap indicesLookup; - if (builtIndicesLookupEagerly) { - if (previousIndicesLookup != null) { - assert previousIndicesLookup.equals(buildIndicesLookup(dataStreamMetadata, indices)); - indicesLookup = previousIndicesLookup; - } else { + if (previousIndicesLookup != null) { + // Can't compare the values, because IndexMetadata uses IndexMetadata, which are allowed to be updated. + // TODO: change IndexAbstraction's getIndices() and getWriteIndex() methods to return String instead of IndexMetadata + assert previousIndicesLookup.keySet().equals(buildIndicesLookup(dataStreamMetadata, indices).keySet()); + indicesLookup = previousIndicesLookup; + } else { + if (builtIndicesLookupEagerly) { indicesLookup = Collections.unmodifiableSortedMap(buildIndicesLookup(dataStreamMetadata, indices)); + } else { + indicesLookup = null; } - } else { - assert previousIndicesLookup != null : "previous indicesLookup provided, but builtIndicesLookupEagerly=false"; - indicesLookup = null; } diff --git a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/IndexMetadataUpdater.java b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/IndexMetadataUpdater.java index bc0da376de98f..48fc1ffbe6d3e 100644 --- a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/IndexMetadataUpdater.java +++ b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/IndexMetadataUpdater.java @@ -114,7 +114,7 @@ public Metadata applyChanges(Metadata oldMetadata, RoutingTable newRoutingTable) if (indexMetadataBuilder != null) { if (metadataBuilder == null) { - metadataBuilder = Metadata.builder(oldMetadata, true); + metadataBuilder = Metadata.builder(oldMetadata); } metadataBuilder.put(indexMetadataBuilder); } From accf90c69d33fa6ec6e945fd118aad9bd71a0e14 Mon Sep 17 00:00:00 2001 From: Martijn van Groningen Date: Fri, 15 Oct 2021 13:36:29 +0200 Subject: [PATCH 3/8] added test --- .../allocation/IndexMetadataUpdater.java | 4 +- .../cluster/service/MasterService.java | 1 + .../cluster/metadata/MetadataTests.java | 99 +++++++++++++++++++ 3 files changed, 103 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/IndexMetadataUpdater.java b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/IndexMetadataUpdater.java index 48fc1ffbe6d3e..8dc6ba0e47031 100644 --- a/server/src/main/java/org/elasticsearch/cluster/routing/allocation/IndexMetadataUpdater.java +++ b/server/src/main/java/org/elasticsearch/cluster/routing/allocation/IndexMetadataUpdater.java @@ -121,7 +121,9 @@ public Metadata applyChanges(Metadata oldMetadata, RoutingTable newRoutingTable) } if (metadataBuilder != null) { - return metadataBuilder.build(); + Metadata newMetadata = metadataBuilder.build(); + assert oldMetadata.getIndicesLookup() == newMetadata.getIndicesLookup(); + return newMetadata; } else { return oldMetadata; } diff --git a/server/src/main/java/org/elasticsearch/cluster/service/MasterService.java b/server/src/main/java/org/elasticsearch/cluster/service/MasterService.java index c1bf8f27ff874..09a0c16eea74f 100644 --- a/server/src/main/java/org/elasticsearch/cluster/service/MasterService.java +++ b/server/src/main/java/org/elasticsearch/cluster/service/MasterService.java @@ -380,6 +380,7 @@ private ClusterState patchVersions(ClusterState previousClusterState, ClusterTas newClusterState = builder.build(); } + assert previousClusterState.metadata().getIndicesLookup() == newClusterState.metadata().getIndicesLookup(); return newClusterState; } diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataTests.java b/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataTests.java index fe56fe4bc5acc..f6a676bedd857 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataTests.java @@ -24,6 +24,7 @@ import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.set.Sets; +import org.elasticsearch.index.IndexSettings; import org.elasticsearch.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentHelper; import org.elasticsearch.xcontent.XContentParser; @@ -57,8 +58,10 @@ import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.greaterThanOrEqualTo; import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.nullValue; +import static org.hamcrest.Matchers.sameInstance; import static org.hamcrest.Matchers.startsWith; public class MetadataTests extends ESTestCase { @@ -1520,6 +1523,102 @@ public void testDataStreamWriteRemoveDataStream() { assertThat(metadata.dataStreamAliases().get("logs-postgres").getDataStreams(), containsInAnyOrder("logs-postgres-replicated")); } + public void testReuseIndicesLookup() { + String indexName = "my-index"; + String aliasName = "my-alias"; + String dataStreamName = "logs-mysql-prod"; + String dataStreamAliasName = "logs-mysql"; + Metadata previous = Metadata.builder().build(); + + // Things that should change indices lookup + { + Metadata.Builder builder = Metadata.builder(previous); + IndexMetadata idx = DataStreamTestHelper.createFirstBackingIndex(dataStreamName).build(); + builder.put(idx, true); + DataStream dataStream = new DataStream(dataStreamName, new DataStream.TimestampField("@timestamp"), List.of(idx.getIndex())); + builder.put(dataStream); + Metadata metadata = builder.build(); + assertThat(previous.getIndicesLookup(), not(sameInstance(metadata.getIndicesLookup()))); + previous = metadata; + } + { + Metadata.Builder builder = Metadata.builder(previous); + builder.put(dataStreamAliasName, dataStreamName, false, null); + Metadata metadata = builder.build(); + assertThat(previous.getIndicesLookup(), not(sameInstance(metadata.getIndicesLookup()))); + previous = metadata; + } + { + Metadata.Builder builder = Metadata.builder(previous); + builder.put(dataStreamAliasName, dataStreamName, true, null); + Metadata metadata = builder.build(); + assertThat(previous.getIndicesLookup(), not(sameInstance(metadata.getIndicesLookup()))); + previous = metadata; + } + { + Metadata.Builder builder = Metadata.builder(previous); + builder.put(IndexMetadata.builder(indexName) + .settings(settings(Version.CURRENT)).creationDate(randomNonNegativeLong()) + .numberOfShards(1).numberOfReplicas(0)); + Metadata metadata = builder.build(); + assertThat(previous.getIndicesLookup(), not(sameInstance(metadata.getIndicesLookup()))); + previous = metadata; + } + { + Metadata.Builder builder = Metadata.builder(previous); + IndexMetadata.Builder imBuilder = IndexMetadata.builder(builder.get(indexName)); + imBuilder.putAlias(AliasMetadata.builder(aliasName).build()); + builder.put(imBuilder); + Metadata metadata = builder.build(); + assertThat(previous.getIndicesLookup(), not(sameInstance(metadata.getIndicesLookup()))); + previous = metadata; + } + { + Metadata.Builder builder = Metadata.builder(previous); + IndexMetadata.Builder imBuilder = IndexMetadata.builder(builder.get(indexName)); + imBuilder.putAlias(AliasMetadata.builder(aliasName).writeIndex(true).build()); + builder.put(imBuilder); + Metadata metadata = builder.build(); + assertThat(previous.getIndicesLookup(), not(sameInstance(metadata.getIndicesLookup()))); + previous = metadata; + } + { + Metadata.Builder builder = Metadata.builder(previous); + IndexMetadata.Builder imBuilder = IndexMetadata.builder(builder.get(indexName)); + Settings.Builder sBuilder = Settings.builder() + .put(builder.get(indexName).getSettings()) + .put(IndexMetadata.INDEX_HIDDEN_SETTING.getKey(), true); + imBuilder.settings(sBuilder.build()); + builder.put(imBuilder); + Metadata metadata = builder.build(); + assertThat(previous.getIndicesLookup(), not(sameInstance(metadata.getIndicesLookup()))); + previous = metadata; + } + + // Things that shouldn't change indices lookup + { + Metadata.Builder builder = Metadata.builder(previous); + IndexMetadata.Builder imBuilder = IndexMetadata.builder(builder.get(indexName)); + imBuilder.numberOfReplicas(2); + builder.put(imBuilder); + Metadata metadata = builder.build(); + assertThat(previous.getIndicesLookup(), sameInstance(metadata.getIndicesLookup())); + previous = metadata; + } + { + Metadata.Builder builder = Metadata.builder(previous); + IndexMetadata.Builder imBuilder = IndexMetadata.builder(builder.get(indexName)); + Settings.Builder sBuilder = Settings.builder() + .put(builder.get(indexName).getSettings()) + .put(IndexSettings.DEFAULT_FIELD_SETTING.getKey(), "val"); + imBuilder.settings(sBuilder.build()); + builder.put(imBuilder); + Metadata metadata = builder.build(); + assertThat(previous.getIndicesLookup(), sameInstance(metadata.getIndicesLookup())); + previous = metadata; + } + } + public static Metadata randomMetadata() { return randomMetadata(1); } From a9084c5f544ce9bad3f2390e5e46f42ef01ca5f7 Mon Sep 17 00:00:00 2001 From: Martijn van Groningen Date: Fri, 15 Oct 2021 16:39:02 +0200 Subject: [PATCH 4/8] added equals() and hashcode() methods for assertions --- .../cluster/metadata/IndexAbstraction.java | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/IndexAbstraction.java b/server/src/main/java/org/elasticsearch/cluster/metadata/IndexAbstraction.java index cd199057e1d96..79f458a26411e 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/IndexAbstraction.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/IndexAbstraction.java @@ -179,6 +179,23 @@ public boolean isSystem() { public List getAliases() { return aliases; } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + ConcreteIndex that = (ConcreteIndex) o; + return isHidden == that.isHidden && + isSystem == that.isSystem && + concreteIndexName.equals(that.concreteIndexName) && + Objects.equals(aliases, that.aliases) && + Objects.equals(dataStream, that.dataStream); + } + + @Override + public int hashCode() { + return Objects.hash(concreteIndexName, isHidden, isSystem, aliases, dataStream); + } } /** @@ -322,6 +339,24 @@ private void validateAliasProperties(List referenceIndexMetadatas private boolean isNonEmpty(List idxMetas) { return (Objects.isNull(idxMetas) || idxMetas.isEmpty()) == false; } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + Alias alias = (Alias) o; + return isHidden == alias.isHidden && + isSystem == alias.isSystem && + dataStreamAlias == alias.dataStreamAlias && + aliasName.equals(alias.aliasName) && + referenceIndexMetadatas.equals(alias.referenceIndexMetadatas) && + Objects.equals(writeIndex, alias.writeIndex); + } + + @Override + public int hashCode() { + return Objects.hash(aliasName, referenceIndexMetadatas, writeIndex, isHidden, isSystem, dataStreamAlias); + } } class DataStream implements IndexAbstraction { @@ -383,6 +418,20 @@ public List getAliases() { public org.elasticsearch.cluster.metadata.DataStream getDataStream() { return dataStream; } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + DataStream that = (DataStream) o; + return dataStream.equals(that.dataStream) && + Objects.equals(referencedByDataStreamAliases, that.referencedByDataStreamAliases); + } + + @Override + public int hashCode() { + return Objects.hash(dataStream, referencedByDataStreamAliases); + } } } From bbd4276b534f4949373c6596202c0877e19d3d2b Mon Sep 17 00:00:00 2001 From: Martijn van Groningen Date: Fri, 15 Oct 2021 16:39:14 +0200 Subject: [PATCH 5/8] fixed assertions --- .../java/org/elasticsearch/cluster/metadata/Metadata.java | 4 +--- .../org/elasticsearch/cluster/service/MasterService.java | 5 ++++- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java b/server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java index 072f8a8de1922..4f9e1f627e73d 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java @@ -1587,9 +1587,7 @@ public Metadata build(boolean builtIndicesLookupEagerly) { SortedMap indicesLookup; if (previousIndicesLookup != null) { - // Can't compare the values, because IndexMetadata uses IndexMetadata, which are allowed to be updated. - // TODO: change IndexAbstraction's getIndices() and getWriteIndex() methods to return String instead of IndexMetadata - assert previousIndicesLookup.keySet().equals(buildIndicesLookup(dataStreamMetadata, indices).keySet()); + assert previousIndicesLookup.equals(buildIndicesLookup(dataStreamMetadata, indices)); indicesLookup = previousIndicesLookup; } else { if (builtIndicesLookupEagerly) { diff --git a/server/src/main/java/org/elasticsearch/cluster/service/MasterService.java b/server/src/main/java/org/elasticsearch/cluster/service/MasterService.java index 09a0c16eea74f..988b7c4bdfa98 100644 --- a/server/src/main/java/org/elasticsearch/cluster/service/MasterService.java +++ b/server/src/main/java/org/elasticsearch/cluster/service/MasterService.java @@ -23,6 +23,7 @@ import org.elasticsearch.cluster.ClusterStateTaskListener; import org.elasticsearch.cluster.coordination.ClusterStatePublisher; import org.elasticsearch.cluster.coordination.FailedToCommitClusterStateException; +import org.elasticsearch.cluster.metadata.IndexAbstraction; import org.elasticsearch.cluster.metadata.Metadata; import org.elasticsearch.cluster.metadata.ProcessClusterEventTimeoutException; import org.elasticsearch.cluster.node.DiscoveryNode; @@ -52,6 +53,7 @@ import java.util.Locale; import java.util.Map; import java.util.Objects; +import java.util.SortedMap; import java.util.concurrent.TimeUnit; import java.util.function.LongSupplier; import java.util.function.Supplier; @@ -368,6 +370,7 @@ private ClusterState patchVersions(ClusterState previousClusterState, ClusterTas if (previousClusterState != newClusterState) { // only the master controls the version numbers + final var previousIndicesLookup = newClusterState.metadata().getIndicesLookup(); Builder builder = incrementVersion(newClusterState); if (previousClusterState.routingTable() != newClusterState.routingTable()) { builder.routingTable(RoutingTable.builder(newClusterState.routingTable()) @@ -378,9 +381,9 @@ private ClusterState patchVersions(ClusterState previousClusterState, ClusterTas } newClusterState = builder.build(); + assert previousIndicesLookup == newClusterState.metadata().getIndicesLookup(); } - assert previousClusterState.metadata().getIndicesLookup() == newClusterState.metadata().getIndicesLookup(); return newClusterState; } From 7e524145b57a3ff53b13ae994b87b532fb009936 Mon Sep 17 00:00:00 2001 From: Martijn van Groningen Date: Fri, 15 Oct 2021 16:56:38 +0200 Subject: [PATCH 6/8] fixed compile error after updating branch --- .../elasticsearch/xpack/deprecation/DeprecationInfoAction.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/DeprecationInfoAction.java b/x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/DeprecationInfoAction.java index 43086c3579601..5ec3c7d84cb6b 100644 --- a/x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/DeprecationInfoAction.java +++ b/x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/DeprecationInfoAction.java @@ -239,7 +239,7 @@ public static DeprecationInfoAction.Response from(ClusterState state, */ private static ClusterState removeSkippedSettings(ClusterState state, String[] indexNames, List skipTheseDeprecatedSettings) { ClusterState.Builder clusterStateBuilder = new ClusterState.Builder(state); - Metadata.Builder metadataBuilder = new Metadata.Builder(state.metadata()); + Metadata.Builder metadataBuilder = Metadata.builder(state.metadata()); metadataBuilder.transientSettings( metadataBuilder.transientSettings().filter(setting -> Regex.simpleMatch(skipTheseDeprecatedSettings, setting) == false)); metadataBuilder.persistentSettings( From 803429ae0e7b3f0933120c67d8b05ee6b3270390 Mon Sep 17 00:00:00 2001 From: Martijn van Groningen Date: Fri, 15 Oct 2021 17:06:05 +0200 Subject: [PATCH 7/8] removed unused imports --- .../java/org/elasticsearch/cluster/service/MasterService.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/service/MasterService.java b/server/src/main/java/org/elasticsearch/cluster/service/MasterService.java index 988b7c4bdfa98..19ef3b0f44d64 100644 --- a/server/src/main/java/org/elasticsearch/cluster/service/MasterService.java +++ b/server/src/main/java/org/elasticsearch/cluster/service/MasterService.java @@ -23,7 +23,6 @@ import org.elasticsearch.cluster.ClusterStateTaskListener; import org.elasticsearch.cluster.coordination.ClusterStatePublisher; import org.elasticsearch.cluster.coordination.FailedToCommitClusterStateException; -import org.elasticsearch.cluster.metadata.IndexAbstraction; import org.elasticsearch.cluster.metadata.Metadata; import org.elasticsearch.cluster.metadata.ProcessClusterEventTimeoutException; import org.elasticsearch.cluster.node.DiscoveryNode; @@ -53,7 +52,6 @@ import java.util.Locale; import java.util.Map; import java.util.Objects; -import java.util.SortedMap; import java.util.concurrent.TimeUnit; import java.util.function.LongSupplier; import java.util.function.Supplier; From 9839e3263da88250e515cc46dafe0786fe5e5265 Mon Sep 17 00:00:00 2001 From: Martijn van Groningen Date: Fri, 15 Oct 2021 17:46:07 +0200 Subject: [PATCH 8/8] also check for system indices --- .../java/org/elasticsearch/cluster/metadata/Metadata.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java b/server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java index 4f9e1f627e73d..0777bf3c30f66 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java @@ -1102,6 +1102,10 @@ boolean unsetPreviousIndicesLookup(IndexMetadata previous, IndexMetadata current return true; } + if (previous.isSystem() != current.isSystem()) { + return true; + } + if (previous.getState() != current.getState()) { return true; }