Refactor SnapshotsInProgress to simplified constructors (#89813)

Simplify logic out of the primary constructor. This allows for faster copy
construction when forwarding the repository generation and allows for a cleaner
implementation of diffing in a follow-up.

modules/data-streams/src/test/java/org/elasticsearch/datastreams/action/DeleteDataStreamTransportActionTests.java

@@ -113,7 +113,7 @@ public class DeleteDataStreamTransportActionTests extends ESTestCase {
     }
 
     private SnapshotsInProgress.Entry createEntry(String dataStreamName, String repo, boolean partial) {
-        return new SnapshotsInProgress.Entry(
+        return SnapshotsInProgress.Entry.snapshot(
             new Snapshot(repo, new SnapshotId("", "")),
             false,
             partial,

server/src/internalClusterTest/java/org/elasticsearch/cluster/ClusterStateDiffIT.java

@@ -706,7 +706,7 @@ public class ClusterStateDiffIT extends ESIntegTestCase {
             public ClusterState.Custom randomCreate(String name) {
                 return switch (randomIntBetween(0, 1)) {
                     case 0 -> SnapshotsInProgress.EMPTY.withAddedEntry(
-                        new SnapshotsInProgress.Entry(
+                        SnapshotsInProgress.Entry.snapshot(
                             new Snapshot(randomName("repo"), new SnapshotId(randomName("snap"), UUIDs.randomBase64UUID())),
                             randomBoolean(),
                             randomBoolean(),

server/src/main/java/org/elasticsearch/cluster/SnapshotsInProgress.java

@@ -255,7 +255,7 @@ public class SnapshotsInProgress extends AbstractNamedDiffable<Custom> implement
         Version version,
         List<SnapshotFeatureInfo> featureStates
     ) {
-        return new SnapshotsInProgress.Entry(
+        return Entry.snapshot(
             snapshot,
             includeGlobalState,
             partial,
@@ -291,23 +291,7 @@ public class SnapshotsInProgress extends AbstractNamedDiffable<Custom> implement
         long repositoryStateId,
         Version version
     ) {
-        return new SnapshotsInProgress.Entry(
-            snapshot,
-            true,
-            false,
-            State.STARTED,
-            indices,
-            Collections.emptyList(),
-            Collections.emptyList(),
-            startTime,
-            repositoryStateId,
-            Map.of(),
-            null,
-            Collections.emptyMap(),
-            version,
-            source,
-            Map.of()
-        );
+        return Entry.createClone(snapshot, State.STARTED, indices, startTime, repositoryStateId, null, version, source, Map.of());
     }
 
     /**
@@ -692,7 +676,7 @@ public class SnapshotsInProgress extends AbstractNamedDiffable<Custom> implement
         private final String failure;
 
         // visible for testing, use #startedEntry and copy constructors in production code
-        public Entry(
+        public static Entry snapshot(
             Snapshot snapshot,
             boolean includeGlobalState,
             boolean partial,
@@ -707,7 +691,17 @@ public class SnapshotsInProgress extends AbstractNamedDiffable<Custom> implement
             Map<String, Object> userMetadata,
             Version version
         ) {
-            this(
+            final Map<String, Index> res = Maps.newMapWithExpectedSize(indices.size());
+            final Map<RepositoryShardId, ShardSnapshotStatus> byRepoShardIdBuilder = Maps.newHashMapWithExpectedSize(shards.size());
+            for (Map.Entry<ShardId, ShardSnapshotStatus> entry : shards.entrySet()) {
+                final ShardId shardId = entry.getKey();
+                final IndexId indexId = indices.get(shardId.getIndexName());
+                final Index index = shardId.getIndex();
+                final Index existing = res.put(indexId.getName(), index);
+                assert existing == null || existing.equals(index) : "Conflicting indices [" + existing + "] and [" + index + "]";
+                byRepoShardIdBuilder.put(new RepositoryShardId(indexId, shardId.id()), entry.getValue());
+            }
+            return new Entry(
                 snapshot,
                 includeGlobalState,
                 partial,
@@ -722,6 +716,38 @@ public class SnapshotsInProgress extends AbstractNamedDiffable<Custom> implement
                 userMetadata,
                 version,
                 null,
+                byRepoShardIdBuilder,
+                res
+            );
+        }
+
+        private static Entry createClone(
+            Snapshot snapshot,
+            State state,
+            Map<String, IndexId> indices,
+            long startTime,
+            long repositoryStateId,
+            String failure,
+            Version version,
+            SnapshotId source,
+            Map<RepositoryShardId, ShardSnapshotStatus> shardStatusByRepoShardId
+        ) {
+            return new Entry(
+                snapshot,
+                true,
+                false,
+                state,
+                indices,
+                List.of(),
+                List.of(),
+                startTime,
+                repositoryStateId,
+                Map.of(),
+                failure,
+                Map.of(),
+                version,
+                source,
+                shardStatusByRepoShardId,
                 Map.of()
             );
         }
@@ -741,7 +767,8 @@ public class SnapshotsInProgress extends AbstractNamedDiffable<Custom> implement
             Map<String, Object> userMetadata,
             Version version,
             @Nullable SnapshotId source,
-            Map<RepositoryShardId, ShardSnapshotStatus> shardStatusByRepoShardId
+            Map<RepositoryShardId, ShardSnapshotStatus> shardStatusByRepoShardId,
+            Map<String, Index> snapshotIndices
         ) {
             this.state = state;
             this.snapshot = snapshot;
@@ -757,26 +784,8 @@ public class SnapshotsInProgress extends AbstractNamedDiffable<Custom> implement
             this.userMetadata = userMetadata == null ? null : Map.copyOf(userMetadata);
             this.version = version;
             this.source = source;
-            if (source == null) {
-                assert shardStatusByRepoShardId == null || shardStatusByRepoShardId.isEmpty()
-                    : "Provided explict repo shard id statuses [" + shardStatusByRepoShardId + "] but no source";
-                final Map<String, Index> res = Maps.newMapWithExpectedSize(indices.size());
-                final Map<RepositoryShardId, ShardSnapshotStatus> byRepoShardIdBuilder = Maps.newHashMapWithExpectedSize(shards.size());
-                for (Map.Entry<ShardId, ShardSnapshotStatus> entry : shards.entrySet()) {
-                    final ShardId shardId = entry.getKey();
-                    final IndexId indexId = indices.get(shardId.getIndexName());
-                    final Index index = shardId.getIndex();
-                    final Index existing = res.put(indexId.getName(), index);
-                    assert existing == null || existing.equals(index) : "Conflicting indices [" + existing + "] and [" + index + "]";
-                    byRepoShardIdBuilder.put(new RepositoryShardId(indexId, shardId.id()), entry.getValue());
-                }
-                this.shardStatusByRepoShardId = Map.copyOf(byRepoShardIdBuilder);
-                this.snapshotIndices = Map.copyOf(res);
-            } else {
-                assert shards.isEmpty();
-                this.shardStatusByRepoShardId = shardStatusByRepoShardId;
-                this.snapshotIndices = Map.of();
-            }
+            this.shardStatusByRepoShardId = Map.copyOf(shardStatusByRepoShardId);
+            this.snapshotIndices = snapshotIndices;
             assert assertShardsConsistent(this.source, this.state, this.indices, this.shards, this.shardStatusByRepoShardId);
         }
 
@@ -809,24 +818,26 @@ public class SnapshotsInProgress extends AbstractNamedDiffable<Custom> implement
                 RepositoryShardId::new,
                 ShardSnapshotStatus::readFrom
             );
-            final List<SnapshotFeatureInfo> featureStates = Collections.unmodifiableList(in.readList(SnapshotFeatureInfo::new));
-            return new SnapshotsInProgress.Entry(
-                snapshot,
-                includeGlobalState,
-                partial,
-                state,
-                indices,
-                dataStreams,
-                featureStates,
-                startTime,
-                repositoryStateId,
-                shards,
-                failure,
-                userMetadata,
-                version,
-                source,
-                clones
-            );
+            final List<SnapshotFeatureInfo> featureStates = in.readImmutableList(SnapshotFeatureInfo::new);
+            if (source == null) {
+                return snapshot(
+                    snapshot,
+                    includeGlobalState,
+                    partial,
+                    state,
+                    indices,
+                    dataStreams,
+                    featureStates,
+                    startTime,
+                    repositoryStateId,
+                    shards,
+                    failure,
+                    userMetadata,
+                    version
+                );
+            }
+            assert shards.isEmpty();
+            return Entry.createClone(snapshot, state, indices, startTime, repositoryStateId, failure, version, source, clones);
         }
 
         private static boolean assertShardsConsistent(
@@ -889,7 +900,8 @@ public class SnapshotsInProgress extends AbstractNamedDiffable<Custom> implement
                 userMetadata,
                 version,
                 source,
-                source == null ? Map.of() : shardStatusByRepoShardId
+                shardStatusByRepoShardId,
+                snapshotIndices
             );
         }
 
@@ -922,7 +934,7 @@ public class SnapshotsInProgress extends AbstractNamedDiffable<Custom> implement
                 }
             }
             if (updatedIndices != null) {
-                return new Entry(
+                return snapshot(
                     snapshot,
                     includeGlobalState,
                     partial,
@@ -935,9 +947,7 @@ public class SnapshotsInProgress extends AbstractNamedDiffable<Custom> implement
                     shards,
                     failure,
                     userMetadata,
-                    version,
-                    source,
-                    Map.of()
+                    version
                 );
             }
             return this;
@@ -948,19 +958,13 @@ public class SnapshotsInProgress extends AbstractNamedDiffable<Custom> implement
                 return this;
             }
             assert shards.isEmpty();
-            return new Entry(
+            return Entry.createClone(
                 snapshot,
-                includeGlobalState,
-                partial,
                 completed(updatedClones.values()) ? (hasFailures(updatedClones) ? State.FAILED : State.SUCCESS) : state,
                 indices,
-                dataStreams,
-                featureStates,
                 startTime,
                 repositoryStateId,
-                Map.of(),
                 failure,
-                userMetadata,
                 version,
                 source,
                 updatedClones
@@ -1000,7 +1004,7 @@ public class SnapshotsInProgress extends AbstractNamedDiffable<Custom> implement
             if (allQueued) {
                 return null;
             }
-            return new Entry(
+            return Entry.snapshot(
                 snapshot,
                 includeGlobalState,
                 partial,
@@ -1013,9 +1017,7 @@ public class SnapshotsInProgress extends AbstractNamedDiffable<Custom> implement
                 Map.copyOf(shardsBuilder),
                 ABORTED_FAILURE_TEXT,
                 userMetadata,
-                version,
-                source,
-                Map.of()
+                version
             );
         }
 
@@ -1029,7 +1031,7 @@ public class SnapshotsInProgress extends AbstractNamedDiffable<Custom> implement
          */
         public Entry withShardStates(Map<ShardId, ShardSnapshotStatus> shards) {
             if (completed(shards.values())) {
-                return new Entry(
+                return Entry.snapshot(
                     snapshot,
                     includeGlobalState,
                     partial,
@@ -1053,7 +1055,7 @@ public class SnapshotsInProgress extends AbstractNamedDiffable<Custom> implement
          * shard snapshots on data nodes for a running snapshot.
          */
         public Entry withStartedShards(Map<ShardId, ShardSnapshotStatus> shards) {
-            final SnapshotsInProgress.Entry updated = new Entry(
+            final SnapshotsInProgress.Entry updated = Entry.snapshot(
                 snapshot,
                 includeGlobalState,
                 partial,

server/src/test/java/org/elasticsearch/cluster/metadata/MetadataDeleteIndexServiceTests.java

@@ -70,7 +70,7 @@ public class MetadataDeleteIndexServiceTests extends ESTestCase {
         String index = randomAlphaOfLength(5);
         Snapshot snapshot = new Snapshot("doesn't matter", new SnapshotId("snapshot name", "snapshot uuid"));
         SnapshotsInProgress snaps = SnapshotsInProgress.EMPTY.withAddedEntry(
-            new SnapshotsInProgress.Entry(
+            SnapshotsInProgress.Entry.snapshot(
                 snapshot,
                 true,
                 false,

server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexStateServiceTests.java

@@ -362,7 +362,7 @@ public class MetadataIndexStateServiceTests extends ESTestCase {
         }
 
         final Snapshot snapshot = new Snapshot(randomAlphaOfLength(10), new SnapshotId(randomAlphaOfLength(5), randomAlphaOfLength(5)));
-        final SnapshotsInProgress.Entry entry = new SnapshotsInProgress.Entry(
+        final SnapshotsInProgress.Entry entry = SnapshotsInProgress.Entry.snapshot(
             snapshot,
             randomBoolean(),
             false,

server/src/test/java/org/elasticsearch/snapshots/SnapshotsInProgressSerializationTests.java

@@ -80,7 +80,7 @@ public class SnapshotsInProgressSerializationTests extends SimpleDiffableWireSer
             }
         }
         List<SnapshotFeatureInfo> featureStates = randomList(5, SnapshotFeatureInfoTests::randomSnapshotFeatureInfo);
-        return new Entry(
+        return Entry.snapshot(
             snapshot,
             includeGlobalState,
             partial,
@@ -190,7 +190,7 @@ public class SnapshotsInProgressSerializationTests extends SimpleDiffableWireSer
         switch (randomInt(8)) {
             case 0 -> {
                 boolean includeGlobalState = entry.includeGlobalState() == false;
-                return new Entry(
+                return Entry.snapshot(
                     entry.snapshot(),
                     includeGlobalState,
                     entry.partial(),
@@ -208,7 +208,7 @@ public class SnapshotsInProgressSerializationTests extends SimpleDiffableWireSer
             }
             case 1 -> {
                 boolean partial = entry.partial() == false;
-                return new Entry(
+                return Entry.snapshot(
                     entry.snapshot(),
                     entry.includeGlobalState(),
                     partial,
@@ -226,7 +226,7 @@ public class SnapshotsInProgressSerializationTests extends SimpleDiffableWireSer
             }
             case 2 -> {
                 List<String> dataStreams = Stream.concat(entry.dataStreams().stream(), Stream.of(randomAlphaOfLength(10))).toList();
-                return new Entry(
+                return Entry.snapshot(
                     entry.snapshot(),
                     entry.includeGlobalState(),
                     entry.partial(),
@@ -244,7 +244,7 @@ public class SnapshotsInProgressSerializationTests extends SimpleDiffableWireSer
             }
             case 3 -> {
                 long startTime = randomValueOtherThan(entry.startTime(), ESTestCase::randomLong);
-                return new Entry(
+                return Entry.snapshot(
                     entry.snapshot(),
                     entry.includeGlobalState(),
                     entry.partial(),
@@ -262,7 +262,7 @@ public class SnapshotsInProgressSerializationTests extends SimpleDiffableWireSer
             }
             case 4 -> {
                 long repositoryStateId = randomValueOtherThan(entry.startTime(), ESTestCase::randomLong);
-                return new Entry(
+                return Entry.snapshot(
                     entry.snapshot(),
                     entry.includeGlobalState(),
                     entry.partial(),
@@ -280,7 +280,7 @@ public class SnapshotsInProgressSerializationTests extends SimpleDiffableWireSer
             }
             case 5 -> {
                 String failure = randomValueOtherThan(entry.failure(), () -> randomAlphaOfLengthBetween(2, 10));
-                return new Entry(
+                return Entry.snapshot(
                     entry.snapshot(),
                     entry.includeGlobalState(),
                     entry.partial(),
@@ -306,7 +306,7 @@ public class SnapshotsInProgressSerializationTests extends SimpleDiffableWireSer
                 for (int j = 0; j < shardsCount; j++) {
                     shards.put(new ShardId(index, j), randomShardSnapshotStatus(randomAlphaOfLength(10)));
                 }
-                return new Entry(
+                return Entry.snapshot(
                     entry.snapshot(),
                     entry.includeGlobalState(),
                     entry.partial(),
@@ -330,7 +330,7 @@ public class SnapshotsInProgressSerializationTests extends SimpleDiffableWireSer
                 } else {
                     userMetadata.put(key, randomAlphaOfLengthBetween(2, 10));
                 }
-                return new Entry(
+                return Entry.snapshot(
                     entry.snapshot(),
                     entry.includeGlobalState(),
                     entry.partial(),
@@ -352,7 +352,7 @@ public class SnapshotsInProgressSerializationTests extends SimpleDiffableWireSer
                     5,
                     () -> randomValueOtherThanMany(entry.featureStates()::contains, SnapshotFeatureInfoTests::randomSnapshotFeatureInfo)
                 );
-                return new Entry(
+                return Entry.snapshot(
                     entry.snapshot(),
                     entry.includeGlobalState(),
                     entry.partial(),
@@ -375,7 +375,7 @@ public class SnapshotsInProgressSerializationTests extends SimpleDiffableWireSer
     public void testXContent() throws IOException {
         final IndexId indexId = new IndexId("index", "uuid");
         SnapshotsInProgress sip = SnapshotsInProgress.EMPTY.withAddedEntry(
-            new Entry(
+            Entry.snapshot(
                 new Snapshot("repo", new SnapshotId("name", "uuid")),
                 true,
                 true,