Deduplicate Node Identifier Strings in SnapshotsInProgress and ShardR…

…outing (#90556) Saw these strings use up tens of MB on a large cluster's master node during snapshotting and responding to indices stats requests. Also, this speeds up node id comparisons in the snapshot shards service and snapshots allocation decider. relates #77466
elastic · Oct 5, 2022 · 2759663 · 2759663
1 parent 4d6d979
commit 2759663
Show file tree

Hide file tree

Showing 3 changed files with 24 additions and 6 deletions.
diff --git a/server/src/main/java/org/elasticsearch/cluster/SnapshotsInProgress.java b/server/src/main/java/org/elasticsearch/cluster/SnapshotsInProgress.java
@@ -10,6 +10,7 @@
 
 import org.elasticsearch.Version;
 import org.elasticsearch.cluster.ClusterState.Custom;
+import org.elasticsearch.cluster.node.DiscoveryNode;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
@@ -544,7 +545,7 @@ private boolean assertConsistent() {
         }
 
         public static ShardSnapshotStatus readFrom(StreamInput in) throws IOException {
-            String nodeId = in.readOptionalString();
+            final String nodeId = DiscoveryNode.deduplicateNodeIdentifier(in.readOptionalString());
             final ShardState state = ShardState.fromValue(in.readByte());
             final ShardGeneration generation = in.readOptionalWriteable(ShardGeneration::new);
             final String reason = in.readOptionalString();

diff --git a/server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNode.java b/server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNode.java
@@ -17,6 +17,7 @@
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.transport.TransportAddress;
 import org.elasticsearch.common.util.StringLiteralDeduplicator;
+import org.elasticsearch.core.Nullable;
 import org.elasticsearch.node.Node;
 import org.elasticsearch.xcontent.ToXContentFragment;
 import org.elasticsearch.xcontent.XContentBuilder;
@@ -362,7 +363,7 @@ public DiscoveryNode(StreamInput in) throws IOException {
         this.hostName = readStringLiteral.read(in);
         this.hostAddress = readStringLiteral.read(in);
         this.address = new TransportAddress(in);
-        this.attributes = Collections.unmodifiableMap(in.readMap(readStringLiteral, readStringLiteral));
+        this.attributes = in.readImmutableMap(readStringLiteral, readStringLiteral);
         int rolesSize = in.readVInt();
         final SortedSet<DiscoveryNodeRole> roles = new TreeSet<>();
         final String[] roleNames = new String[rolesSize];
@@ -373,19 +374,20 @@ public DiscoveryNode(StreamInput in) throws IOException {
             final Optional<DiscoveryNodeRole> maybeRole = DiscoveryNodeRole.maybeGetRoleFromRoleName(roleName);
             if (maybeRole.isEmpty()) {
                 roles.add(new DiscoveryNodeRole.UnknownRole(roleName, roleNameAbbreviation, canContainData));
+                roleNames[i] = roleName;
             } else {
                 final DiscoveryNodeRole role = maybeRole.get();
                 assert roleName.equals(role.roleName()) : "role name [" + roleName + "] does not match role [" + role.roleName() + "]";
                 assert roleNameAbbreviation.equals(role.roleNameAbbreviation())
                     : "role name abbreviation [" + roleName + "] does not match role [" + role.roleNameAbbreviation() + "]";
                 roles.add(role);
+                roleNames[i] = role.roleName();
             }
-            roleNames[i] = roleName;
         }
         this.roles = Collections.unmodifiableSortedSet(roles);
         this.version = Version.readVersion(in);
         if (in.getVersion().onOrAfter(EXTERNAL_ID_VERSION)) {
-            this.externalId = in.readString();
+            this.externalId = readStringLiteral.read(in);
         } else {
             this.externalId = nodeName;
         }
@@ -609,4 +611,18 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
         return builder;
     }
 
+    /**
+     * Deduplicate the given string that must be a node id or node name.
+     * This method accepts {@code null} input for which it returns {@code null} for convenience when used in deserialization code.
+     *
+     * @param nodeIdentifier node name or node id or {@code null}
+     * @return deduplicated string or {@code null} on null input
+     */
+    @Nullable
+    public static String deduplicateNodeIdentifier(@Nullable String nodeIdentifier) {
+        if (nodeIdentifier == null) {
+            return null;
+        }
+        return nodeStringDeduplicator.deduplicate(nodeIdentifier);
+    }
 }
diff --git a/server/src/main/java/org/elasticsearch/cluster/routing/ShardRouting.java b/server/src/main/java/org/elasticsearch/cluster/routing/ShardRouting.java
@@ -9,6 +9,7 @@
 package org.elasticsearch.cluster.routing;
 
 import org.elasticsearch.Version;
+import org.elasticsearch.cluster.node.DiscoveryNode;
 import org.elasticsearch.cluster.routing.RecoverySource.ExistingStoreRecoverySource;
 import org.elasticsearch.cluster.routing.RecoverySource.PeerRecoverySource;
 import org.elasticsearch.cluster.routing.allocation.allocator.BalancedShardsAllocator;
@@ -293,8 +294,8 @@ public ShardIterator shardsIt() {
 
     public ShardRouting(ShardId shardId, StreamInput in) throws IOException {
         this.shardId = shardId;
-        currentNodeId = in.readOptionalString();
-        relocatingNodeId = in.readOptionalString();
+        currentNodeId = DiscoveryNode.deduplicateNodeIdentifier(in.readOptionalString());
+        relocatingNodeId = DiscoveryNode.deduplicateNodeIdentifier(in.readOptionalString());
         primary = in.readBoolean();
         state = ShardRoutingState.fromValue(in.readByte());
         if (state == ShardRoutingState.UNASSIGNED || state == ShardRoutingState.INITIALIZING) {