Some Minor Cleanups in o.e.snapshots (#40962)

* Some Minor Cleanups in o.e.snapshots

* Some minor obvious cleanups that became possible now tht we're on JDK11
* Removing redundant `Updates` internal class

server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotResponse.java

@@ -77,7 +77,7 @@ public class RestoreSnapshotResponse extends ActionResponse implements ToXConten
         if (restoreInfo == null) {
             return RestStatus.ACCEPTED;
         }
-        return restoreInfo.status();
+        return RestStatus.OK;
     }
 
     @Override

server/src/main/java/org/elasticsearch/snapshots/RestoreInfo.java

@@ -27,7 +27,6 @@ import org.elasticsearch.common.xcontent.ObjectParser;
 import org.elasticsearch.common.xcontent.ToXContentObject;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentParser;
-import org.elasticsearch.rest.RestStatus;
 
 import java.io.IOException;
 import java.util.ArrayList;
@@ -51,7 +50,6 @@ public class RestoreInfo implements ToXContentObject, Streamable {
     private int successfulShards;
 
     RestoreInfo() {
-
     }
 
     public RestoreInfo(String name, List<String> indices, int totalShards, int successfulShards) {
@@ -106,15 +104,6 @@ public class RestoreInfo implements ToXContentObject, Streamable {
         return successfulShards;
     }
 
-    /**
-     * REST status of the operation
-     *
-     * @return REST status
-     */
-    public RestStatus status() {
-        return RestStatus.OK;
-    }
-
     static final class Fields {
         static final String SNAPSHOT = "snapshot";
         static final String INDICES = "indices";

server/src/main/java/org/elasticsearch/snapshots/RestoreService.java

@@ -163,7 +163,7 @@ public class RestoreService implements ClusterStateApplier {
         this.metaDataIndexUpgradeService = metaDataIndexUpgradeService;
         clusterService.addStateApplier(this);
         this.clusterSettings = clusterSettings;
-        this.cleanRestoreStateTaskExecutor = new CleanRestoreStateTaskExecutor(logger);
+        this.cleanRestoreStateTaskExecutor = new CleanRestoreStateTaskExecutor();
     }
 
     /**
@@ -221,7 +221,7 @@ public class RestoreService implements ClusterStateApplier {
             // Now we can start the actual restore process by adding shards to be recovered in the cluster state
             // and updating cluster metadata (global and index) as needed
             clusterService.submitStateUpdateTask("restore_snapshot[" + snapshotName + ']', new ClusterStateUpdateTask() {
-                String restoreUUID = UUIDs.randomBase64UUID();
+                final String restoreUUID = UUIDs.randomBase64UUID();
                 RestoreInfo restoreInfo = null;
 
                 @Override
@@ -354,7 +354,7 @@ public class RestoreService implements ClusterStateApplier {
                         shards = shardsBuilder.build();
                         RestoreInProgress.Entry restoreEntry = new RestoreInProgress.Entry(
                             restoreUUID, snapshot, overallState(RestoreInProgress.State.INIT, shards),
-                            Collections.unmodifiableList(new ArrayList<>(indices.keySet())),
+                            List.copyOf(indices.keySet()),
                             shards
                         );
                         RestoreInProgress.Builder restoreInProgressBuilder;
@@ -397,7 +397,7 @@ public class RestoreService implements ClusterStateApplier {
                     if (completed(shards)) {
                         // We don't have any indices to restore - we are done
                         restoreInfo = new RestoreInfo(snapshotId.getName(),
-                                                      Collections.unmodifiableList(new ArrayList<>(indices.keySet())),
+                                                      List.copyOf(indices.keySet()),
                                                       shards.size(),
                                                       shards.size() - failedShards(shards));
                     }
@@ -595,7 +595,8 @@ public class RestoreService implements ClusterStateApplier {
     }
 
     public static class RestoreInProgressUpdater extends RoutingChangesObserver.AbstractRoutingChangesObserver {
-        private final Map<String, Updates> shardChanges = new HashMap<>();
+        // Map of RestoreUUID to a of changes to the shards' restore statuses
+        private final Map<String, Map<ShardId, ShardRestoreStatus>> shardChanges = new HashMap<>();
 
         @Override
         public void shardStarted(ShardRouting initializingShard, ShardRouting startedShard) {
@@ -603,7 +604,7 @@ public class RestoreService implements ClusterStateApplier {
             if (initializingShard.primary()) {
                 RecoverySource recoverySource = initializingShard.recoverySource();
                 if (recoverySource.getType() == RecoverySource.Type.SNAPSHOT) {
-                    changes(recoverySource).shards.put(
+                    changes(recoverySource).put(
                         initializingShard.shardId(),
                         new ShardRestoreStatus(initializingShard.currentNodeId(), RestoreInProgress.State.SUCCESS));
                 }
@@ -619,7 +620,7 @@ public class RestoreService implements ClusterStateApplier {
                     // to restore this shard on another node if the snapshot files are corrupt. In case where a node just left or crashed,
                     // however, we only want to acknowledge the restore operation once it has been successfully restored on another node.
                     if (unassignedInfo.getFailure() != null && Lucene.isCorruptionException(unassignedInfo.getFailure().getCause())) {
-                        changes(recoverySource).shards.put(
+                        changes(recoverySource).put(
                             failedShard.shardId(), new ShardRestoreStatus(failedShard.currentNodeId(),
                                 RestoreInProgress.State.FAILURE, unassignedInfo.getFailure().getCause().getMessage()));
                     }
@@ -632,7 +633,7 @@ public class RestoreService implements ClusterStateApplier {
             // if we force an empty primary, we should also fail the restore entry
             if (unassignedShard.recoverySource().getType() == RecoverySource.Type.SNAPSHOT &&
                 initializedShard.recoverySource().getType() != RecoverySource.Type.SNAPSHOT) {
-                changes(unassignedShard.recoverySource()).shards.put(
+                changes(unassignedShard.recoverySource()).put(
                     unassignedShard.shardId(),
                     new ShardRestoreStatus(null, RestoreInProgress.State.FAILURE,
                         "recovery source type changed from snapshot to " + initializedShard.recoverySource())
@@ -646,7 +647,7 @@ public class RestoreService implements ClusterStateApplier {
             if (recoverySource.getType() == RecoverySource.Type.SNAPSHOT) {
                 if (newUnassignedInfo.getLastAllocationStatus() == UnassignedInfo.AllocationStatus.DECIDERS_NO) {
                     String reason = "shard could not be allocated to any of the nodes";
-                    changes(recoverySource).shards.put(
+                    changes(recoverySource).put(
                         unassignedShard.shardId(),
                         new ShardRestoreStatus(unassignedShard.currentNodeId(), RestoreInProgress.State.FAILURE, reason));
                 }
@@ -657,24 +658,20 @@ public class RestoreService implements ClusterStateApplier {
          * Helper method that creates update entry for the given recovery source's restore uuid
          * if such an entry does not exist yet.
          */
-        private Updates changes(RecoverySource recoverySource) {
+        private Map<ShardId, ShardRestoreStatus> changes(RecoverySource recoverySource) {
             assert recoverySource.getType() == RecoverySource.Type.SNAPSHOT;
-            return shardChanges.computeIfAbsent(((SnapshotRecoverySource) recoverySource).restoreUUID(), k -> new Updates());
-        }
-
-        private static class Updates {
-            private Map<ShardId, ShardRestoreStatus> shards = new HashMap<>();
+            return shardChanges.computeIfAbsent(((SnapshotRecoverySource) recoverySource).restoreUUID(), k -> new HashMap<>());
         }
 
         public RestoreInProgress applyChanges(final RestoreInProgress oldRestore) {
             if (shardChanges.isEmpty() == false) {
                 RestoreInProgress.Builder builder = new RestoreInProgress.Builder();
                 for (RestoreInProgress.Entry entry : oldRestore) {
-                    Updates updates = shardChanges.get(entry.uuid());
+                    Map<ShardId, ShardRestoreStatus> updates = shardChanges.get(entry.uuid());
                     ImmutableOpenMap<ShardId, ShardRestoreStatus> shardStates = entry.shards();
-                    if (updates != null && updates.shards.isEmpty() == false) {
+                    if (updates != null && updates.isEmpty() == false) {
                         ImmutableOpenMap.Builder<ShardId, ShardRestoreStatus> shardsBuilder = ImmutableOpenMap.builder(shardStates);
-                        for (Map.Entry<ShardId, ShardRestoreStatus> shard : updates.shards.entrySet()) {
+                        for (Map.Entry<ShardId, ShardRestoreStatus> shard : updates.entrySet()) {
                             ShardId shardId = shard.getKey();
                             ShardRestoreStatus status = shardStates.get(shardId);
                             if (status == null || status.state().completed() == false) {
@@ -720,14 +717,8 @@ public class RestoreService implements ClusterStateApplier {
             }
         }
 
-        private final Logger logger;
-
-        CleanRestoreStateTaskExecutor(Logger logger) {
-            this.logger = logger;
-        }
-
         @Override
-        public ClusterTasksResult<Task> execute(final ClusterState currentState, final List<Task> tasks) throws Exception {
+        public ClusterTasksResult<Task> execute(final ClusterState currentState, final List<Task> tasks) {
             final ClusterTasksResult.Builder<Task> resultBuilder = ClusterTasksResult.<Task>builder().successes(tasks);
             Set<String> completedRestores = tasks.stream().map(e -> e.uuid).collect(Collectors.toSet());
             RestoreInProgress.Builder restoreInProgressBuilder = new RestoreInProgress.Builder();
@@ -782,8 +773,8 @@ public class RestoreService implements ClusterStateApplier {
         }
     }
 
-    public static RestoreInProgress.State overallState(RestoreInProgress.State nonCompletedState,
-                                                       ImmutableOpenMap<ShardId, RestoreInProgress.ShardRestoreStatus> shards) {
+    private static RestoreInProgress.State overallState(RestoreInProgress.State nonCompletedState,
+                                                        ImmutableOpenMap<ShardId, RestoreInProgress.ShardRestoreStatus> shards) {
         boolean hasFailed = false;
         for (ObjectCursor<RestoreInProgress.ShardRestoreStatus> status : shards.values()) {
             if (!status.value.state().completed()) {
@@ -819,7 +810,7 @@ public class RestoreService implements ClusterStateApplier {
         return failedShards;
     }
 
-    private Map<String, String> renamedIndices(RestoreSnapshotRequest request, List<String> filteredIndices) {
+    private static Map<String, String> renamedIndices(RestoreSnapshotRequest request, List<String> filteredIndices) {
         Map<String, String> renamedIndices = new HashMap<>();
         for (String index : filteredIndices) {
             String renamedIndex = index;
@@ -841,7 +832,7 @@ public class RestoreService implements ClusterStateApplier {
      * @param repository      repository name
      * @param snapshotInfo    snapshot metadata
      */
-    private void validateSnapshotRestorable(final String repository, final SnapshotInfo snapshotInfo) {
+    private static void validateSnapshotRestorable(final String repository, final SnapshotInfo snapshotInfo) {
         if (!snapshotInfo.state().restorable()) {
             throw new SnapshotRestoreException(new Snapshot(repository, snapshotInfo.snapshotId()),
                                                "unsupported snapshot state [" + snapshotInfo.state() + "]");
@@ -853,7 +844,7 @@ public class RestoreService implements ClusterStateApplier {
         }
     }
 
-    private boolean failed(SnapshotInfo snapshot, String index) {
+    private static boolean failed(SnapshotInfo snapshot, String index) {
         for (SnapshotShardFailure failure : snapshot.shardFailures()) {
             if (index.equals(failure.index())) {
                 return true;

server/src/main/java/org/elasticsearch/snapshots/SnapshotException.java

@@ -58,11 +58,7 @@ public class SnapshotException extends ElasticsearchException {
     }
 
     public SnapshotException(final String repositoryName, final String snapshotName, final String msg) {
-        this(repositoryName, snapshotName, msg, null);
-    }
-
-    public SnapshotException(final String repositoryName, final String snapshotName, final String msg, final Throwable cause) {
-        super("[" + repositoryName + ":" + snapshotName + "] " + msg, cause);
+        super("[" + repositoryName + ":" + snapshotName + "] " + msg);
         this.repositoryName = repositoryName;
         this.snapshotName = snapshotName;
     }

server/src/main/java/org/elasticsearch/snapshots/SnapshotInfo.java

@@ -221,7 +221,7 @@ public final class SnapshotInfo implements Comparable<SnapshotInfo>, ToXContent,
     private final int successfulShards;
 
     @Nullable
-    private Boolean includeGlobalState;
+    private final Boolean includeGlobalState;
 
     @Nullable
     private final Version version;

server/src/main/java/org/elasticsearch/snapshots/SnapshotShardFailure.java

@@ -155,16 +155,6 @@ public class SnapshotShardFailure extends ShardOperationFailedException {
 
         ShardId shardId = new ShardId(index, indexUuid != null ? indexUuid : IndexMetaData.INDEX_UUID_NA_VALUE, intShardId);
 
-        // Workaround for https://github.com/elastic/elasticsearch/issues/25878
-        // Some old snapshot might still have null in shard failure reasons
-        String nonNullReason;
-        if (reason != null) {
-            nonNullReason = reason;
-        } else {
-            nonNullReason = "";
-        }
-
-
         RestStatus restStatus;
         if (status != null) {
             restStatus = RestStatus.valueOf(status);
@@ -172,7 +162,7 @@ public class SnapshotShardFailure extends ShardOperationFailedException {
             restStatus = RestStatus.INTERNAL_SERVER_ERROR;
         }
 
-        return new SnapshotShardFailure(nodeId, shardId, nonNullReason, restStatus);
+        return new SnapshotShardFailure(nodeId, shardId, reason, restStatus);
     }
 
     /**

server/src/main/java/org/elasticsearch/snapshots/SnapshotShardsService.java

@@ -492,7 +492,7 @@ public class SnapshotShardsService extends AbstractLifecycleComponent implements
     void sendSnapshotShardUpdate(final Snapshot snapshot, final ShardId shardId, final ShardSnapshotStatus status) {
         remoteFailedRequestDeduplicator.executeOnce(
             new UpdateIndexShardSnapshotStatusRequest(snapshot, shardId, status),
-            new ActionListener<Void>() {
+            new ActionListener<>() {
                 @Override
                 public void onResponse(Void aVoid) {
                     logger.trace("[{}] [{}] updated snapshot state", snapshot, status);
@@ -557,7 +557,7 @@ public class SnapshotShardsService extends AbstractLifecycleComponent implements
             });
     }
 
-    private class SnapshotStateExecutor implements ClusterStateTaskExecutor<UpdateIndexShardSnapshotStatusRequest> {
+    private static class SnapshotStateExecutor implements ClusterStateTaskExecutor<UpdateIndexShardSnapshotStatusRequest> {
 
         @Override
         public ClusterTasksResult<UpdateIndexShardSnapshotStatusRequest>

server/src/main/java/org/elasticsearch/snapshots/SnapshotUtils.java

@@ -23,9 +23,7 @@ import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
 import org.elasticsearch.common.regex.Regex;
 import org.elasticsearch.index.IndexNotFoundException;
 
-import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
@@ -116,8 +114,8 @@ public class SnapshotUtils {
             }
         }
         if (result == null) {
-            return Collections.unmodifiableList(new ArrayList<>(Arrays.asList(selectedIndices)));
+            return List.of(selectedIndices);
         }
-        return Collections.unmodifiableList(new ArrayList<>(result));
+        return List.copyOf(result);
     }
 }

server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java

@@ -312,7 +312,7 @@ public class SnapshotsService extends AbstractLifecycleComponent implements Clus
                 if (newSnapshot != null) {
                     final Snapshot current = newSnapshot.snapshot();
                     assert initializingSnapshots.contains(current);
-                    beginSnapshot(newState, newSnapshot, request.partial(), new ActionListener<Snapshot>() {
+                    beginSnapshot(newState, newSnapshot, request.partial(), new ActionListener<>() {
                         @Override
                         public void onResponse(final Snapshot snapshot) {
                             initializingSnapshots.remove(snapshot);
@@ -342,7 +342,7 @@ public class SnapshotsService extends AbstractLifecycleComponent implements Clus
      * @param snapshotName snapshot name
      * @param state   current cluster state
      */
-    private void validate(String repositoryName, String snapshotName, ClusterState state) {
+    private static void validate(String repositoryName, String snapshotName, ClusterState state) {
         RepositoriesMetaData repositoriesMetaData = state.getMetaData().custom(RepositoriesMetaData.TYPE);
         if (repositoriesMetaData == null || repositoriesMetaData.repository(repositoryName) == null) {
             throw new RepositoryMissingException(repositoryName);
@@ -797,7 +797,7 @@ public class SnapshotsService extends AbstractLifecycleComponent implements Clus
                         entries.add(updatedSnapshot);
 
                         // Clean up the snapshot that failed to start from the old master
-                        deleteSnapshot(snapshot.snapshot(), new ActionListener<Void>() {
+                        deleteSnapshot(snapshot.snapshot(), new ActionListener<>() {
                             @Override
                             public void onResponse(Void aVoid) {
                                 logger.debug("cleaned up abandoned snapshot {} in INIT state", snapshot.snapshot());
@@ -938,7 +938,7 @@ public class SnapshotsService extends AbstractLifecycleComponent implements Clus
      * @param shards list of shard statuses
      * @return list of failed and closed indices
      */
-    private Tuple<Set<String>, Set<String>> indicesWithMissingShards(
+    private static Tuple<Set<String>, Set<String>> indicesWithMissingShards(
         ImmutableOpenMap<ShardId, SnapshotsInProgress.ShardSnapshotStatus> shards, MetaData metaData) {
         Set<String> missing = new HashSet<>();
         Set<String> closed = new HashSet<>();
@@ -1140,7 +1140,7 @@ public class SnapshotsService extends AbstractLifecycleComponent implements Clus
             boolean waitForSnapshot = false;
 
             @Override
-            public ClusterState execute(ClusterState currentState) throws Exception {
+            public ClusterState execute(ClusterState currentState) {
                 SnapshotDeletionsInProgress deletionsInProgress = currentState.custom(SnapshotDeletionsInProgress.TYPE);
                 if (deletionsInProgress != null && deletionsInProgress.hasDeletionsInProgress()) {
                     throw new ConcurrentSnapshotExecutionException(snapshot,
@@ -1310,7 +1310,7 @@ public class SnapshotsService extends AbstractLifecycleComponent implements Clus
      * @param repositoryStateId the unique id representing the state of the repository at the time the deletion began
      */
     private void deleteSnapshotFromRepository(Snapshot snapshot, @Nullable ActionListener<Void> listener, long repositoryStateId) {
-        threadPool.executor(ThreadPool.Names.SNAPSHOT).execute(new ActionRunnable<Void>(listener) {
+        threadPool.executor(ThreadPool.Names.SNAPSHOT).execute(new ActionRunnable<>(listener) {
             @Override
             protected void doRun() {
                 Repository repository = repositoriesService.repository(snapshot.getRepository());

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

@@ -109,7 +109,6 @@ public class MetadataLoadingDuringSnapshotRestoreIT extends AbstractSnapshotInte
                                                                                     .setWaitForCompletion(true)
                                                                                     .get();
         assertThat(restoreSnapshotResponse.getRestoreInfo().failedShards(), equalTo(0));
-        assertThat(restoreSnapshotResponse.getRestoreInfo().status(), equalTo(RestStatus.OK));
         assertGlobalMetadataLoads("snap", 0);
         assertIndexMetadataLoads("snap", "docs", 2);
         assertIndexMetadataLoads("snap", "others", 2);
@@ -122,7 +121,6 @@ public class MetadataLoadingDuringSnapshotRestoreIT extends AbstractSnapshotInte
                                                             .setWaitForCompletion(true)
                                                             .get();
         assertThat(restoreSnapshotResponse.getRestoreInfo().failedShards(), equalTo(0));
-        assertThat(restoreSnapshotResponse.getRestoreInfo().status(), equalTo(RestStatus.OK));
         assertGlobalMetadataLoads("snap", 0);
         assertIndexMetadataLoads("snap", "docs", 3);
         assertIndexMetadataLoads("snap", "others", 2);
@@ -136,7 +134,6 @@ public class MetadataLoadingDuringSnapshotRestoreIT extends AbstractSnapshotInte
             .setWaitForCompletion(true)
             .get();
         assertThat(restoreSnapshotResponse.getRestoreInfo().failedShards(), equalTo(0));
-        assertThat(restoreSnapshotResponse.getRestoreInfo().status(), equalTo(RestStatus.OK));
         assertGlobalMetadataLoads("snap", 1);
         assertIndexMetadataLoads("snap", "docs", 4);
         assertIndexMetadataLoads("snap", "others", 3);