Skip to content
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

Merged
merged 4 commits into from
Jan 2, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions docs/reference/index-modules/store.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ The following sections lists all the different storage types supported.
`fs`::

Default file system implementation. This will pick the best implementation
depending on the operating environment, which is currently `mmapfs` on all
depending on the operating environment, which is currently `hybridfs` on all
supported systems but is subject to change.

[[simplefs]]`simplefs`::
Expand All @@ -67,12 +67,22 @@ process equal to the size of the file being mapped. Before using this
class, be sure you have allowed plenty of
<<vm-max-map-count,virtual address space>>.

[[hybridfs]]`hybridfs`::

The `hybridfs` type is a hybrid of `niofs` and `mmapfs`, which chooses the best
file system type for each type of file based on the read access pattern.
Currently only the Lucene term dictionary, norms and doc values files are
memory mapped. All other files are opened using Lucene `NIOFSDirectory`.
Similarly to `mmapfs` be sure you have allowed plenty of
<<vm-max-map-count,virtual address space>>.

[[allow-mmapfs]]
Copy link
Member Author

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).

Copy link
Member Author

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 to node.store.allow_mmap.

Copy link
Member Author

@danielmitterdorfer danielmitterdorfer Jan 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have opened #37070.

You can restrict the use of the `mmapfs` store type via the setting
`node.store.allow_mmapfs`. This is a boolean setting indicating whether or not
`mmapfs` is allowed. The default is to allow `mmapfs`. This setting is useful,
for example, if you are in an environment where you can not control the ability
to create a lot of memory maps so you need disable the ability to use `mmapfs`.
You can restrict the use of the `mmapfs` and the related `hybridfs` store type
via the setting `node.store.allow_mmapfs`. This is a boolean setting indicating
whether or not memory-mapping is allowed. The default is to allow it. This
setting is useful, for example, if you are in an environment where you can not
control the ability to create a lot of memory maps so you need disable the
ability to use memory-mapping.

=== Pre-loading data into the file system cache

Expand Down
5 changes: 3 additions & 2 deletions server/src/main/java/org/elasticsearch/index/IndexModule.java
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,7 @@ public static boolean isBuiltinType(String storeType) {


public enum Type {
HYBRIDFS("hybridfs"),
NIOFS("niofs"),
MMAPFS("mmapfs"),
SIMPLEFS("simplefs"),
Expand Down Expand Up @@ -330,7 +331,7 @@ public String getSettingsKey() {
public static Type fromSettingsKey(final String key) {
final Type type = TYPES.get(key);
if (type == null) {
throw new IllegalArgumentException("no matching type for [" + key + "]");
throw new IllegalArgumentException("no matching store type for [" + key + "]");
}
return type;
}
Expand All @@ -356,7 +357,7 @@ public interface IndexSearcherWrapperFactory {

public static Type defaultStoreType(final boolean allowMmapfs) {
if (allowMmapfs && Constants.JRE_IS_64BIT && MMapDirectory.UNMAP_SUPPORTED) {
return Type.MMAPFS;
return Type.HYBRIDFS;
} else if (Constants.WINDOWS) {
return Type.SIMPLEFS;
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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) -> {
Expand Down Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
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;
Expand Down Expand Up @@ -64,6 +65,9 @@ private void doTestStoreDirectory(Index index, Path tempDir, String typeSettingV
new ShardPath(false, tempDir, tempDir, new ShardId(index, 0)));
try (Directory directory = service.newFSDirectory(tempDir, NoLockFactory.INSTANCE)) {
switch (type) {
case HYBRIDFS:
assertTrue(type + " " + directory.toString(), directory instanceof FileSwitchDirectory);
break;
case NIOFS:
assertTrue(type + " " + directory.toString(), directory instanceof NIOFSDirectory);
break;
Expand All @@ -75,7 +79,7 @@ private void doTestStoreDirectory(Index index, Path tempDir, String typeSettingV
break;
case FS:
if (Constants.JRE_IS_64BIT && MMapDirectory.UNMAP_SUPPORTED) {
assertTrue(directory.toString(), directory instanceof MMapDirectory);
assertTrue(directory.toString(), directory instanceof FileSwitchDirectory);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the two above cases, maybe also check that directory.getPrimaryDir() is a MMapDirectory?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added an additional assertion.

} else if (Constants.WINDOWS) {
assertTrue(directory.toString(), directory instanceof SimpleFSDirectory);
} else {
Expand Down