Fix code review issues

Author: Andrey Ershov

server/src/main/java/org/elasticsearch/gateway/GatewayMetaState.java

@@ -114,7 +114,6 @@ private void maybeUpgradeMetaData(MetaDataIndexUpgradeService metaDataIndexUpgra
                     List<Runnable> cleanupActions = new ArrayList<>();
                     // If globalStateGeneration is non-negative, it means we should have some metadata on disk
                     // Always re-write it even if upgrade plugins do not upgrade it, to be sure it's properly persisted on all data path
-                    Map<Index, Long> indices = new HashMap<>(metaState.getIndices());
                     final MetaData upgradedMetaData = upgradeMetaData(metaData, metaDataIndexUpgradeService, metaDataUpgrader);
 
                     if (MetaData.isGlobalStateEquals(metaData, upgradedMetaData) == false) {
@@ -125,6 +124,7 @@ private void maybeUpgradeMetaData(MetaDataIndexUpgradeService metaDataIndexUpgra
                     final long currentGlobalStateGeneration = globalStateGeneration;
                     cleanupActions.add(() -> metaStateService.cleanupGlobalState(currentGlobalStateGeneration));
 
+                    Map<Index, Long> indices = new HashMap<>();
                     for (IndexMetaData indexMetaData : upgradedMetaData) {
                         long generation;
                         if (metaData.hasIndexMetaData(indexMetaData) == false) {
@@ -143,7 +143,7 @@ private void maybeUpgradeMetaData(MetaDataIndexUpgradeService metaDataIndexUpgra
                     performCleanup(cleanupActions);
                 }
             } catch (Exception e) {
-                logger.error("failed to read local state, exiting...", e);
+                logger.error("failed to read or re-write local state, exiting...", e);
                 throw e;
             }
         }

server/src/main/java/org/elasticsearch/gateway/MetaDataStateFormat.java

@@ -209,7 +209,7 @@ public final long write(final T state, final Path... locations) throws WriteStat
         }
         assert maxStateId >= 0 : "maxStateId must be positive but was: [" + maxStateId + "]";
 
-        final String fileName = prefix + maxStateId + STATE_FILE_EXTENSION;
+        final String fileName = getStateFileName(maxStateId);
         final String tmpFileName = fileName + ".tmp";
         List<Tuple<Path, Directory>> directories = new ArrayList<>();
 
@@ -291,6 +291,9 @@ protected Directory newDirectory(Path dir) throws IOException {
         return new SimpleFSDirectory(dir);
     }
 
+    /**
+     * Whether to perform autoCleanup of old state files after successful {@link #write(Object, Path...)}.
+     */
     protected boolean autoCleanup() {
         return true;
     }
@@ -302,7 +305,7 @@ protected boolean autoCleanup() {
      * @param locations         state paths.
      */
     public void cleanupOldFiles(final long currentGeneration, Path[] locations) {
-        final String fileNameToKeep = prefix + currentGeneration + STATE_FILE_EXTENSION;
+        final String fileNameToKeep = getStateFileName(currentGeneration);
         for (Path location : locations) {
             logger.trace("cleanupOldFiles: cleaning up {}", location);
             Path stateLocation = location.resolve(STATE_DIR_NAME);
@@ -351,7 +354,7 @@ private List<Path> findStateFilesByGeneration(final long generation, Path... loc
             return files;
         }
 
-        final String fileName = prefix + generation + STATE_FILE_EXTENSION;
+        final String fileName = getStateFileName(generation);
         for (Path dataLocation : locations) {
             final Path stateFilePath = dataLocation.resolve(STATE_DIR_NAME).resolve(fileName);
             if (Files.exists(stateFilePath)) {
@@ -363,6 +366,10 @@ private List<Path> findStateFilesByGeneration(final long generation, Path... loc
         return files;
     }
 
+    private String getStateFileName(long generation) {
+        return prefix + generation + STATE_FILE_EXTENSION;
+    }
+
     /**
      * Tries to load the state of particular generation from the given data-locations. If any of data locations contain state files with
      * given generation, state will be loaded from these state files.

server/src/main/java/org/elasticsearch/gateway/MetaStateService.java

@@ -19,7 +19,6 @@
 
 package org.elasticsearch.gateway;
 
-import org.apache.logging.log4j.message.ParameterizedMessage;
 import org.elasticsearch.Version;
 import org.elasticsearch.cluster.metadata.IndexMetaData;
 import org.elasticsearch.cluster.metadata.MetaData;
@@ -56,7 +55,7 @@ public MetaStateService(Settings settings, NodeEnvironment nodeEnv, NamedXConten
      * Loads the full state, which includes both the global state and all the indices meta data. <br>
      * When loading, manifest file is consulted (represented by {@link MetaState} class), to load proper generations. <br>
      * If there is no manifest file on disk, this method fallbacks to BWC mode, where latest generation of global and indices
-     * metadata is loaded. Please note that currently where is no way to distinguish between manifest file being removed and manifest
+     * metadata is loaded. Please note that currently there is no way to distinguish between manifest file being removed and manifest
      * file was not yet created. It means that this method always fallbacks to BWC mode, if there is no manifest file.
      *
      * @return tuple of {@link MetaState} and {@link MetaData} with global metadata and indices metadata. If there is no state on disk,
@@ -66,12 +65,6 @@ public MetaStateService(Settings settings, NodeEnvironment nodeEnv, NamedXConten
     Tuple<MetaState, MetaData> loadFullState() throws IOException {
         final MetaState metaState = loadMetaState();
         if (metaState == null) {
-            MetaData globalMetaData =
-                    MetaData.FORMAT.loadLatestState(logger, namedXContentRegistry, nodeEnv.nodeDataPaths());
-            boolean isFreshStartup = globalMetaData == null;
-            if (isFreshStartup == false && Version.CURRENT.major >= 8) {
-                throw new IOException("failed to find manifest file, which is mandatory staring with ElasticSearch version 8.0");
-            }
             return loadFullStateBWC();
         }
         final MetaData.Builder metaDataBuilder;
@@ -107,6 +100,12 @@ private Tuple<MetaState, MetaData> loadFullStateBWC() throws IOException {
                 MetaData.FORMAT.loadLatestStateWithGeneration(logger, namedXContentRegistry, nodeEnv.nodeDataPaths());
         MetaData globalMetaData = metaDataAndGeneration.v1();
         long globalStateGeneration = metaDataAndGeneration.v2();
+        boolean isFreshStartup = globalMetaData == null;
+
+        if (isFreshStartup) {
+            assert Version.CURRENT.major < 8 : "failed to find manifest file, which is mandatory staring with Elasticsearch version 8.0";
+        }
+
         MetaData.Builder metaDataBuilder;
         if (globalMetaData != null) {
             metaDataBuilder = MetaData.builder(globalMetaData);
@@ -185,8 +184,7 @@ public long writeMetaState(String reason, MetaState metaState) throws WriteState
             logger.trace("[_meta] state written (generation: {})", generation);
             return generation;
         } catch (WriteStateException ex) {
-            logger.warn("[_meta]: failed to write meta state", ex);
-            throw ex;
+            throw new WriteStateException(ex.isDirty(), "[_meta]: failed to write meta state", ex);
         }
     }
 
@@ -207,9 +205,7 @@ public long writeIndex(String reason, IndexMetaData indexMetaData) throws WriteS
             logger.trace("[{}] state written", index);
             return generation;
         } catch (WriteStateException ex) {
-            logger.warn(() -> new ParameterizedMessage("[{}]: failed to write index state", index), ex);
-            ex.resetDirty();
-            throw ex;
+            throw new WriteStateException(false, "[" + index + "]: failed to write index state", ex);
         }
     }
 
@@ -226,9 +222,7 @@ long writeGlobalState(String reason, MetaData metaData) throws WriteStateExcepti
             logger.trace("[_global] state written");
             return generation;
         } catch (WriteStateException ex) {
-            logger.warn("[_global]: failed to write global state", ex);
-            ex.resetDirty();
-            throw ex;
+            throw new WriteStateException(false, "[_global]: failed to write global state", ex);
         }
     }
 

server/src/main/java/org/elasticsearch/gateway/WriteStateException.java

@@ -24,7 +24,7 @@
  * This exception is thrown when there is a problem of writing state to disk.
  */
 public class WriteStateException extends IOException {
-    private boolean dirty;
+    private final boolean dirty;
 
     WriteStateException(boolean dirty, String message, Exception cause) {
         super(message, cause);
@@ -38,8 +38,4 @@ public class WriteStateException extends IOException {
     public boolean isDirty() {
         return dirty;
     }
-
-    public void resetDirty() {
-        this.dirty = false;
-    }
 }