Make translog file name parsing strict

Today we are very lenient in parsing the translog files. This is
actually not necessary since we have a clear run once upgrade path.
All files are converted into the new file name pattern such that we
only need to look at old file patterns in the context of the upgrade.

This commit makes parsing really strict with the exceptoin of the upgrade path.

core/src/main/java/org/elasticsearch/index/translog/Translog.java

@@ -104,7 +104,7 @@ public class Translog extends AbstractIndexShardComponent implements IndexShardC
     public static final String CHECKPOINT_SUFFIX = ".ckp";
     public static final String CHECKPOINT_FILE_NAME = "translog" + CHECKPOINT_SUFFIX;
 
-    static final Pattern PARSE_ID_PATTERN = Pattern.compile("^" + TRANSLOG_FILE_PREFIX + "(\\d+)((\\.recovering)|(\\.tlog))?$");
+    static final Pattern PARSE_STRICT_ID_PATTERN = Pattern.compile("^" + TRANSLOG_FILE_PREFIX + "(\\d+)(\\.tlog)$");
 
     private final List<ImmutableTranslogReader> recoveredTranslogs;
     private volatile ScheduledFuture<?> syncScheduler;
@@ -217,24 +217,34 @@ public class Translog extends AbstractIndexShardComponent implements IndexShardC
         } catch (NoSuchFileException | FileNotFoundException ex) {
             logger.debug("upgrading translog - no checkpoint found");
         }
-
+        final Pattern parseLegacyIdPattern = Pattern.compile("^" + TRANSLOG_FILE_PREFIX + "(\\d+)((\\.recovering))?$"); // here we have to be lenient - nowhere else!
         try (DirectoryStream<Path> stream = Files.newDirectoryStream(translogPath, new DirectoryStream.Filter<Path>() {
             @Override
             public boolean accept(Path entry) throws IOException {
-                Matcher matcher = PARSE_ID_PATTERN.matcher(entry.getFileName().toString());
+                Matcher matcher = parseLegacyIdPattern.matcher(entry.getFileName().toString());
-                return matcher.matches();
+                if (matcher.matches() == false) {
+                    Matcher newIdMatcher = PARSE_STRICT_ID_PATTERN.matcher(entry.getFileName().toString());
+                    return newIdMatcher.matches();
+                } else {
+                    return true;
+                }
             }
         })) {
             long latestGeneration = -1;
             List<Tuple<Path, Long>> filesToUpgrade = new ArrayList<>();
             for (Path path : stream) {
-                Matcher matcher = PARSE_ID_PATTERN.matcher(path.getFileName().toString());
+                Matcher matcher = parseLegacyIdPattern.matcher(path.getFileName().toString());
                 if (matcher.matches()) {
                     long generation = Long.parseLong(matcher.group(1));
                     if (generation >= translogGeneration.translogFileGeneration) {
                         latestGeneration = Math.max(translogGeneration.translogFileGeneration, generation);
                     }
                     filesToUpgrade.add(new Tuple<>(path, generation));
+                } else {
+                    Matcher strict_matcher = PARSE_STRICT_ID_PATTERN.matcher(path.getFileName().toString());
+                    if (strict_matcher.matches()) {
+                        throw new IllegalStateException("non-legacy translog file [" + path.getFileName().toString() + "] found on a translog that wasn't upgraded yet");
+                    }
                 }
             }
             if (latestGeneration < translogGeneration.translogFileGeneration) {
@@ -302,9 +312,11 @@ public class Translog extends AbstractIndexShardComponent implements IndexShardC
     }
 
     ImmutableTranslogReader openReader(Path path, Checkpoint checkpoint) throws IOException {
-        final long generation = parseIdFromFileName(path);
+        final long generation;
-        if (generation < 0) {
+        try {
-            throw new TranslogException(shardId, "failed to parse generation from file name matching pattern " + path);
+            generation = parseIdFromFileName(path);
+        } catch (IllegalArgumentException ex) {
+            throw new TranslogException(shardId, "failed to parse generation from file name matching pattern " + path, ex);
         }
         FileChannel channel = FileChannel.open(path, StandardOpenOption.READ);
         try {
@@ -317,18 +329,22 @@ public class Translog extends AbstractIndexShardComponent implements IndexShardC
         }
     }
 
-    /* extracts the translog generation from a file name. returns -1 upon failure */
+    /**
+     * Extracts the translog generation from a file name.
+     *
+     * @throw IllegalArgumentException if the path doesn't match the expected pattern.
+     */
     public static long parseIdFromFileName(Path translogFile) {
         final String fileName = translogFile.getFileName().toString();
-        final Matcher matcher = PARSE_ID_PATTERN.matcher(fileName);
+        final Matcher matcher = PARSE_STRICT_ID_PATTERN.matcher(fileName);
         if (matcher.matches()) {
             try {
                 return Long.parseLong(matcher.group(1));
             } catch (NumberFormatException e) {
-                throw new ElasticsearchException("number formatting issue in a file that passed PARSE_ID_PATTERN: " + fileName + "]", e);
+                throw new IllegalStateException("number formatting issue in a file that passed PARSE_STRICT_ID_PATTERN: " + fileName + "]", e);
             }
         }
-        return -1;
+        throw new IllegalArgumentException("can't parse id from file: " + fileName);
     }
 
     public void updateBuffer(ByteSizeValue bufferSize) {
@@ -617,7 +633,7 @@ public class Translog extends AbstractIndexShardComponent implements IndexShardC
                 }
                 try (DirectoryStream<Path> stream = Files.newDirectoryStream(location)) {
                     for (Path path : stream) {
-                        Matcher matcher = PARSE_ID_PATTERN.matcher(path.getFileName().toString());
+                        Matcher matcher = PARSE_STRICT_ID_PATTERN.matcher(path.getFileName().toString());
                         if (matcher.matches()) {
                             long generation = Long.parseLong(matcher.group(1));
                             if (isReferencedGeneration(generation) == false) {

core/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java

@@ -77,6 +77,7 @@ import org.elasticsearch.index.store.DirectoryUtils;
 import org.elasticsearch.index.store.Store;
 import org.elasticsearch.index.translog.Translog;
 import org.elasticsearch.index.translog.TranslogConfig;
+import org.elasticsearch.index.translog.TranslogTests;
 import org.elasticsearch.test.DummyShardLock;
 import org.elasticsearch.test.ElasticsearchTestCase;
 import org.elasticsearch.threadpool.ThreadPool;
@@ -94,6 +95,7 @@ import java.nio.file.Path;
 import java.util.*;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.regex.Pattern;
 
 import static org.elasticsearch.common.settings.Settings.Builder.EMPTY_SETTINGS;
 import static org.elasticsearch.index.engine.Engine.Operation.Origin.PRIMARY;
@@ -101,6 +103,8 @@ import static org.elasticsearch.index.engine.Engine.Operation.Origin.REPLICA;
 import static org.hamcrest.Matchers.*;
 public class InternalEngineTests extends ElasticsearchTestCase {
 
+    private static final Pattern PARSE_LEGACY_ID_PATTERN = Pattern.compile("^" + Translog.TRANSLOG_FILE_PREFIX + "(\\d+)((\\.recovering))?$");
+
     protected final ShardId shardId = new ShardId(new Index("index"), 1);
 
     protected ThreadPool threadPool;
@@ -1753,7 +1757,7 @@ public class InternalEngineTests extends ElasticsearchTestCase {
             assertEquals(Arrays.toString(tlogFiles), tlogFiles.length, 1);
             final long size = Files.size(tlogFiles[0]);
 
-            final long generation = Translog.parseIdFromFileName(tlogFiles[0]);
+            final long generation = TranslogTests.parseLegacyTranslogFile(tlogFiles[0]);
             assertTrue(generation >= 1);
             logger.debug("upgrading index {} file: {} size: {}", indexName, tlogFiles[0].getFileName(), size);
             Directory directory = newFSDirectory(src.resolve("0").resolve("index"));

core/src/test/java/org/elasticsearch/index/translog/TranslogTests.java

@@ -60,6 +60,8 @@ import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 
 import static com.google.common.collect.Lists.newArrayList;
 import static org.hamcrest.Matchers.*;
@@ -70,6 +72,8 @@ import static org.hamcrest.Matchers.*;
 @LuceneTestCase.SuppressFileSystems("ExtrasFS")
 public class TranslogTests extends ElasticsearchTestCase {
 
+    private static final Pattern PARSE_LEGACY_ID_PATTERN = Pattern.compile("^" + Translog.TRANSLOG_FILE_PREFIX + "(\\d+)((\\.recovering))?$");
+
     protected final ShardId shardId = new ShardId(new Index("index"), 1);
 
     protected Translog translog;
@@ -126,17 +130,40 @@ public class TranslogTests extends ElasticsearchTestCase {
 
     public void testIdParsingFromFile() {
         long id = randomIntBetween(0, Integer.MAX_VALUE);
-        Path file = translogDir.resolve(Translog.TRANSLOG_FILE_PREFIX + id);
+        Path file = translogDir.resolve(Translog.TRANSLOG_FILE_PREFIX + id + ".tlog");
         assertThat(Translog.parseIdFromFileName(file), equalTo(id));
 
+        id = randomIntBetween(0, Integer.MAX_VALUE);
+        file = translogDir.resolve(Translog.TRANSLOG_FILE_PREFIX + id);
+        try {
+            Translog.parseIdFromFileName(file);
+            fail("invalid pattern");
+        } catch (IllegalArgumentException ex) {
+            // all good
+        }
+
         file = translogDir.resolve(Translog.TRANSLOG_FILE_PREFIX + id + ".recovering");
-        assertThat(Translog.parseIdFromFileName(file), equalTo(id));
+        try {
+            Translog.parseIdFromFileName(file);
+            fail("invalid pattern");
+        } catch (IllegalArgumentException ex) {
+            // all good
+        }
 
         file = translogDir.resolve(Translog.TRANSLOG_FILE_PREFIX + randomNonTranslogPatternString(1, 10) + id);
-        assertThat(Translog.parseIdFromFileName(file), equalTo(-1l));
+        try {
-
+            Translog.parseIdFromFileName(file);
+            fail("invalid pattern");
+        } catch (IllegalArgumentException ex) {
+            // all good
+        }
         file = translogDir.resolve(randomNonTranslogPatternString(1, Translog.TRANSLOG_FILE_PREFIX.length() - 1));
-        assertThat(Translog.parseIdFromFileName(file), equalTo(-1l));
+        try {
+            Translog.parseIdFromFileName(file);
+            fail("invalid pattern");
+        } catch (IllegalArgumentException ex) {
+            // all good
+        }
     }
 
     private String randomNonTranslogPatternString(int min, int max) {
@@ -151,7 +178,7 @@ public class TranslogTests extends ElasticsearchTestCase {
             } catch (InvalidPathException ex) {
                 // some FS don't like our random file names -- let's just skip these random choices
             }
-        } while (Translog.PARSE_ID_PATTERN.matcher(string).matches() || validPathString == false);
+        } while (Translog.PARSE_STRICT_ID_PATTERN.matcher(string).matches() || validPathString == false);
         return string;
     }
 
@@ -1166,7 +1193,7 @@ public class TranslogTests extends ElasticsearchTestCase {
             assertEquals(tlogFiles.length, 1);
             final long size = Files.size(tlogFiles[0]);
 
-            final long generation = Translog.parseIdFromFileName(tlogFiles[0]);
+            final long generation = parseLegacyTranslogFile(tlogFiles[0]);
             assertTrue(generation >= 1);
             logger.debug("upgrading index {} file: {} size: {}", indexName, tlogFiles[0].getFileName(), size);
             TranslogConfig upgradeConfig = new TranslogConfig(config.getShardId(), translog, config.getIndexSettings(), config.getDurabilty(), config.getBigArrays(), config.getThreadPool());
@@ -1226,7 +1253,7 @@ public class TranslogTests extends ElasticsearchTestCase {
                 } catch (IllegalArgumentException ex) {
                     // expected
                 }
-                long generation = Translog.parseIdFromFileName(legacyTranslog);
+                long generation = parseLegacyTranslogFile(legacyTranslog);
                 upgradeConfig.setTranslogGeneration(new Translog.TranslogGeneration(null, generation));
                 Translog.upgradeLegacyTranslog(logger, upgradeConfig);
                 try (Translog tlog = new Translog(upgradeConfig)) {
@@ -1243,4 +1270,17 @@ public class TranslogTests extends ElasticsearchTestCase {
             }
         }
     }
+
+    public static long parseLegacyTranslogFile(Path translogFile) {
+        final String fileName = translogFile.getFileName().toString();
+        final Matcher matcher = PARSE_LEGACY_ID_PATTERN.matcher(fileName);
+        if (matcher.matches()) {
+            try {
+                return Long.parseLong(matcher.group(1));
+            } catch (NumberFormatException e) {
+                throw new IllegalStateException("number formatting issue in a file that passed PARSE_STRICT_ID_PATTERN: " + fileName + "]", e);
+            }
+        }
+        throw new IllegalArgumentException("can't parse id from file: " + fileName);
+    }
 }