From 7409e4d4f10ecbe90d52d680f0d4acbdcefdcf47 Mon Sep 17 00:00:00 2001 From: Sandesh Kumar Date: Wed, 31 May 2023 15:32:11 -0700 Subject: [PATCH 01/16] Draft Cluster State Update Optimization Signed-off-by: Sandesh Kumar --- .../opensearch/cluster/metadata/Metadata.java | 97 +++++++++++++------ .../MetadataIndexTemplateService.java | 4 +- .../cluster/service/MasterService.java | 9 +- 3 files changed, 79 insertions(+), 31 deletions(-) diff --git a/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java b/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java index dde9ebfb54a49..c379f2627f7e4 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java @@ -55,6 +55,7 @@ import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Setting.Property; import org.opensearch.common.settings.Settings; +import org.opensearch.common.unit.TimeValue; import org.opensearch.common.xcontent.XContentHelper; import org.opensearch.core.common.Strings; import org.opensearch.core.xcontent.NamedObjectNotFoundException; @@ -1122,6 +1123,16 @@ public static class Builder { private final Map templates; private final Map customs; + private boolean rebuildIndicesLookups = true; + + public SortedMap indicesLookup = new TreeMap<>(); + private String[] allIndices = new String[0]; + private String[] visibleIndices = new String[0]; + private String[] allOpenIndices = new String[0]; + private String[] visibleOpenIndices = new String[0]; + private String[] allClosedIndices = new String[0]; + private String[] visibleClosedIndices = new String[0]; + public Builder() { clusterUUID = UNKNOWN_CLUSTER_UUID; indices = new HashMap<>(); @@ -1141,6 +1152,13 @@ public Builder(Metadata metadata) { this.indices = new HashMap<>(metadata.indices); this.templates = new HashMap<>(metadata.templates); this.customs = new HashMap<>(metadata.customs); + this.indicesLookup = new TreeMap<>(metadata.indicesLookup); + this.allIndices = Arrays.copyOf(metadata.allIndices, metadata.allIndices.length); + this.visibleIndices = Arrays.copyOf(metadata.visibleIndices, metadata.visibleIndices.length); + this.allOpenIndices = Arrays.copyOf(metadata.allOpenIndices, metadata.allOpenIndices.length); + this.visibleOpenIndices = Arrays.copyOf(metadata.visibleOpenIndices, metadata.visibleOpenIndices.length); + this.allClosedIndices = Arrays.copyOf(metadata.allClosedIndices, metadata.allClosedIndices.length); + this.visibleClosedIndices = Arrays.copyOf(metadata.visibleClosedIndices, metadata.visibleClosedIndices.length); } public Builder put(IndexMetadata.Builder indexMetadataBuilder) { @@ -1425,6 +1443,37 @@ public Builder generateClusterUuidIfNeeded() { } public Metadata build() { + TimeValue buildStartTime = TimeValue.timeValueMillis(System.currentTimeMillis()); + if (rebuildIndicesLookups == true) { + buildMetadataIndicesLookups(); + } + + Metadata metadata = new Metadata( + clusterUUID, + clusterUUIDCommitted, + version, + coordinationMetadata, + transientSettings, + persistentSettings, + hashesOfConsistentSettings, + indices, + templates, + customs, + allIndices, + visibleIndices, + allOpenIndices, + visibleOpenIndices, + allClosedIndices, + visibleClosedIndices, + indicesLookup + ); + TimeValue endBuildTime = TimeValue.timeValueMillis(System.currentTimeMillis()); + // Logging for testing only - will remove in future iterations + logger.info("built metadata in {} ms", endBuildTime.millis() - buildStartTime.millis()); + return metadata; + } + + private void buildMetadataIndicesLookups() { // TODO: We should move these datastructures to IndexNameExpressionResolver, this will give the following benefits: // 1) The datastructures will be rebuilt only when needed. Now during serializing we rebuild these datastructures // while these datastructures aren't even used. @@ -1511,7 +1560,11 @@ public Metadata build() { ); } - SortedMap indicesLookup = Collections.unmodifiableSortedMap(buildIndicesLookup()); + TimeValue startTime = TimeValue.timeValueMillis(System.currentTimeMillis()); + indicesLookup = Collections.unmodifiableSortedMap(buildIndicesLookup()); + TimeValue endTime = TimeValue.timeValueMillis(System.currentTimeMillis()); + // Logging for testing only - will remove in future iterations + logger.info("rebuilt indicesLookupMap in {} ms, {} entries", endTime.millis() - startTime.millis(), indicesLookup != null ? indicesLookup.size() : 0); validateDataStreams(indicesLookup, (DataStreamMetadata) customs.get(DataStreamMetadata.TYPE)); @@ -1519,32 +1572,12 @@ public Metadata build() { // TODO: I think we can remove these arrays. it isn't worth the effort, for operations on all indices. // When doing an operation across all indices, most of the time is spent on actually going to all shards and // do the required operations, the bottleneck isn't resolving expressions into concrete indices. - String[] allIndicesArray = allIndices.toArray(Strings.EMPTY_ARRAY); - String[] visibleIndicesArray = visibleIndices.toArray(Strings.EMPTY_ARRAY); - String[] allOpenIndicesArray = allOpenIndices.toArray(Strings.EMPTY_ARRAY); - String[] visibleOpenIndicesArray = visibleOpenIndices.toArray(Strings.EMPTY_ARRAY); - String[] allClosedIndicesArray = allClosedIndices.toArray(Strings.EMPTY_ARRAY); - String[] visibleClosedIndicesArray = visibleClosedIndices.toArray(Strings.EMPTY_ARRAY); - - return new Metadata( - clusterUUID, - clusterUUIDCommitted, - version, - coordinationMetadata, - transientSettings, - persistentSettings, - hashesOfConsistentSettings, - indices, - templates, - customs, - allIndicesArray, - visibleIndicesArray, - allOpenIndicesArray, - visibleOpenIndicesArray, - allClosedIndicesArray, - visibleClosedIndicesArray, - indicesLookup - ); + this.allIndices = allIndices.toArray(new String[allIndices.size()]); + this.allOpenIndices = allOpenIndices.toArray(new String[allOpenIndices.size()]); + this.allClosedIndices = allClosedIndices.toArray(new String[allClosedIndices.size()]); + this.visibleIndices = visibleIndices.toArray(new String[visibleIndices.size()]); + this.visibleOpenIndices = visibleOpenIndices.toArray(new String[visibleOpenIndices.size()]); + this.visibleClosedIndices = visibleClosedIndices.toArray(new String[visibleClosedIndices.size()]); } private SortedMap buildIndicesLookup() { @@ -1763,6 +1796,16 @@ public static Metadata fromXContent(XContentParser parser) throws IOException { } return builder.build(); } + + /** + * This flag is used to omit re-evaluating the indices lookup when building Metadata to save time taken for Metadata build() + * Its utility is at places where there is no change in indices related objects of Metadata contents. + * Its default value is true - Toggle it to false only when you are sure about that you do not need to re-evaluate Metadata + */ + public Builder rebuildIndicesLookups(boolean rebuildIndicesLookups) { + this.rebuildIndicesLookups = rebuildIndicesLookups; + return this; + } } private static final ToXContent.Params FORMAT_PARAMS; diff --git a/server/src/main/java/org/opensearch/cluster/metadata/MetadataIndexTemplateService.java b/server/src/main/java/org/opensearch/cluster/metadata/MetadataIndexTemplateService.java index 76860e568f1a1..368b8b18989b1 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/MetadataIndexTemplateService.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/MetadataIndexTemplateService.java @@ -989,8 +989,8 @@ static ClusterState innerPutTemplate( return currentState; } - Metadata.Builder builder = Metadata.builder(currentState.metadata()).put(template); - + // Template actions don't require changes in indicesLookups - so setting rebuildIndicesLookups(false) + Metadata.Builder builder = Metadata.builder(currentState.metadata()).put(template).rebuildIndicesLookups(false); logger.info("adding template [{}] for index patterns {}", request.name, request.indexPatterns); return ClusterState.builder(currentState).metadata(builder).build(); } diff --git a/server/src/main/java/org/opensearch/cluster/service/MasterService.java b/server/src/main/java/org/opensearch/cluster/service/MasterService.java index 9712fdbfbe8ec..c803919cc2302 100644 --- a/server/src/main/java/org/opensearch/cluster/service/MasterService.java +++ b/server/src/main/java/org/opensearch/cluster/service/MasterService.java @@ -292,10 +292,13 @@ private void runTasks(TaskInputs taskInputs) { } final long computationStartTime = threadPool.relativeTimeInMillis(); + TimeValue startTime = TimeValue.timeValueMillis(System.currentTimeMillis()); final TaskOutputs taskOutputs = calculateTaskOutputs(taskInputs, previousClusterState); taskOutputs.notifyFailedTasks(); final TimeValue computationTime = getTimeSince(computationStartTime); - logExecutionTime(computationTime, "compute cluster state update", summary); + TimeValue endTime = TimeValue.timeValueMillis(System.currentTimeMillis()); + // Temporarily logging time diff from System time since threadpool time is not precise for less than 200ms + logExecutionTime(TimeValue.timeValueMillis(endTime.millis() - startTime.millis()), "compute cluster state update", summary); if (taskOutputs.clusterStateUnchanged()) { final long notificationStartTime = threadPool.relativeTimeInMillis(); @@ -444,7 +447,9 @@ private ClusterState patchVersions(ClusterState previousClusterState, ClusterTas ); } if (previousClusterState.metadata() != newClusterState.metadata()) { - builder.metadata(Metadata.builder(newClusterState.metadata()).version(newClusterState.metadata().version() + 1)); + // Logging for testing only - will remove in future iterations + logger.info("Skipping recomputing indices lookup in metadata now"); + builder.metadata(Metadata.builder(newClusterState.metadata()).version(newClusterState.metadata().version() + 1).rebuildIndicesLookups(false)); } newClusterState = builder.build(); From 22e435f3ab26a48bd23d1b38b8078d40b512e7b5 Mon Sep 17 00:00:00 2001 From: Sandesh Kumar Date: Thu, 1 Jun 2023 19:59:15 -0700 Subject: [PATCH 02/16] Removing explicit skip computation variable for computing all indices lookups Signed-off-by: Sandesh Kumar --- .../opensearch/cluster/metadata/Metadata.java | 36 +++++++++---------- .../MetadataIndexTemplateService.java | 3 +- .../cluster/service/MasterService.java | 4 +-- 3 files changed, 20 insertions(+), 23 deletions(-) diff --git a/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java b/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java index c379f2627f7e4..4a9da08ef914f 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java @@ -1120,10 +1120,12 @@ public static class Builder { private DiffableStringMap hashesOfConsistentSettings = new DiffableStringMap(Collections.emptyMap()); private final Map indices; + + private final Map indicesPreviousState; private final Map templates; private final Map customs; + private final Map customsPreviousState; - private boolean rebuildIndicesLookups = true; public SortedMap indicesLookup = new TreeMap<>(); private String[] allIndices = new String[0]; @@ -1136,8 +1138,10 @@ public static class Builder { public Builder() { clusterUUID = UNKNOWN_CLUSTER_UUID; indices = new HashMap<>(); + indicesPreviousState = new HashMap<>(); templates = new HashMap<>(); customs = new HashMap<>(); + customsPreviousState = new HashMap<>(); indexGraveyard(IndexGraveyard.builder().build()); // create new empty index graveyard to initialize } @@ -1150,8 +1154,10 @@ public Builder(Metadata metadata) { this.hashesOfConsistentSettings = metadata.hashesOfConsistentSettings; this.version = metadata.version; this.indices = new HashMap<>(metadata.indices); + this.indicesPreviousState = new HashMap<>(metadata.indices); // required for comparing with updated indices this.templates = new HashMap<>(metadata.templates); this.customs = new HashMap<>(metadata.customs); + this.customsPreviousState = new HashMap<>(metadata.customs); this.indicesLookup = new TreeMap<>(metadata.indicesLookup); this.allIndices = Arrays.copyOf(metadata.allIndices, metadata.allIndices.length); this.visibleIndices = Arrays.copyOf(metadata.visibleIndices, metadata.visibleIndices.length); @@ -1443,8 +1449,12 @@ public Builder generateClusterUuidIfNeeded() { } public Metadata build() { - TimeValue buildStartTime = TimeValue.timeValueMillis(System.currentTimeMillis()); - if (rebuildIndicesLookups == true) { + TimeValue buildStartTime = TimeValue.timeValueMillis(System.nanoTime()); + boolean recomputeRequired = indices.equals(indicesPreviousState) == false || customs.equals(customsPreviousState) == false;; + TimeValue recomputeEndTime = TimeValue.timeValueMillis(System.nanoTime()); + logger.info("Recompute required: {}, time taken for comparing indices: {} ms", recomputeRequired, (recomputeEndTime.getNanos()-buildStartTime.getNanos())/1000000L); + // Will simplify this later to omit this recomputeRequired variable entirely - it's only used for testing for now + if (recomputeRequired) { buildMetadataIndicesLookups(); } @@ -1467,9 +1477,9 @@ public Metadata build() { visibleClosedIndices, indicesLookup ); - TimeValue endBuildTime = TimeValue.timeValueMillis(System.currentTimeMillis()); + TimeValue endBuildTime = TimeValue.timeValueMillis(System.nanoTime()); // Logging for testing only - will remove in future iterations - logger.info("built metadata in {} ms", endBuildTime.millis() - buildStartTime.millis()); + logger.info("built metadata in {} ms", (endBuildTime.millis() - buildStartTime.millis())/1000000L); return metadata; } @@ -1560,11 +1570,11 @@ private void buildMetadataIndicesLookups() { ); } - TimeValue startTime = TimeValue.timeValueMillis(System.currentTimeMillis()); + TimeValue startTime = TimeValue.timeValueNanos(System.nanoTime()); indicesLookup = Collections.unmodifiableSortedMap(buildIndicesLookup()); - TimeValue endTime = TimeValue.timeValueMillis(System.currentTimeMillis()); + TimeValue endTime = TimeValue.timeValueNanos(System.nanoTime()); // Logging for testing only - will remove in future iterations - logger.info("rebuilt indicesLookupMap in {} ms, {} entries", endTime.millis() - startTime.millis(), indicesLookup != null ? indicesLookup.size() : 0); + logger.info("rebuilt indicesLookupMap in {} ms, {} entries", (endTime.nanos() - startTime.nanos())/1000000L, indicesLookup != null ? indicesLookup.size() : 0); validateDataStreams(indicesLookup, (DataStreamMetadata) customs.get(DataStreamMetadata.TYPE)); @@ -1796,16 +1806,6 @@ public static Metadata fromXContent(XContentParser parser) throws IOException { } return builder.build(); } - - /** - * This flag is used to omit re-evaluating the indices lookup when building Metadata to save time taken for Metadata build() - * Its utility is at places where there is no change in indices related objects of Metadata contents. - * Its default value is true - Toggle it to false only when you are sure about that you do not need to re-evaluate Metadata - */ - public Builder rebuildIndicesLookups(boolean rebuildIndicesLookups) { - this.rebuildIndicesLookups = rebuildIndicesLookups; - return this; - } } private static final ToXContent.Params FORMAT_PARAMS; diff --git a/server/src/main/java/org/opensearch/cluster/metadata/MetadataIndexTemplateService.java b/server/src/main/java/org/opensearch/cluster/metadata/MetadataIndexTemplateService.java index 368b8b18989b1..5b8b436292cc1 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/MetadataIndexTemplateService.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/MetadataIndexTemplateService.java @@ -989,8 +989,7 @@ static ClusterState innerPutTemplate( return currentState; } - // Template actions don't require changes in indicesLookups - so setting rebuildIndicesLookups(false) - Metadata.Builder builder = Metadata.builder(currentState.metadata()).put(template).rebuildIndicesLookups(false); + Metadata.Builder builder = Metadata.builder(currentState.metadata()).put(template); logger.info("adding template [{}] for index patterns {}", request.name, request.indexPatterns); return ClusterState.builder(currentState).metadata(builder).build(); } diff --git a/server/src/main/java/org/opensearch/cluster/service/MasterService.java b/server/src/main/java/org/opensearch/cluster/service/MasterService.java index c803919cc2302..2a6612e0b0bdf 100644 --- a/server/src/main/java/org/opensearch/cluster/service/MasterService.java +++ b/server/src/main/java/org/opensearch/cluster/service/MasterService.java @@ -447,9 +447,7 @@ private ClusterState patchVersions(ClusterState previousClusterState, ClusterTas ); } if (previousClusterState.metadata() != newClusterState.metadata()) { - // Logging for testing only - will remove in future iterations - logger.info("Skipping recomputing indices lookup in metadata now"); - builder.metadata(Metadata.builder(newClusterState.metadata()).version(newClusterState.metadata().version() + 1).rebuildIndicesLookups(false)); + builder.metadata(Metadata.builder(newClusterState.metadata()).version(newClusterState.metadata().version() + 1)); } newClusterState = builder.build(); From bae5446c39115a13940da31c8f6a9a67af745cc0 Mon Sep 17 00:00:00 2001 From: Sandesh Kumar Date: Thu, 1 Jun 2023 20:06:47 -0700 Subject: [PATCH 03/16] Minor formatting Signed-off-by: Sandesh Kumar --- .../opensearch/cluster/metadata/Metadata.java | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java b/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java index 4a9da08ef914f..d3dbe3206ffc3 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java @@ -1120,13 +1120,10 @@ public static class Builder { private DiffableStringMap hashesOfConsistentSettings = new DiffableStringMap(Collections.emptyMap()); private final Map indices; - private final Map indicesPreviousState; private final Map templates; private final Map customs; private final Map customsPreviousState; - - public SortedMap indicesLookup = new TreeMap<>(); private String[] allIndices = new String[0]; private String[] visibleIndices = new String[0]; @@ -1450,9 +1447,13 @@ public Builder generateClusterUuidIfNeeded() { public Metadata build() { TimeValue buildStartTime = TimeValue.timeValueMillis(System.nanoTime()); - boolean recomputeRequired = indices.equals(indicesPreviousState) == false || customs.equals(customsPreviousState) == false;; + boolean recomputeRequired = indices.equals(indicesPreviousState) == false || customs.equals(customsPreviousState) == false; TimeValue recomputeEndTime = TimeValue.timeValueMillis(System.nanoTime()); - logger.info("Recompute required: {}, time taken for comparing indices: {} ms", recomputeRequired, (recomputeEndTime.getNanos()-buildStartTime.getNanos())/1000000L); + logger.info( + "Recompute required: {}, time taken for comparing indices: {} ms", + recomputeRequired, + (recomputeEndTime.getNanos() - buildStartTime.getNanos()) / 1000000L + ); // Will simplify this later to omit this recomputeRequired variable entirely - it's only used for testing for now if (recomputeRequired) { buildMetadataIndicesLookups(); @@ -1479,7 +1480,7 @@ public Metadata build() { ); TimeValue endBuildTime = TimeValue.timeValueMillis(System.nanoTime()); // Logging for testing only - will remove in future iterations - logger.info("built metadata in {} ms", (endBuildTime.millis() - buildStartTime.millis())/1000000L); + logger.info("built metadata in {} ms", (endBuildTime.millis() - buildStartTime.millis()) / 1000000L); return metadata; } @@ -1574,7 +1575,11 @@ private void buildMetadataIndicesLookups() { indicesLookup = Collections.unmodifiableSortedMap(buildIndicesLookup()); TimeValue endTime = TimeValue.timeValueNanos(System.nanoTime()); // Logging for testing only - will remove in future iterations - logger.info("rebuilt indicesLookupMap in {} ms, {} entries", (endTime.nanos() - startTime.nanos())/1000000L, indicesLookup != null ? indicesLookup.size() : 0); + logger.info( + "rebuilt indicesLookupMap in {} ms, {} entries", + (endTime.nanos() - startTime.nanos()) / 1000000L, + indicesLookup != null ? indicesLookup.size() : 0 + ); validateDataStreams(indicesLookup, (DataStreamMetadata) customs.get(DataStreamMetadata.TYPE)); From 37ca81d2b501b58e8d18c2b5b18a35b8d9fbcc1b Mon Sep 17 00:00:00 2001 From: Sandesh Kumar Date: Fri, 2 Jun 2023 11:36:22 -0700 Subject: [PATCH 04/16] Add Chengelog Signed-off-by: Sandesh Kumar --- CHANGELOG.md | 1 + .../cluster/metadata/MetadataIndexTemplateService.java | 1 + 2 files changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 330659fb28843..9b57bff0ff32e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -85,6 +85,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Changed - Replace jboss-annotations-api_1.2_spec with jakarta.annotation-api ([#7836](https://github.com/opensearch-project/OpenSearch/pull/7836)) +- Optimized Metadata build() to skip redundant computations as part of ClusterState build ([#7853](https://github.com/opensearch-project/OpenSearch/pull/7853)) ### Deprecated diff --git a/server/src/main/java/org/opensearch/cluster/metadata/MetadataIndexTemplateService.java b/server/src/main/java/org/opensearch/cluster/metadata/MetadataIndexTemplateService.java index 5b8b436292cc1..76860e568f1a1 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/MetadataIndexTemplateService.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/MetadataIndexTemplateService.java @@ -990,6 +990,7 @@ static ClusterState innerPutTemplate( } Metadata.Builder builder = Metadata.builder(currentState.metadata()).put(template); + logger.info("adding template [{}] for index patterns {}", request.name, request.indexPatterns); return ClusterState.builder(currentState).metadata(builder).build(); } From 62a3f690179dfba0b3878e877d66efbf8662d803 Mon Sep 17 00:00:00 2001 From: Sandesh Kumar Date: Fri, 2 Jun 2023 14:09:52 -0700 Subject: [PATCH 05/16] Refactor Signed-off-by: Sandesh Kumar --- .../opensearch/cluster/metadata/Metadata.java | 18 ++++++++++++------ .../cluster/service/MasterService.java | 5 +---- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java b/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java index d3dbe3206ffc3..33a5873eb9f12 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java @@ -1125,12 +1125,12 @@ public static class Builder { private final Map customs; private final Map customsPreviousState; public SortedMap indicesLookup = new TreeMap<>(); - private String[] allIndices = new String[0]; - private String[] visibleIndices = new String[0]; - private String[] allOpenIndices = new String[0]; - private String[] visibleOpenIndices = new String[0]; - private String[] allClosedIndices = new String[0]; - private String[] visibleClosedIndices = new String[0]; + private String[] allIndices; + private String[] visibleIndices; + private String[] allOpenIndices; + private String[] visibleOpenIndices; + private String[] allClosedIndices; + private String[] visibleClosedIndices; public Builder() { clusterUUID = UNKNOWN_CLUSTER_UUID; @@ -1139,6 +1139,12 @@ public Builder() { templates = new HashMap<>(); customs = new HashMap<>(); customsPreviousState = new HashMap<>(); + allIndices = Strings.EMPTY_ARRAY; + visibleIndices = Strings.EMPTY_ARRAY; + allOpenIndices = Strings.EMPTY_ARRAY; + visibleOpenIndices = Strings.EMPTY_ARRAY; + allClosedIndices = Strings.EMPTY_ARRAY; + visibleClosedIndices = Strings.EMPTY_ARRAY; indexGraveyard(IndexGraveyard.builder().build()); // create new empty index graveyard to initialize } diff --git a/server/src/main/java/org/opensearch/cluster/service/MasterService.java b/server/src/main/java/org/opensearch/cluster/service/MasterService.java index 2a6612e0b0bdf..9712fdbfbe8ec 100644 --- a/server/src/main/java/org/opensearch/cluster/service/MasterService.java +++ b/server/src/main/java/org/opensearch/cluster/service/MasterService.java @@ -292,13 +292,10 @@ private void runTasks(TaskInputs taskInputs) { } final long computationStartTime = threadPool.relativeTimeInMillis(); - TimeValue startTime = TimeValue.timeValueMillis(System.currentTimeMillis()); final TaskOutputs taskOutputs = calculateTaskOutputs(taskInputs, previousClusterState); taskOutputs.notifyFailedTasks(); final TimeValue computationTime = getTimeSince(computationStartTime); - TimeValue endTime = TimeValue.timeValueMillis(System.currentTimeMillis()); - // Temporarily logging time diff from System time since threadpool time is not precise for less than 200ms - logExecutionTime(TimeValue.timeValueMillis(endTime.millis() - startTime.millis()), "compute cluster state update", summary); + logExecutionTime(computationTime, "compute cluster state update", summary); if (taskOutputs.clusterStateUnchanged()) { final long notificationStartTime = threadPool.relativeTimeInMillis(); From 198a11b390631aad21cd0b4c73230a3480246879 Mon Sep 17 00:00:00 2001 From: Sandesh Kumar Date: Fri, 2 Jun 2023 11:36:22 -0700 Subject: [PATCH 06/16] Add Chengelog Signed-off-by: Sandesh Kumar --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index da225c295367b..0e4dec2cf75c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -94,6 +94,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Changed - Replace jboss-annotations-api_1.2_spec with jakarta.annotation-api ([#7836](https://github.com/opensearch-project/OpenSearch/pull/7836)) - Add min, max, average and thread info to resource stats in tasks API ([#7673](https://github.com/opensearch-project/OpenSearch/pull/7673)) +- Compress and cache cluster state during validate join request ([#7321](https://github.com/opensearch-project/OpenSearch/pull/7321)) +- [Snapshot Interop] Add Changes in Create Snapshot Flow for remote store interoperability. ([#7118](https://github.com/opensearch-project/OpenSearch/pull/7118)) +- Add new query profile collector fields with concurrent search execution ([#7898](https://github.com/opensearch-project/OpenSearch/pull/7898)) - Optimized Metadata build() to skip redundant computations as part of ClusterState build ([#7853](https://github.com/opensearch-project/OpenSearch/pull/7853)) ### Deprecated From ecec7c8bc84f1dd14808393efae5580df658b742 Mon Sep 17 00:00:00 2001 From: Sandesh Kumar Date: Thu, 8 Jun 2023 11:47:07 -0700 Subject: [PATCH 07/16] Change customs comparison to data stream comparison only Signed-off-by: Sandesh Kumar --- .../java/org/opensearch/cluster/metadata/Metadata.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java b/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java index 33a5873eb9f12..9db8e402a2138 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java @@ -1123,7 +1123,7 @@ public static class Builder { private final Map indicesPreviousState; private final Map templates; private final Map customs; - private final Map customsPreviousState; + private final DataStreamMetadata dataStreamPreviousMetadata; public SortedMap indicesLookup = new TreeMap<>(); private String[] allIndices; private String[] visibleIndices; @@ -1138,7 +1138,7 @@ public Builder() { indicesPreviousState = new HashMap<>(); templates = new HashMap<>(); customs = new HashMap<>(); - customsPreviousState = new HashMap<>(); + dataStreamPreviousMetadata = null; allIndices = Strings.EMPTY_ARRAY; visibleIndices = Strings.EMPTY_ARRAY; allOpenIndices = Strings.EMPTY_ARRAY; @@ -1160,7 +1160,7 @@ public Builder(Metadata metadata) { this.indicesPreviousState = new HashMap<>(metadata.indices); // required for comparing with updated indices this.templates = new HashMap<>(metadata.templates); this.customs = new HashMap<>(metadata.customs); - this.customsPreviousState = new HashMap<>(metadata.customs); + dataStreamPreviousMetadata = (DataStreamMetadata) metadata.customs.get(DataStreamMetadata.TYPE); this.indicesLookup = new TreeMap<>(metadata.indicesLookup); this.allIndices = Arrays.copyOf(metadata.allIndices, metadata.allIndices.length); this.visibleIndices = Arrays.copyOf(metadata.visibleIndices, metadata.visibleIndices.length); @@ -1453,7 +1453,7 @@ public Builder generateClusterUuidIfNeeded() { public Metadata build() { TimeValue buildStartTime = TimeValue.timeValueMillis(System.nanoTime()); - boolean recomputeRequired = indices.equals(indicesPreviousState) == false || customs.equals(customsPreviousState) == false; + boolean recomputeRequired = indices.equals(indicesPreviousState) == false || dataStreamPreviousMetadata.equals(this.customs.get(DataStreamMetadata.TYPE)) == false; TimeValue recomputeEndTime = TimeValue.timeValueMillis(System.nanoTime()); logger.info( "Recompute required: {}, time taken for comparing indices: {} ms", From 35b1c8a1143064c1b52c0d6f92feeb35e08a97f4 Mon Sep 17 00:00:00 2001 From: Sandesh Kumar Date: Thu, 8 Jun 2023 12:11:20 -0700 Subject: [PATCH 08/16] Fix dataStreamPreviousMetadata NPE Signed-off-by: Sandesh Kumar --- .../main/java/org/opensearch/cluster/metadata/Metadata.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java b/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java index 9db8e402a2138..dd5030160c4c0 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java @@ -1138,7 +1138,7 @@ public Builder() { indicesPreviousState = new HashMap<>(); templates = new HashMap<>(); customs = new HashMap<>(); - dataStreamPreviousMetadata = null; + dataStreamPreviousMetadata = DataStreamMetadata.builder().build(); allIndices = Strings.EMPTY_ARRAY; visibleIndices = Strings.EMPTY_ARRAY; allOpenIndices = Strings.EMPTY_ARRAY; @@ -1453,6 +1453,7 @@ public Builder generateClusterUuidIfNeeded() { public Metadata build() { TimeValue buildStartTime = TimeValue.timeValueMillis(System.nanoTime()); + boolean dataStreamsChanged = dataStreamPreviousMetadata == this.customs.get(DataStreamMetadata.TYPE); boolean recomputeRequired = indices.equals(indicesPreviousState) == false || dataStreamPreviousMetadata.equals(this.customs.get(DataStreamMetadata.TYPE)) == false; TimeValue recomputeEndTime = TimeValue.timeValueMillis(System.nanoTime()); logger.info( From 01fcd3ad240026387b0031df83a3e6402946aedd Mon Sep 17 00:00:00 2001 From: Sandesh Kumar Date: Thu, 8 Jun 2023 12:14:24 -0700 Subject: [PATCH 09/16] Remove dead code Signed-off-by: Sandesh Kumar --- .../src/main/java/org/opensearch/cluster/metadata/Metadata.java | 1 - 1 file changed, 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java b/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java index dd5030160c4c0..9520d939f737e 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java @@ -1453,7 +1453,6 @@ public Builder generateClusterUuidIfNeeded() { public Metadata build() { TimeValue buildStartTime = TimeValue.timeValueMillis(System.nanoTime()); - boolean dataStreamsChanged = dataStreamPreviousMetadata == this.customs.get(DataStreamMetadata.TYPE); boolean recomputeRequired = indices.equals(indicesPreviousState) == false || dataStreamPreviousMetadata.equals(this.customs.get(DataStreamMetadata.TYPE)) == false; TimeValue recomputeEndTime = TimeValue.timeValueMillis(System.nanoTime()); logger.info( From 59af76e5941c9622fb56a8d42d75f53905c7943f Mon Sep 17 00:00:00 2001 From: Sandesh Kumar Date: Thu, 8 Jun 2023 12:29:54 -0700 Subject: [PATCH 10/16] Spotless fix Signed-off-by: Sandesh Kumar --- .../main/java/org/opensearch/cluster/metadata/Metadata.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java b/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java index 9520d939f737e..ac99c5c3b4f1d 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java @@ -1160,7 +1160,7 @@ public Builder(Metadata metadata) { this.indicesPreviousState = new HashMap<>(metadata.indices); // required for comparing with updated indices this.templates = new HashMap<>(metadata.templates); this.customs = new HashMap<>(metadata.customs); - dataStreamPreviousMetadata = (DataStreamMetadata) metadata.customs.get(DataStreamMetadata.TYPE); + this.dataStreamPreviousMetadata = (DataStreamMetadata) metadata.customs.get(DataStreamMetadata.TYPE); this.indicesLookup = new TreeMap<>(metadata.indicesLookup); this.allIndices = Arrays.copyOf(metadata.allIndices, metadata.allIndices.length); this.visibleIndices = Arrays.copyOf(metadata.visibleIndices, metadata.visibleIndices.length); @@ -1453,7 +1453,8 @@ public Builder generateClusterUuidIfNeeded() { public Metadata build() { TimeValue buildStartTime = TimeValue.timeValueMillis(System.nanoTime()); - boolean recomputeRequired = indices.equals(indicesPreviousState) == false || dataStreamPreviousMetadata.equals(this.customs.get(DataStreamMetadata.TYPE)) == false; + boolean recomputeRequired = indices.equals(indicesPreviousState) == false + || dataStreamPreviousMetadata.equals(this.customs.get(DataStreamMetadata.TYPE)) == false; TimeValue recomputeEndTime = TimeValue.timeValueMillis(System.nanoTime()); logger.info( "Recompute required: {}, time taken for comparing indices: {} ms", From e347287ab20a3f19e306d666137774046c35b348 Mon Sep 17 00:00:00 2001 From: Sandesh Kumar Date: Thu, 8 Jun 2023 13:30:25 -0700 Subject: [PATCH 11/16] Tests fix for NPE Signed-off-by: Sandesh Kumar --- .../main/java/org/opensearch/cluster/metadata/Metadata.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java b/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java index ac99c5c3b4f1d..13e28d1c0ca53 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java @@ -1138,7 +1138,7 @@ public Builder() { indicesPreviousState = new HashMap<>(); templates = new HashMap<>(); customs = new HashMap<>(); - dataStreamPreviousMetadata = DataStreamMetadata.builder().build(); + dataStreamPreviousMetadata = null; allIndices = Strings.EMPTY_ARRAY; visibleIndices = Strings.EMPTY_ARRAY; allOpenIndices = Strings.EMPTY_ARRAY; @@ -1453,8 +1453,10 @@ public Builder generateClusterUuidIfNeeded() { public Metadata build() { TimeValue buildStartTime = TimeValue.timeValueMillis(System.nanoTime()); + DataStreamMetadata dataStreamMetadata = (DataStreamMetadata) this.customs.get(DataStreamMetadata.TYPE); boolean recomputeRequired = indices.equals(indicesPreviousState) == false - || dataStreamPreviousMetadata.equals(this.customs.get(DataStreamMetadata.TYPE)) == false; + || (dataStreamPreviousMetadata != null && dataStreamPreviousMetadata.equals(dataStreamMetadata) == false) + || (dataStreamMetadata != null && dataStreamMetadata.equals(dataStreamPreviousMetadata) == false); TimeValue recomputeEndTime = TimeValue.timeValueMillis(System.nanoTime()); logger.info( "Recompute required: {}, time taken for comparing indices: {} ms", From 264a435842291611d96ad539fc91181f89779cc5 Mon Sep 17 00:00:00 2001 From: Sandesh Kumar Date: Sun, 18 Jun 2023 20:52:12 -0700 Subject: [PATCH 12/16] Utilize previousMetadata to insted of multiple variable copies Signed-off-by: Sandesh Kumar --- .../opensearch/cluster/metadata/Metadata.java | 117 +++++++++--------- 1 file changed, 60 insertions(+), 57 deletions(-) diff --git a/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java b/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java index 13e28d1c0ca53..b0fc404d40c5b 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java @@ -1120,31 +1120,16 @@ public static class Builder { private DiffableStringMap hashesOfConsistentSettings = new DiffableStringMap(Collections.emptyMap()); private final Map indices; - private final Map indicesPreviousState; private final Map templates; private final Map customs; - private final DataStreamMetadata dataStreamPreviousMetadata; - public SortedMap indicesLookup = new TreeMap<>(); - private String[] allIndices; - private String[] visibleIndices; - private String[] allOpenIndices; - private String[] visibleOpenIndices; - private String[] allClosedIndices; - private String[] visibleClosedIndices; + private final Metadata previousMetadata; public Builder() { clusterUUID = UNKNOWN_CLUSTER_UUID; indices = new HashMap<>(); - indicesPreviousState = new HashMap<>(); templates = new HashMap<>(); customs = new HashMap<>(); - dataStreamPreviousMetadata = null; - allIndices = Strings.EMPTY_ARRAY; - visibleIndices = Strings.EMPTY_ARRAY; - allOpenIndices = Strings.EMPTY_ARRAY; - visibleOpenIndices = Strings.EMPTY_ARRAY; - allClosedIndices = Strings.EMPTY_ARRAY; - visibleClosedIndices = Strings.EMPTY_ARRAY; + previousMetadata = null; indexGraveyard(IndexGraveyard.builder().build()); // create new empty index graveyard to initialize } @@ -1157,17 +1142,9 @@ public Builder(Metadata metadata) { this.hashesOfConsistentSettings = metadata.hashesOfConsistentSettings; this.version = metadata.version; this.indices = new HashMap<>(metadata.indices); - this.indicesPreviousState = new HashMap<>(metadata.indices); // required for comparing with updated indices this.templates = new HashMap<>(metadata.templates); this.customs = new HashMap<>(metadata.customs); - this.dataStreamPreviousMetadata = (DataStreamMetadata) metadata.customs.get(DataStreamMetadata.TYPE); - this.indicesLookup = new TreeMap<>(metadata.indicesLookup); - this.allIndices = Arrays.copyOf(metadata.allIndices, metadata.allIndices.length); - this.visibleIndices = Arrays.copyOf(metadata.visibleIndices, metadata.visibleIndices.length); - this.allOpenIndices = Arrays.copyOf(metadata.allOpenIndices, metadata.allOpenIndices.length); - this.visibleOpenIndices = Arrays.copyOf(metadata.visibleOpenIndices, metadata.visibleOpenIndices.length); - this.allClosedIndices = Arrays.copyOf(metadata.allClosedIndices, metadata.allClosedIndices.length); - this.visibleClosedIndices = Arrays.copyOf(metadata.visibleClosedIndices, metadata.visibleClosedIndices.length); + this.previousMetadata = metadata; } public Builder put(IndexMetadata.Builder indexMetadataBuilder) { @@ -1454,21 +1431,32 @@ public Builder generateClusterUuidIfNeeded() { public Metadata build() { TimeValue buildStartTime = TimeValue.timeValueMillis(System.nanoTime()); DataStreamMetadata dataStreamMetadata = (DataStreamMetadata) this.customs.get(DataStreamMetadata.TYPE); - boolean recomputeRequired = indices.equals(indicesPreviousState) == false - || (dataStreamPreviousMetadata != null && dataStreamPreviousMetadata.equals(dataStreamMetadata) == false) - || (dataStreamMetadata != null && dataStreamMetadata.equals(dataStreamPreviousMetadata) == false); + DataStreamMetadata previousDataStreamMetadata = (previousMetadata != null) + ? (DataStreamMetadata) this.previousMetadata.customs.get(DataStreamMetadata.TYPE) + : null; + + boolean recomputeRequired = (previousMetadata == null) + || (indices.equals(previousMetadata.indices) == false) + || (previousDataStreamMetadata != null && previousDataStreamMetadata.equals(dataStreamMetadata) == false) + || (dataStreamMetadata != null && dataStreamMetadata.equals(previousDataStreamMetadata) == false); TimeValue recomputeEndTime = TimeValue.timeValueMillis(System.nanoTime()); logger.info( "Recompute required: {}, time taken for comparing indices: {} ms", recomputeRequired, (recomputeEndTime.getNanos() - buildStartTime.getNanos()) / 1000000L ); - // Will simplify this later to omit this recomputeRequired variable entirely - it's only used for testing for now - if (recomputeRequired) { - buildMetadataIndicesLookups(); - } - Metadata metadata = new Metadata( + Metadata metadata = (recomputeRequired == false) + ? buildMetadataWithPreviousIndicesLookups() + : buildMetadataWithRecomputedIndicesLookups(); + TimeValue endBuildTime = TimeValue.timeValueMillis(System.nanoTime()); + // Logging for testing only - will remove in future iterations + logger.info("built metadata in {} ms", (endBuildTime.millis() - buildStartTime.millis()) / 1000000L); + return metadata; + } + + private Metadata buildMetadataWithPreviousIndicesLookups() { + return new Metadata( clusterUUID, clusterUUIDCommitted, version, @@ -1479,21 +1467,17 @@ public Metadata build() { indices, templates, customs, - allIndices, - visibleIndices, - allOpenIndices, - visibleOpenIndices, - allClosedIndices, - visibleClosedIndices, - indicesLookup + Arrays.copyOf(previousMetadata.allIndices, previousMetadata.allIndices.length), + Arrays.copyOf(previousMetadata.visibleIndices, previousMetadata.visibleIndices.length), + Arrays.copyOf(previousMetadata.allOpenIndices, previousMetadata.allOpenIndices.length), + Arrays.copyOf(previousMetadata.visibleOpenIndices, previousMetadata.visibleOpenIndices.length), + Arrays.copyOf(previousMetadata.allClosedIndices, previousMetadata.allClosedIndices.length), + Arrays.copyOf(previousMetadata.visibleClosedIndices, previousMetadata.visibleClosedIndices.length), + new TreeMap<>(previousMetadata.indicesLookup) ); - TimeValue endBuildTime = TimeValue.timeValueMillis(System.nanoTime()); - // Logging for testing only - will remove in future iterations - logger.info("built metadata in {} ms", (endBuildTime.millis() - buildStartTime.millis()) / 1000000L); - return metadata; } - private void buildMetadataIndicesLookups() { + private Metadata buildMetadataWithRecomputedIndicesLookups() { // TODO: We should move these datastructures to IndexNameExpressionResolver, this will give the following benefits: // 1) The datastructures will be rebuilt only when needed. Now during serializing we rebuild these datastructures // while these datastructures aren't even used. @@ -1581,13 +1565,13 @@ private void buildMetadataIndicesLookups() { } TimeValue startTime = TimeValue.timeValueNanos(System.nanoTime()); - indicesLookup = Collections.unmodifiableSortedMap(buildIndicesLookup()); + SortedMap indicesLookup = Collections.unmodifiableSortedMap(buildIndicesLookup()); TimeValue endTime = TimeValue.timeValueNanos(System.nanoTime()); // Logging for testing only - will remove in future iterations - logger.info( + logger.debug( "rebuilt indicesLookupMap in {} ms, {} entries", (endTime.nanos() - startTime.nanos()) / 1000000L, - indicesLookup != null ? indicesLookup.size() : 0 + indicesLookup.size() ); validateDataStreams(indicesLookup, (DataStreamMetadata) customs.get(DataStreamMetadata.TYPE)); @@ -1596,12 +1580,32 @@ private void buildMetadataIndicesLookups() { // TODO: I think we can remove these arrays. it isn't worth the effort, for operations on all indices. // When doing an operation across all indices, most of the time is spent on actually going to all shards and // do the required operations, the bottleneck isn't resolving expressions into concrete indices. - this.allIndices = allIndices.toArray(new String[allIndices.size()]); - this.allOpenIndices = allOpenIndices.toArray(new String[allOpenIndices.size()]); - this.allClosedIndices = allClosedIndices.toArray(new String[allClosedIndices.size()]); - this.visibleIndices = visibleIndices.toArray(new String[visibleIndices.size()]); - this.visibleOpenIndices = visibleOpenIndices.toArray(new String[visibleOpenIndices.size()]); - this.visibleClosedIndices = visibleClosedIndices.toArray(new String[visibleClosedIndices.size()]); + String[] allIndicesArray = allIndices.toArray(Strings.EMPTY_ARRAY); + String[] visibleIndicesArray = visibleIndices.toArray(Strings.EMPTY_ARRAY); + String[] allOpenIndicesArray = allOpenIndices.toArray(Strings.EMPTY_ARRAY); + String[] visibleOpenIndicesArray = visibleOpenIndices.toArray(Strings.EMPTY_ARRAY); + String[] allClosedIndicesArray = allClosedIndices.toArray(Strings.EMPTY_ARRAY); + String[] visibleClosedIndicesArray = visibleClosedIndices.toArray(Strings.EMPTY_ARRAY); + + return new Metadata( + clusterUUID, + clusterUUIDCommitted, + version, + coordinationMetadata, + transientSettings, + persistentSettings, + hashesOfConsistentSettings, + indices, + templates, + customs, + allIndicesArray, + visibleIndicesArray, + allOpenIndicesArray, + visibleOpenIndicesArray, + allClosedIndicesArray, + visibleClosedIndicesArray, + indicesLookup + ); } private SortedMap buildIndicesLookup() { @@ -1643,8 +1647,7 @@ private SortedMap buildIndicesLookup() { IndexAbstraction existing = indicesLookup.put(indexMetadata.getIndex().getName(), index); assert existing == null : "duplicate for " + indexMetadata.getIndex(); - for (final AliasMetadata aliasCursor : indexMetadata.getAliases().values()) { - AliasMetadata aliasMetadata = aliasCursor; + for (final AliasMetadata aliasMetadata : indexMetadata.getAliases().values()) { indicesLookup.compute(aliasMetadata.getAlias(), (aliasName, alias) -> { if (alias == null) { return new IndexAbstraction.Alias(aliasMetadata, indexMetadata); From 00199f9c307d91991391ed59c81a0cf9624371cd Mon Sep 17 00:00:00 2001 From: Sandesh Kumar Date: Thu, 29 Jun 2023 22:07:21 -0700 Subject: [PATCH 13/16] Remove extra log lines and add test cases Signed-off-by: Sandesh Kumar --- .../opensearch/cluster/metadata/Metadata.java | 30 ++---------- .../cluster/metadata/MetadataTests.java | 48 +++++++++++++++++++ 2 files changed, 52 insertions(+), 26 deletions(-) diff --git a/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java b/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java index b0fc404d40c5b..d89c2048fdeb0 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java @@ -55,7 +55,6 @@ import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Setting.Property; import org.opensearch.common.settings.Settings; -import org.opensearch.common.unit.TimeValue; import org.opensearch.common.xcontent.XContentHelper; import org.opensearch.core.common.Strings; import org.opensearch.core.xcontent.NamedObjectNotFoundException; @@ -1429,7 +1428,6 @@ public Builder generateClusterUuidIfNeeded() { } public Metadata build() { - TimeValue buildStartTime = TimeValue.timeValueMillis(System.nanoTime()); DataStreamMetadata dataStreamMetadata = (DataStreamMetadata) this.customs.get(DataStreamMetadata.TYPE); DataStreamMetadata previousDataStreamMetadata = (previousMetadata != null) ? (DataStreamMetadata) this.previousMetadata.customs.get(DataStreamMetadata.TYPE) @@ -1439,23 +1437,11 @@ public Metadata build() { || (indices.equals(previousMetadata.indices) == false) || (previousDataStreamMetadata != null && previousDataStreamMetadata.equals(dataStreamMetadata) == false) || (dataStreamMetadata != null && dataStreamMetadata.equals(previousDataStreamMetadata) == false); - TimeValue recomputeEndTime = TimeValue.timeValueMillis(System.nanoTime()); - logger.info( - "Recompute required: {}, time taken for comparing indices: {} ms", - recomputeRequired, - (recomputeEndTime.getNanos() - buildStartTime.getNanos()) / 1000000L - ); - Metadata metadata = (recomputeRequired == false) - ? buildMetadataWithPreviousIndicesLookups() - : buildMetadataWithRecomputedIndicesLookups(); - TimeValue endBuildTime = TimeValue.timeValueMillis(System.nanoTime()); - // Logging for testing only - will remove in future iterations - logger.info("built metadata in {} ms", (endBuildTime.millis() - buildStartTime.millis()) / 1000000L); - return metadata; + return (recomputeRequired == false) ? buildMetadataWithPreviousIndicesLookups() : buildMetadataWithRecomputedIndicesLookups(); } - private Metadata buildMetadataWithPreviousIndicesLookups() { + protected Metadata buildMetadataWithPreviousIndicesLookups() { return new Metadata( clusterUUID, clusterUUIDCommitted, @@ -1473,11 +1459,11 @@ private Metadata buildMetadataWithPreviousIndicesLookups() { Arrays.copyOf(previousMetadata.visibleOpenIndices, previousMetadata.visibleOpenIndices.length), Arrays.copyOf(previousMetadata.allClosedIndices, previousMetadata.allClosedIndices.length), Arrays.copyOf(previousMetadata.visibleClosedIndices, previousMetadata.visibleClosedIndices.length), - new TreeMap<>(previousMetadata.indicesLookup) + Collections.unmodifiableSortedMap(previousMetadata.indicesLookup) ); } - private Metadata buildMetadataWithRecomputedIndicesLookups() { + protected Metadata buildMetadataWithRecomputedIndicesLookups() { // TODO: We should move these datastructures to IndexNameExpressionResolver, this will give the following benefits: // 1) The datastructures will be rebuilt only when needed. Now during serializing we rebuild these datastructures // while these datastructures aren't even used. @@ -1564,15 +1550,7 @@ private Metadata buildMetadataWithRecomputedIndicesLookups() { ); } - TimeValue startTime = TimeValue.timeValueNanos(System.nanoTime()); SortedMap indicesLookup = Collections.unmodifiableSortedMap(buildIndicesLookup()); - TimeValue endTime = TimeValue.timeValueNanos(System.nanoTime()); - // Logging for testing only - will remove in future iterations - logger.debug( - "rebuilt indicesLookupMap in {} ms, {} entries", - (endTime.nanos() - startTime.nanos()) / 1000000L, - indicesLookup.size() - ); validateDataStreams(indicesLookup, (DataStreamMetadata) customs.get(DataStreamMetadata.TYPE)); diff --git a/server/src/test/java/org/opensearch/cluster/metadata/MetadataTests.java b/server/src/test/java/org/opensearch/cluster/metadata/MetadataTests.java index a744c181ee341..6ea9070dc90e8 100644 --- a/server/src/test/java/org/opensearch/cluster/metadata/MetadataTests.java +++ b/server/src/test/java/org/opensearch/cluster/metadata/MetadataTests.java @@ -32,6 +32,7 @@ package org.opensearch.cluster.metadata; +import org.mockito.Mockito; import org.opensearch.Version; import org.opensearch.action.admin.indices.alias.get.GetAliasesRequest; import org.opensearch.cluster.ClusterModule; @@ -58,6 +59,7 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -1435,4 +1437,50 @@ private static class CreateIndexResult { this.metadata = metadata; } } + + public void testMetadataBuild() { + Metadata previousMetadata = randomMetadata(); + Metadata.Builder spyBuilder = Mockito.spy(Metadata.builder()); + + // previous Metadata state was not provided to Builder during assignment - indices lookups should get re-computed + spyBuilder.build(); + Mockito.verify(spyBuilder, Mockito.times(1)).buildMetadataWithRecomputedIndicesLookups(); + Mockito.verify(spyBuilder, Mockito.times(0)).buildMetadataWithPreviousIndicesLookups(); + + // no changes in builder method after initialization from previous Metadata - indices lookups should not be re-computed + spyBuilder = Mockito.spy(Metadata.builder(previousMetadata)); + spyBuilder.build(); + Mockito.verify(spyBuilder, Mockito.times(0)).buildMetadataWithRecomputedIndicesLookups(); + Mockito.verify(spyBuilder, Mockito.times(1)).buildMetadataWithPreviousIndicesLookups(); + Mockito.reset(spyBuilder); + + // Adding new index - all indices lookups should get re-computed + spyBuilder = Mockito.spy(Metadata.builder(previousMetadata)); + String index = "new-index"; + spyBuilder.indices(Collections.singletonMap(index, IndexMetadata.builder(index) + .settings(settings(Version.CURRENT)).numberOfShards(1).numberOfReplicas(1).build())) + .build(); + Mockito.verify(spyBuilder, Mockito.times(1)).buildMetadataWithRecomputedIndicesLookups(); + Mockito.verify(spyBuilder, Mockito.times(0)).buildMetadataWithPreviousIndicesLookups(); + Mockito.reset(spyBuilder); + + // Adding new templates - indices lookups should not get recomputed + spyBuilder = Mockito.spy(Metadata.builder(previousMetadata)); + spyBuilder.put("component_template_new_" + randomAlphaOfLength(3), ComponentTemplateTests.randomInstance()) + .put("index_template_v2_new_" + randomAlphaOfLength(3), ComposableIndexTemplateTests.randomInstance()) + .build(); + Mockito.verify(spyBuilder, Mockito.times(0)).buildMetadataWithRecomputedIndicesLookups(); + Mockito.verify(spyBuilder, Mockito.times(1)).buildMetadataWithPreviousIndicesLookups(); + Mockito.reset(spyBuilder); + + // Adding new data-stream - indices lookups should get re-computed + spyBuilder = Mockito.spy(Metadata.builder(previousMetadata)); + DataStream dataStream = DataStreamTests.randomInstance(); + for (Index backingIndex : dataStream.getIndices()) { + spyBuilder.put(DataStreamTestHelper.getIndexMetadataBuilderForIndex(backingIndex)); + } + spyBuilder.put(dataStream).build(); + Mockito.verify(spyBuilder, Mockito.times(1)).buildMetadataWithRecomputedIndicesLookups(); + Mockito.verify(spyBuilder, Mockito.times(0)).buildMetadataWithPreviousIndicesLookups(); + } } From e3cca6ff7c1c0be7f7957149b07b20ad5a2b253a Mon Sep 17 00:00:00 2001 From: Sandesh Kumar Date: Thu, 29 Jun 2023 22:36:45 -0700 Subject: [PATCH 14/16] Refactoring and spotless check Signed-off-by: Sandesh Kumar --- .../opensearch/cluster/metadata/Metadata.java | 6 +- .../cluster/metadata/MetadataTests.java | 95 ++++++++++--------- 2 files changed, 53 insertions(+), 48 deletions(-) diff --git a/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java b/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java index d89c2048fdeb0..5d0e4f9aa7e3f 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java @@ -1433,12 +1433,14 @@ public Metadata build() { ? (DataStreamMetadata) this.previousMetadata.customs.get(DataStreamMetadata.TYPE) : null; - boolean recomputeRequired = (previousMetadata == null) + boolean recomputeRequiredforIndicesLookups = (previousMetadata == null) || (indices.equals(previousMetadata.indices) == false) || (previousDataStreamMetadata != null && previousDataStreamMetadata.equals(dataStreamMetadata) == false) || (dataStreamMetadata != null && dataStreamMetadata.equals(previousDataStreamMetadata) == false); - return (recomputeRequired == false) ? buildMetadataWithPreviousIndicesLookups() : buildMetadataWithRecomputedIndicesLookups(); + return (recomputeRequiredforIndicesLookups == false) + ? buildMetadataWithPreviousIndicesLookups() + : buildMetadataWithRecomputedIndicesLookups(); } protected Metadata buildMetadataWithPreviousIndicesLookups() { diff --git a/server/src/test/java/org/opensearch/cluster/metadata/MetadataTests.java b/server/src/test/java/org/opensearch/cluster/metadata/MetadataTests.java index 6ea9070dc90e8..da2a779903f7c 100644 --- a/server/src/test/java/org/opensearch/cluster/metadata/MetadataTests.java +++ b/server/src/test/java/org/opensearch/cluster/metadata/MetadataTests.java @@ -1366,6 +1366,55 @@ public void testValidateDataStreamsForNullDataStreamMetadata() { } } + public void testMetadataBuildInvocations() { + Metadata previousMetadata = randomMetadata(); + Metadata.Builder spyBuilder = Mockito.spy(Metadata.builder()); + + // previous Metadata state was not provided to Builder during assignment - indices lookups should get re-computed + spyBuilder.build(); + Mockito.verify(spyBuilder, Mockito.times(1)).buildMetadataWithRecomputedIndicesLookups(); + Mockito.verify(spyBuilder, Mockito.times(0)).buildMetadataWithPreviousIndicesLookups(); + + // no changes in builder method after initialization from previous Metadata - indices lookups should not be re-computed + spyBuilder = Mockito.spy(Metadata.builder(previousMetadata)); + spyBuilder.build(); + Mockito.verify(spyBuilder, Mockito.times(0)).buildMetadataWithRecomputedIndicesLookups(); + Mockito.verify(spyBuilder, Mockito.times(1)).buildMetadataWithPreviousIndicesLookups(); + Mockito.reset(spyBuilder); + + // Adding new index - all indices lookups should get re-computed + spyBuilder = Mockito.spy(Metadata.builder(previousMetadata)); + String index = "new-index"; + spyBuilder.indices( + Collections.singletonMap( + index, + IndexMetadata.builder(index).settings(settings(Version.CURRENT)).numberOfShards(1).numberOfReplicas(1).build() + ) + ).build(); + Mockito.verify(spyBuilder, Mockito.times(1)).buildMetadataWithRecomputedIndicesLookups(); + Mockito.verify(spyBuilder, Mockito.times(0)).buildMetadataWithPreviousIndicesLookups(); + Mockito.reset(spyBuilder); + + // Adding new templates - indices lookups should not get recomputed + spyBuilder = Mockito.spy(Metadata.builder(previousMetadata)); + spyBuilder.put("component_template_new_" + randomAlphaOfLength(3), ComponentTemplateTests.randomInstance()) + .put("index_template_v2_new_" + randomAlphaOfLength(3), ComposableIndexTemplateTests.randomInstance()) + .build(); + Mockito.verify(spyBuilder, Mockito.times(0)).buildMetadataWithRecomputedIndicesLookups(); + Mockito.verify(spyBuilder, Mockito.times(1)).buildMetadataWithPreviousIndicesLookups(); + Mockito.reset(spyBuilder); + + // Adding new data-stream - indices lookups should get re-computed + spyBuilder = Mockito.spy(Metadata.builder(previousMetadata)); + DataStream dataStream = DataStreamTests.randomInstance(); + for (Index backingIndex : dataStream.getIndices()) { + spyBuilder.put(DataStreamTestHelper.getIndexMetadataBuilderForIndex(backingIndex)); + } + spyBuilder.put(dataStream).build(); + Mockito.verify(spyBuilder, Mockito.times(1)).buildMetadataWithRecomputedIndicesLookups(); + Mockito.verify(spyBuilder, Mockito.times(0)).buildMetadataWithPreviousIndicesLookups(); + } + public static Metadata randomMetadata() { Metadata.Builder md = Metadata.builder() .put(buildIndexMetadata("index", "alias", randomBoolean() ? null : randomBoolean()).build(), randomBoolean()) @@ -1437,50 +1486,4 @@ private static class CreateIndexResult { this.metadata = metadata; } } - - public void testMetadataBuild() { - Metadata previousMetadata = randomMetadata(); - Metadata.Builder spyBuilder = Mockito.spy(Metadata.builder()); - - // previous Metadata state was not provided to Builder during assignment - indices lookups should get re-computed - spyBuilder.build(); - Mockito.verify(spyBuilder, Mockito.times(1)).buildMetadataWithRecomputedIndicesLookups(); - Mockito.verify(spyBuilder, Mockito.times(0)).buildMetadataWithPreviousIndicesLookups(); - - // no changes in builder method after initialization from previous Metadata - indices lookups should not be re-computed - spyBuilder = Mockito.spy(Metadata.builder(previousMetadata)); - spyBuilder.build(); - Mockito.verify(spyBuilder, Mockito.times(0)).buildMetadataWithRecomputedIndicesLookups(); - Mockito.verify(spyBuilder, Mockito.times(1)).buildMetadataWithPreviousIndicesLookups(); - Mockito.reset(spyBuilder); - - // Adding new index - all indices lookups should get re-computed - spyBuilder = Mockito.spy(Metadata.builder(previousMetadata)); - String index = "new-index"; - spyBuilder.indices(Collections.singletonMap(index, IndexMetadata.builder(index) - .settings(settings(Version.CURRENT)).numberOfShards(1).numberOfReplicas(1).build())) - .build(); - Mockito.verify(spyBuilder, Mockito.times(1)).buildMetadataWithRecomputedIndicesLookups(); - Mockito.verify(spyBuilder, Mockito.times(0)).buildMetadataWithPreviousIndicesLookups(); - Mockito.reset(spyBuilder); - - // Adding new templates - indices lookups should not get recomputed - spyBuilder = Mockito.spy(Metadata.builder(previousMetadata)); - spyBuilder.put("component_template_new_" + randomAlphaOfLength(3), ComponentTemplateTests.randomInstance()) - .put("index_template_v2_new_" + randomAlphaOfLength(3), ComposableIndexTemplateTests.randomInstance()) - .build(); - Mockito.verify(spyBuilder, Mockito.times(0)).buildMetadataWithRecomputedIndicesLookups(); - Mockito.verify(spyBuilder, Mockito.times(1)).buildMetadataWithPreviousIndicesLookups(); - Mockito.reset(spyBuilder); - - // Adding new data-stream - indices lookups should get re-computed - spyBuilder = Mockito.spy(Metadata.builder(previousMetadata)); - DataStream dataStream = DataStreamTests.randomInstance(); - for (Index backingIndex : dataStream.getIndices()) { - spyBuilder.put(DataStreamTestHelper.getIndexMetadataBuilderForIndex(backingIndex)); - } - spyBuilder.put(dataStream).build(); - Mockito.verify(spyBuilder, Mockito.times(1)).buildMetadataWithRecomputedIndicesLookups(); - Mockito.verify(spyBuilder, Mockito.times(0)).buildMetadataWithPreviousIndicesLookups(); - } } From a91edcb594a1395b6b8ed367f4b546c5ac82e92e Mon Sep 17 00:00:00 2001 From: Sandesh Kumar Date: Fri, 30 Jun 2023 13:34:34 -0700 Subject: [PATCH 15/16] Inclusing tests for metadata comparison in existing test Signed-off-by: Sandesh Kumar --- CHANGELOG.md | 2 +- .../cluster/metadata/MetadataTests.java | 110 +++++++++++++----- 2 files changed, 80 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b73b9a028ac0b..8219d6d1d9164 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -141,7 +141,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Add self-organizing hash table to improve the performance of bucket aggregations ([#7652](https://github.com/opensearch-project/OpenSearch/pull/7652)) - Check UTF16 string size before converting to String to avoid OOME ([#7963](https://github.com/opensearch-project/OpenSearch/pull/7963)) - Move ZSTD compression codecs out of the sandbox ([#7908](https://github.com/opensearch-project/OpenSearch/pull/7908)) -- Optimized Metadata build() to skip redundant computations as part of ClusterState build ([#7853](https://github.com/opensearch-project/OpenSearch/pull/7853)) +- Optimize Metadata build() to skip redundant computations as part of ClusterState build ([#7853](https://github.com/opensearch-project/OpenSearch/pull/7853)) ### Deprecated diff --git a/server/src/test/java/org/opensearch/cluster/metadata/MetadataTests.java b/server/src/test/java/org/opensearch/cluster/metadata/MetadataTests.java index da2a779903f7c..fed06e7a21ce9 100644 --- a/server/src/test/java/org/opensearch/cluster/metadata/MetadataTests.java +++ b/server/src/test/java/org/opensearch/cluster/metadata/MetadataTests.java @@ -32,7 +32,6 @@ package org.opensearch.cluster.metadata; -import org.mockito.Mockito; import org.opensearch.Version; import org.opensearch.action.admin.indices.alias.get.GetAliasesRequest; import org.opensearch.cluster.ClusterModule; @@ -78,6 +77,10 @@ import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.startsWith; +import static org.mockito.Mockito.reset; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; public class MetadataTests extends OpenSearchTestCase { @@ -1367,52 +1370,59 @@ public void testValidateDataStreamsForNullDataStreamMetadata() { } public void testMetadataBuildInvocations() { - Metadata previousMetadata = randomMetadata(); - Metadata.Builder spyBuilder = Mockito.spy(Metadata.builder()); + final Metadata previousMetadata = randomMetadata(); + Metadata builtMetadata; + Metadata.Builder spyBuilder; // previous Metadata state was not provided to Builder during assignment - indices lookups should get re-computed - spyBuilder.build(); - Mockito.verify(spyBuilder, Mockito.times(1)).buildMetadataWithRecomputedIndicesLookups(); - Mockito.verify(spyBuilder, Mockito.times(0)).buildMetadataWithPreviousIndicesLookups(); + spyBuilder = spy(Metadata.builder()); + builtMetadata = spyBuilder.build(); + verify(spyBuilder, times(1)).buildMetadataWithRecomputedIndicesLookups(); + verify(spyBuilder, times(0)).buildMetadataWithPreviousIndicesLookups(); + compareMetadata(Metadata.EMPTY_METADATA, builtMetadata, true, true, false); // no changes in builder method after initialization from previous Metadata - indices lookups should not be re-computed - spyBuilder = Mockito.spy(Metadata.builder(previousMetadata)); - spyBuilder.build(); - Mockito.verify(spyBuilder, Mockito.times(0)).buildMetadataWithRecomputedIndicesLookups(); - Mockito.verify(spyBuilder, Mockito.times(1)).buildMetadataWithPreviousIndicesLookups(); - Mockito.reset(spyBuilder); - - // Adding new index - all indices lookups should get re-computed - spyBuilder = Mockito.spy(Metadata.builder(previousMetadata)); - String index = "new-index"; - spyBuilder.indices( + spyBuilder = spy(Metadata.builder(previousMetadata)); + builtMetadata = spyBuilder.version(previousMetadata.version() + 1).build(); + verify(spyBuilder, times(0)).buildMetadataWithRecomputedIndicesLookups(); + verify(spyBuilder, times(1)).buildMetadataWithPreviousIndicesLookups(); + compareMetadata(previousMetadata, builtMetadata, true, true, true); + reset(spyBuilder); + + // adding new index - all indices lookups should get re-computed + spyBuilder = spy(Metadata.builder(previousMetadata)); + String index = "new_index_" + randomAlphaOfLength(3); + builtMetadata = spyBuilder.indices( Collections.singletonMap( index, IndexMetadata.builder(index).settings(settings(Version.CURRENT)).numberOfShards(1).numberOfReplicas(1).build() ) ).build(); - Mockito.verify(spyBuilder, Mockito.times(1)).buildMetadataWithRecomputedIndicesLookups(); - Mockito.verify(spyBuilder, Mockito.times(0)).buildMetadataWithPreviousIndicesLookups(); - Mockito.reset(spyBuilder); - - // Adding new templates - indices lookups should not get recomputed - spyBuilder = Mockito.spy(Metadata.builder(previousMetadata)); - spyBuilder.put("component_template_new_" + randomAlphaOfLength(3), ComponentTemplateTests.randomInstance()) + verify(spyBuilder, times(1)).buildMetadataWithRecomputedIndicesLookups(); + verify(spyBuilder, times(0)).buildMetadataWithPreviousIndicesLookups(); + compareMetadata(previousMetadata, builtMetadata, false, true, false); + reset(spyBuilder); + + // adding new templates - indices lookups should not get recomputed + spyBuilder = spy(Metadata.builder(previousMetadata)); + builtMetadata = spyBuilder.put("component_template_new_" + randomAlphaOfLength(3), ComponentTemplateTests.randomInstance()) .put("index_template_v2_new_" + randomAlphaOfLength(3), ComposableIndexTemplateTests.randomInstance()) .build(); - Mockito.verify(spyBuilder, Mockito.times(0)).buildMetadataWithRecomputedIndicesLookups(); - Mockito.verify(spyBuilder, Mockito.times(1)).buildMetadataWithPreviousIndicesLookups(); - Mockito.reset(spyBuilder); + verify(spyBuilder, times(0)).buildMetadataWithRecomputedIndicesLookups(); + verify(spyBuilder, times(1)).buildMetadataWithPreviousIndicesLookups(); + compareMetadata(previousMetadata, builtMetadata, true, false, false); + reset(spyBuilder); - // Adding new data-stream - indices lookups should get re-computed - spyBuilder = Mockito.spy(Metadata.builder(previousMetadata)); + // adding new data stream - indices lookups should get re-computed + spyBuilder = spy(Metadata.builder(previousMetadata)); DataStream dataStream = DataStreamTests.randomInstance(); for (Index backingIndex : dataStream.getIndices()) { spyBuilder.put(DataStreamTestHelper.getIndexMetadataBuilderForIndex(backingIndex)); } - spyBuilder.put(dataStream).build(); - Mockito.verify(spyBuilder, Mockito.times(1)).buildMetadataWithRecomputedIndicesLookups(); - Mockito.verify(spyBuilder, Mockito.times(0)).buildMetadataWithPreviousIndicesLookups(); + builtMetadata = spyBuilder.put(dataStream).version(previousMetadata.version() + 1).build(); + verify(spyBuilder, times(1)).buildMetadataWithRecomputedIndicesLookups(); + verify(spyBuilder, times(0)).buildMetadataWithPreviousIndicesLookups(); + compareMetadata(previousMetadata, builtMetadata, false, true, true); } public static Metadata randomMetadata() { @@ -1486,4 +1496,42 @@ private static class CreateIndexResult { this.metadata = metadata; } } + + private static void compareMetadata( + final Metadata previousMetadata, + final Metadata newMetadata, + final boolean compareIndicesLookups, + final boolean compareTemplates, + final boolean checkVersionIncrement + ) { + assertEquals(previousMetadata.clusterUUID(), newMetadata.clusterUUID()); + assertEquals(previousMetadata.clusterUUIDCommitted(), newMetadata.clusterUUIDCommitted()); + assertEquals(previousMetadata.coordinationMetadata(), newMetadata.coordinationMetadata()); + assertEquals(previousMetadata.settings(), newMetadata.settings()); + assertEquals(previousMetadata.transientSettings(), newMetadata.transientSettings()); + assertEquals(previousMetadata.persistentSettings(), newMetadata.persistentSettings()); + assertEquals(previousMetadata.hashesOfConsistentSettings(), newMetadata.hashesOfConsistentSettings()); + + if (compareIndicesLookups == true) { + assertEquals(previousMetadata.indices(), newMetadata.indices()); + assertEquals(previousMetadata.getConcreteAllIndices(), newMetadata.getConcreteAllIndices()); + assertEquals(previousMetadata.getConcreteAllClosedIndices(), newMetadata.getConcreteAllClosedIndices()); + assertEquals(previousMetadata.getConcreteAllOpenIndices(), newMetadata.getConcreteAllOpenIndices()); + assertEquals(previousMetadata.getConcreteVisibleIndices(), newMetadata.getConcreteVisibleIndices()); + assertEquals(previousMetadata.getConcreteVisibleClosedIndices(), newMetadata.getConcreteVisibleClosedIndices()); + assertEquals(previousMetadata.getConcreteVisibleOpenIndices(), newMetadata.getConcreteVisibleOpenIndices()); + assertEquals(previousMetadata.getIndicesLookup(), newMetadata.getIndicesLookup()); + assertEquals(previousMetadata.getCustoms(), newMetadata.getCustoms()); + } + + if (compareTemplates == true) { + assertEquals(previousMetadata.templates(), newMetadata.templates()); + assertEquals(previousMetadata.templatesV2(), newMetadata.templatesV2()); + assertEquals(previousMetadata.componentTemplates(), newMetadata.componentTemplates()); + } + + if (checkVersionIncrement == true) { + assertEquals(previousMetadata.version() + 1, newMetadata.version()); + } + } } From f16d1ac991f2cf9edb8e3dabee6eab147e160eea Mon Sep 17 00:00:00 2001 From: Sandesh Kumar Date: Fri, 30 Jun 2023 13:42:02 -0700 Subject: [PATCH 16/16] Fix test Signed-off-by: Sandesh Kumar --- .../java/org/opensearch/cluster/metadata/MetadataTests.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/server/src/test/java/org/opensearch/cluster/metadata/MetadataTests.java b/server/src/test/java/org/opensearch/cluster/metadata/MetadataTests.java index fed06e7a21ce9..29904aa9e3ff8 100644 --- a/server/src/test/java/org/opensearch/cluster/metadata/MetadataTests.java +++ b/server/src/test/java/org/opensearch/cluster/metadata/MetadataTests.java @@ -1521,7 +1521,7 @@ private static void compareMetadata( assertEquals(previousMetadata.getConcreteVisibleClosedIndices(), newMetadata.getConcreteVisibleClosedIndices()); assertEquals(previousMetadata.getConcreteVisibleOpenIndices(), newMetadata.getConcreteVisibleOpenIndices()); assertEquals(previousMetadata.getIndicesLookup(), newMetadata.getIndicesLookup()); - assertEquals(previousMetadata.getCustoms(), newMetadata.getCustoms()); + assertEquals(previousMetadata.getCustoms().get(DataStreamMetadata.TYPE), newMetadata.getCustoms().get(DataStreamMetadata.TYPE)); } if (compareTemplates == true) { @@ -1530,6 +1530,10 @@ private static void compareMetadata( assertEquals(previousMetadata.componentTemplates(), newMetadata.componentTemplates()); } + if (compareIndicesLookups == true && compareTemplates == true) { + assertEquals(previousMetadata.getCustoms(), newMetadata.getCustoms()); + } + if (checkVersionIncrement == true) { assertEquals(previousMetadata.version() + 1, newMetadata.version()); }