From 0cc877026f8cdc43dc1a0d3801ea18a40e8a9603 Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Sat, 5 Jan 2019 10:15:33 +0100 Subject: [PATCH] Subclass NIOFSDirectory instead of using FileSwitchDirectory (#37140) We don't want two FSDirectories manage pending deletes separately and optimize file listing. This confuses IndexWriter and causes exceptions when files are deleted twice but are pending for deletion. This change move to using a NIOFS subclass that only delegates to MMAP for opening files all metadata and pending deletes are managed on top. Closes #37111 Relates to #36668 --- .../index/store/FsDirectoryService.java | 59 ++++++++++++++----- .../index/store/IndexStoreTests.java | 7 +-- 2 files changed, 47 insertions(+), 19 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/store/FsDirectoryService.java b/server/src/main/java/org/elasticsearch/index/store/FsDirectoryService.java index 7e6a440c525..ab93d175939 100644 --- a/server/src/main/java/org/elasticsearch/index/store/FsDirectoryService.java +++ b/server/src/main/java/org/elasticsearch/index/store/FsDirectoryService.java @@ -22,6 +22,8 @@ package org.elasticsearch.index.store; import org.apache.lucene.store.Directory; import org.apache.lucene.store.FSDirectory; import org.apache.lucene.store.FileSwitchDirectory; +import org.apache.lucene.store.IOContext; +import org.apache.lucene.store.IndexInput; import org.apache.lucene.store.LockFactory; import org.apache.lucene.store.MMapDirectory; import org.apache.lucene.store.NIOFSDirectory; @@ -31,7 +33,7 @@ import org.apache.lucene.store.SimpleFSLockFactory; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Setting.Property; -import org.elasticsearch.common.util.set.Sets; +import org.elasticsearch.core.internal.io.IOUtils; import org.elasticsearch.index.IndexModule; import org.elasticsearch.index.IndexSettings; import org.elasticsearch.index.shard.ShardPath; @@ -39,17 +41,10 @@ import org.elasticsearch.index.shard.ShardPath; import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; -import java.util.Collections; import java.util.HashSet; import java.util.Set; public class FsDirectoryService extends DirectoryService { - /* - * We are mmapping norms, docvalues as well as term dictionaries, all other files are served through NIOFS - * this provides good random access performance and does not lead to page cache thrashing. - */ - private static final Set PRIMARY_EXTENSIONS = Collections.unmodifiableSet(Sets.newHashSet("nvd", "dvd", "tim")); - protected final IndexStore indexStore; public static final Setting INDEX_LOCK_FACTOR_SETTING = new Setting<>("index.store.fs.fs_lock", "native", (s) -> { switch (s) { @@ -97,13 +92,7 @@ public class FsDirectoryService extends DirectoryService { // Use Lucene defaults final FSDirectory primaryDirectory = FSDirectory.open(location, lockFactory); if (primaryDirectory instanceof MMapDirectory) { - return new FileSwitchDirectory(PRIMARY_EXTENSIONS, primaryDirectory, new NIOFSDirectory(location, lockFactory), true) { - @Override - public String[] listAll() throws IOException { - // Avoid doing listAll twice: - return primaryDirectory.listAll(); - } - }; + return new HybridDirectory(location, lockFactory, primaryDirectory); } else { return primaryDirectory; } @@ -139,4 +128,44 @@ public class FsDirectoryService extends DirectoryService { } return directory; } + + static final class HybridDirectory extends NIOFSDirectory { + private final FSDirectory randomAccessDirectory; + + HybridDirectory(Path location, LockFactory lockFactory, FSDirectory randomAccessDirectory) throws IOException { + super(location, lockFactory); + this.randomAccessDirectory = randomAccessDirectory; + } + + @Override + public IndexInput openInput(String name, IOContext context) throws IOException { + String extension = FileSwitchDirectory.getExtension(name); + switch(extension) { + // We are mmapping norms, docvalues as well as term dictionaries, all other files are served through NIOFS + // this provides good random access performance and does not lead to page cache thrashing. + case "nvd": + case "dvd": + case "tim": + // we need to do these checks on the outer directory since the inner doesn't know about pending deletes + ensureOpen(); + ensureCanRead(name); + // we only use the mmap to open inputs. Everything else is managed by the NIOFSDirectory otherwise + // we might run into trouble with files that are pendingDelete in one directory but still + // listed in listAll() from the other. We on the other hand don't want to list files from both dirs + // and intersect for perf reasons. + return randomAccessDirectory.openInput(name, context); + default: + return super.openInput(name, context); + } + } + + @Override + public void close() throws IOException { + IOUtils.close(super::close, randomAccessDirectory); + } + + Directory getRandomAccessDirectory() { + return randomAccessDirectory; + } + } } diff --git a/server/src/test/java/org/elasticsearch/index/store/IndexStoreTests.java b/server/src/test/java/org/elasticsearch/index/store/IndexStoreTests.java index eb85219bcc2..4e22aad8d15 100644 --- a/server/src/test/java/org/elasticsearch/index/store/IndexStoreTests.java +++ b/server/src/test/java/org/elasticsearch/index/store/IndexStoreTests.java @@ -19,7 +19,6 @@ package org.elasticsearch.index.store; import org.apache.lucene.store.Directory; -import org.apache.lucene.store.FileSwitchDirectory; import org.apache.lucene.store.MMapDirectory; import org.apache.lucene.store.NIOFSDirectory; import org.apache.lucene.store.NoLockFactory; @@ -93,8 +92,8 @@ public class IndexStoreTests extends ESTestCase { } private void assertHybridDirectory(Directory directory) { - assertTrue(directory.toString(), directory instanceof FileSwitchDirectory); - Directory primaryDirectory = ((FileSwitchDirectory) directory).getPrimaryDir(); - assertTrue("primary directory " + primaryDirectory.toString(), primaryDirectory instanceof MMapDirectory); + assertTrue(directory.toString(), directory instanceof FsDirectoryService.HybridDirectory); + Directory randomAccessDirectory = ((FsDirectoryService.HybridDirectory) directory).getRandomAccessDirectory(); + assertTrue("randomAccessDirectory: " + randomAccessDirectory.toString(), randomAccessDirectory instanceof MMapDirectory); } }