-
Notifications
You must be signed in to change notification settings - Fork 24.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add hybridfs store type #36668
Add hybridfs store type #36668
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ | |
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.LockFactory; | ||
import org.apache.lucene.store.MMapDirectory; | ||
|
@@ -30,17 +31,25 @@ | |
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.index.IndexModule; | ||
import org.elasticsearch.index.IndexSettings; | ||
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 while not creating unnecessary mmaps for files like stored | ||
* fields etc. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's mention FS cache usage rather than the number of mmaps as a reason for this hybrid store type? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just restored the original comment but that makes sense to me. I'll update the comment accordingly. |
||
*/ | ||
private static final Set<String> PRIMARY_EXTENSIONS = Collections.unmodifiableSet(Sets.newHashSet("nvd", "dvd", "tim")); | ||
|
||
protected final IndexStore indexStore; | ||
public static final Setting<LockFactory> INDEX_LOCK_FACTOR_SETTING = new Setting<>("index.store.fs.fs_lock", "native", (s) -> { | ||
|
@@ -78,27 +87,36 @@ public Directory newDirectory() throws IOException { | |
protected Directory newFSDirectory(Path location, LockFactory lockFactory) throws IOException { | ||
final String storeType = | ||
indexSettings.getSettings().get(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), IndexModule.Type.FS.getSettingsKey()); | ||
IndexModule.Type type; | ||
if (IndexModule.Type.FS.match(storeType)) { | ||
final IndexModule.Type type = | ||
IndexModule.defaultStoreType(IndexModule.NODE_STORE_ALLOW_MMAPFS.get(indexSettings.getNodeSettings())); | ||
switch (type) { | ||
case MMAPFS: | ||
return new MMapDirectory(location, lockFactory); | ||
case SIMPLEFS: | ||
return new SimpleFSDirectory(location, lockFactory); | ||
case NIOFS: | ||
return new NIOFSDirectory(location, lockFactory); | ||
default: | ||
throw new AssertionError("unexpected built-in store type [" + type + "]"); | ||
} | ||
} else if (IndexModule.Type.SIMPLEFS.match(storeType)) { | ||
return new SimpleFSDirectory(location, lockFactory); | ||
} else if (IndexModule.Type.NIOFS.match(storeType)) { | ||
return new NIOFSDirectory(location, lockFactory); | ||
} else if (IndexModule.Type.MMAPFS.match(storeType)) { | ||
return new MMapDirectory(location, lockFactory); | ||
type = IndexModule.defaultStoreType(IndexModule.NODE_STORE_ALLOW_MMAPFS.get(indexSettings.getNodeSettings())); | ||
} else { | ||
type = IndexModule.Type.fromSettingsKey(storeType); | ||
} | ||
switch (type) { | ||
case HYBRIDFS: | ||
// 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(); | ||
} | ||
}; | ||
} else { | ||
return primaryDirectory; | ||
} | ||
case MMAPFS: | ||
return new MMapDirectory(location, lockFactory); | ||
case SIMPLEFS: | ||
return new SimpleFSDirectory(location, lockFactory); | ||
case NIOFS: | ||
return new NIOFSDirectory(location, lockFactory); | ||
default: | ||
throw new AssertionError("unexpected built-in store type [" + type + "]"); | ||
} | ||
throw new IllegalArgumentException("No directory found for type [" + storeType + "]"); | ||
} | ||
|
||
private static Directory setPreload(Directory directory, Path location, LockFactory lockFactory, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO we should rename this setting in order to decouple it from the store type, e.g.
node.store.allow_mmap
. The only complication is when we backport this to 6.x because changing a property name would be a breaking change (and that's the reason I did not rename it yet).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jpountz and me discussed this and we'll open a separate PR to deprecate
node.store.allow_mmapfs
and rename it tonode.store.allow_mmap
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have opened #37070.