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

[DEPRECATE] SimpleFS in favor of NIOFS #1073

Merged
merged 5 commits into from
Aug 19, 2021
Merged
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
import org.apache.lucene.codecs.CodecUtil;
import org.apache.lucene.store.IOContext;
import org.apache.lucene.store.IndexOutput;
import org.apache.lucene.store.SimpleFSDirectory;
import org.apache.lucene.store.NIOFSDirectory;
import org.opensearch.common.Randomness;
import org.opensearch.core.internal.io.IOUtils;
import org.opensearch.env.Environment;
Expand Down Expand Up @@ -192,7 +192,7 @@ public void testUpgradeNoop() throws Exception {

public void testFailWhenCannotConsumeSecretStream() throws Exception {
Path configDir = env.configFile();
SimpleFSDirectory directory = new SimpleFSDirectory(configDir);
NIOFSDirectory directory = new NIOFSDirectory(configDir);
try (IndexOutput indexOutput = directory.createOutput("opensearch.keystore", IOContext.DEFAULT)) {
CodecUtil.writeHeader(indexOutput, "opensearch.keystore", 3);
indexOutput.writeByte((byte) 0); // No password
Expand Down Expand Up @@ -220,7 +220,7 @@ public void testFailWhenCannotConsumeSecretStream() throws Exception {

public void testFailWhenCannotConsumeEncryptedBytesStream() throws Exception {
Path configDir = env.configFile();
SimpleFSDirectory directory = new SimpleFSDirectory(configDir);
NIOFSDirectory directory = new NIOFSDirectory(configDir);
try (IndexOutput indexOutput = directory.createOutput("opensearch.keystore", IOContext.DEFAULT)) {
CodecUtil.writeHeader(indexOutput, "opensearch.keystore", 3);
indexOutput.writeByte((byte) 0); // No password
Expand Down Expand Up @@ -249,7 +249,7 @@ public void testFailWhenCannotConsumeEncryptedBytesStream() throws Exception {

public void testFailWhenSecretStreamNotConsumed() throws Exception {
Path configDir = env.configFile();
SimpleFSDirectory directory = new SimpleFSDirectory(configDir);
NIOFSDirectory directory = new NIOFSDirectory(configDir);
try (IndexOutput indexOutput = directory.createOutput("opensearch.keystore", IOContext.DEFAULT)) {
CodecUtil.writeHeader(indexOutput, "opensearch.keystore", 3);
indexOutput.writeByte((byte) 0); // No password
Expand All @@ -276,7 +276,7 @@ public void testFailWhenSecretStreamNotConsumed() throws Exception {

public void testFailWhenEncryptedBytesStreamIsNotConsumed() throws Exception {
Path configDir = env.configFile();
SimpleFSDirectory directory = new SimpleFSDirectory(configDir);
NIOFSDirectory directory = new NIOFSDirectory(configDir);
try (IndexOutput indexOutput = directory.createOutput("opensearch.keystore", IOContext.DEFAULT)) {
CodecUtil.writeHeader(indexOutput, "opensearch.keystore", 3);
indexOutput.writeByte((byte) 0); // No password
Expand Down Expand Up @@ -362,7 +362,7 @@ public void testIllegalSettingName() throws Exception {
public void testBackcompatV1() throws Exception {
assumeFalse("Can't run in a FIPS JVM as PBE is not available", inFipsJvm());
Path configDir = env.configFile();
SimpleFSDirectory directory = new SimpleFSDirectory(configDir);
NIOFSDirectory directory = new NIOFSDirectory(configDir);
try (IndexOutput output = directory.createOutput("opensearch.keystore", IOContext.DEFAULT)) {
CodecUtil.writeHeader(output, "opensearch.keystore", 1);
output.writeByte((byte) 0); // hasPassword = false
Expand Down Expand Up @@ -393,7 +393,7 @@ public void testBackcompatV1() throws Exception {
public void testBackcompatV2() throws Exception {
assumeFalse("Can't run in a FIPS JVM as PBE is not available", inFipsJvm());
Path configDir = env.configFile();
SimpleFSDirectory directory = new SimpleFSDirectory(configDir);
NIOFSDirectory directory = new NIOFSDirectory(configDir);
byte[] fileBytes = new byte[20];
random().nextBytes(fileBytes);
try (IndexOutput output = directory.createOutput("opensearch.keystore", IOContext.DEFAULT)) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*/

package org.opensearch.index.store;

import org.opensearch.common.settings.Settings;

/**
* Index Settings Tests for NIO FileSystem as index store type.
*/
public class SmbNIOFsTests extends AbstractAzureFsTestCase {
@Override
public Settings indexSettings() {
return Settings.builder()
.put(super.indexSettings())
.put("index.store.type", "smb_nio_fs")
.build();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*/

package org.opensearch.index.store.smbniofs;
Copy link
Contributor

@adnapibar adnapibar Aug 18, 2021

Choose a reason for hiding this comment

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

Can the implementations SmbMmapFsDirectoryFactory, SmbNIOFsDirectoryFactory and SmbSimpleFsDirectoryFactory be in the same package something like smbfs or simply smb ? Similarly for all the test classes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I agree. I think maybe we do that refactor in a followup PR? How about in the removal PR?


import org.apache.lucene.store.Directory;
import org.apache.lucene.store.LockFactory;
import org.apache.lucene.store.NIOFSDirectory;
import org.opensearch.index.IndexSettings;
import org.opensearch.index.store.FsDirectoryFactory;
import org.opensearch.index.store.SmbDirectoryWrapper;

import java.io.IOException;
import java.nio.file.Path;

/**
* Factory to create a new NIO File System type directory accessible as a SMB share
*/
public final class SmbNIOFsDirectoryFactory extends FsDirectoryFactory {

@Override
protected Directory newFSDirectory(Path location, LockFactory lockFactory, IndexSettings indexSettings) throws IOException {
return new SmbDirectoryWrapper(new NIOFSDirectory(location, lockFactory));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,17 +35,29 @@
import org.apache.lucene.store.Directory;
import org.apache.lucene.store.LockFactory;
import org.apache.lucene.store.SimpleFSDirectory;
import org.opensearch.common.logging.DeprecationLogger;
import org.opensearch.index.IndexModule;
import org.opensearch.index.IndexSettings;
import org.opensearch.index.store.FsDirectoryFactory;
import org.opensearch.index.store.SmbDirectoryWrapper;

import java.io.IOException;
import java.nio.file.Path;

/**
* Factory to create a new Simple File System type directory accessible as a SMB share
*
* @deprecated use {@link org.opensearch.index.store.smbniofs.SmbNIOFsDirectoryFactory} instead
*/
@Deprecated
public final class SmbSimpleFsDirectoryFactory extends FsDirectoryFactory {

private static final DeprecationLogger DEPRECATION_LOGGER = DeprecationLogger.getLogger(SmbSimpleFsDirectoryFactory.class);

@Override
protected Directory newFSDirectory(Path location, LockFactory lockFactory, IndexSettings indexSettings) throws IOException {
DEPRECATION_LOGGER.deprecate(IndexModule.Type.SIMPLEFS.getSettingsKey(), IndexModule.Type.SIMPLEFS.getSettingsKey()
Copy link
Contributor

@adnapibar adnapibar Aug 18, 2021

Choose a reason for hiding this comment

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

Is this redundant considering the MetadataCreateIndexService.validateStoreTypeSettings will log a deprecation message ?

+ " is deprecated and will be removed in 2.0");
return new SmbDirectoryWrapper(new SimpleFSDirectory(location, lockFactory));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,10 @@

package org.opensearch.plugin.store.smb;

import org.opensearch.common.logging.DeprecationLogger;
import org.opensearch.index.IndexModule;
import org.opensearch.index.store.smbmmapfs.SmbMmapFsDirectoryFactory;
import org.opensearch.index.store.smbniofs.SmbNIOFsDirectoryFactory;
import org.opensearch.index.store.smbsimplefs.SmbSimpleFsDirectoryFactory;
import org.opensearch.plugins.IndexStorePlugin;
import org.opensearch.plugins.Plugin;
Expand All @@ -43,11 +46,16 @@

public class SMBStorePlugin extends Plugin implements IndexStorePlugin {

private static final DeprecationLogger DEPRECATION_LOGGER = DeprecationLogger.getLogger(SMBStorePlugin.class);

@Override
public Map<String, DirectoryFactory> getDirectoryFactories() {
final Map<String, DirectoryFactory> indexStoreFactories = new HashMap<>(2);
indexStoreFactories.put("smb_mmap_fs", new SmbMmapFsDirectoryFactory());
DEPRECATION_LOGGER.deprecate(IndexModule.Type.SIMPLEFS.getSettingsKey(), IndexModule.Type.SIMPLEFS.getSettingsKey()
Copy link
Contributor

Choose a reason for hiding this comment

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

Redundant logging (as commented above)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This message is posted once, when the plugin is loaded and the Factory instance is created. The other message is posted every time the factory.newFSDirectory is called (which could, though probably unlikely, happen in a user application) to instantiate a new instance of the SimpleFSDirectory. So I think it's worthy having both.

+ " is deprecated and will be removed in 2.0");
indexStoreFactories.put("smb_simple_fs", new SmbSimpleFsDirectoryFactory());
indexStoreFactories.put("smb_nio_fs", new SmbNIOFsDirectoryFactory());
return Collections.unmodifiableMap(indexStoreFactories);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*/

package org.opensearch.index.store;

import org.apache.lucene.store.Directory;
import org.apache.lucene.store.NIOFSDirectory;

import java.io.IOException;
import java.nio.file.Path;

/**
* SMB Tests using NIO FileSystem as index store type.
*/
public class SmbNIOFSDirectoryTests extends OpenSearchBaseDirectoryTestCase {

@Override
protected Directory getDirectory(Path file) throws IOException {
return new SmbDirectoryWrapper(new NIOFSDirectory(file));
}

@Override
public void testCreateOutputForExistingFile() throws IOException {
/**
* This test is disabled because {@link SmbDirectoryWrapper} opens existing file
* with an explicit StandardOpenOption.TRUNCATE_EXISTING option.
*/
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@

package org.opensearch.cluster.routing;

import org.apache.lucene.store.SimpleFSDirectory;
import org.apache.lucene.store.NIOFSDirectory;

import org.opensearch.action.admin.cluster.allocation.ClusterAllocationExplanation;
import org.opensearch.action.admin.indices.stats.ShardStats;
Expand Down Expand Up @@ -151,7 +151,7 @@ public void testFailedRecoveryOnAllocateStalePrimaryRequiresAnotherAllocateStale
});

internalCluster().stopRandomNode(InternalTestCluster.nameFilter(node1));
try(Store store = new Store(shardId, indexSettings, new SimpleFSDirectory(indexPath), new DummyShardLock(shardId))) {
try(Store store = new Store(shardId, indexSettings, new NIOFSDirectory(indexPath), new DummyShardLock(shardId))) {
store.removeCorruptionMarker();
}
node1 = internalCluster().startNode(node1DataPathSettings);
Expand Down Expand Up @@ -229,7 +229,7 @@ private String historyUUID(String node, String indexName) {
}

private void putFakeCorruptionMarker(IndexSettings indexSettings, ShardId shardId, Path indexPath) throws IOException {
try(Store store = new Store(shardId, indexSettings, new SimpleFSDirectory(indexPath), new DummyShardLock(shardId))) {
try(Store store = new Store(shardId, indexSettings, new NIOFSDirectory(indexPath), new DummyShardLock(shardId))) {
store.markStoreCorrupted(new IOException("fake ioexception"));
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@
import org.opensearch.common.xcontent.XContentHelper;
import org.opensearch.env.Environment;
import org.opensearch.index.Index;
import org.opensearch.index.IndexModule;
import org.opensearch.index.IndexNotFoundException;
import org.opensearch.index.IndexService;
import org.opensearch.index.IndexSettings;
Expand Down Expand Up @@ -783,9 +784,20 @@ static Settings aggregateIndexSettings(ClusterState currentState, CreateIndexClu
"Please do not specify value for setting [index.soft_deletes.enabled] of index [" + request.index() + "].");
}
validateTranslogRetentionSettings(indexSettings);
validateStoreTypeSettings(indexSettings);

return indexSettings;
}

public static void validateStoreTypeSettings(Settings settings) {
// deprecate simplefs store type:
if (IndexModule.Type.SIMPLEFS.match(IndexModule.INDEX_STORE_TYPE_SETTING.get(settings))) {
DEPRECATION_LOGGER.deprecate("store_type_setting",
"[simplefs] is deprecated and will be removed in 2.0. Use [niofs], which offers equal or better performance, " +
"or other file systems instead.");
}
}

static int getNumberOfShards(final Settings.Builder indexSettingsBuilder) {
// TODO: this logic can be removed when the current major version is 8
assert Version.CURRENT.major == 1;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -250,8 +250,12 @@ public ClusterState execute(ClusterState currentState) {

if (IndexSettings.INDEX_TRANSLOG_RETENTION_AGE_SETTING.exists(normalizedSettings) ||
IndexSettings.INDEX_TRANSLOG_RETENTION_SIZE_SETTING.exists(normalizedSettings)) {
Settings indexSettings;
for (String index : actualIndices) {
MetadataCreateIndexService.validateTranslogRetentionSettings(metadataBuilder.get(index).getSettings());
indexSettings = metadataBuilder.get(index).getSettings();
MetadataCreateIndexService.validateTranslogRetentionSettings(indexSettings);
// validate storeType for deprecating index stores
MetadataCreateIndexService.validateStoreTypeSettings(indexSettings);
}
}
boolean changed = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
import org.apache.lucene.store.IOContext;
import org.apache.lucene.store.IndexInput;
import org.apache.lucene.store.IndexOutput;
import org.apache.lucene.store.SimpleFSDirectory;
import org.apache.lucene.store.NIOFSDirectory;
import org.apache.lucene.util.SetOnce;
import org.opensearch.cli.ExitCodes;
import org.opensearch.cli.UserException;
Expand Down Expand Up @@ -236,7 +236,7 @@ public static KeyStoreWrapper load(Path configDir, String keystoreFileName) thro
return null;
}

SimpleFSDirectory directory = new SimpleFSDirectory(configDir);
NIOFSDirectory directory = new NIOFSDirectory(configDir);
try (IndexInput indexInput = directory.openInput(keystoreFileName, IOContext.READONCE)) {
ChecksumIndexInput input = new BufferedChecksumIndexInput(indexInput);
final int formatVersion;
Expand Down Expand Up @@ -502,7 +502,7 @@ private void decryptLegacyEntries() throws GeneralSecurityException, IOException
public synchronized void save(Path configDir, char[] password) throws Exception {
ensureOpen();

SimpleFSDirectory directory = new SimpleFSDirectory(configDir);
NIOFSDirectory directory = new NIOFSDirectory(configDir);
// write to tmp file first, then overwrite
String tmpFile = KEYSTORE_FILENAME + ".tmp";
try (IndexOutput output = directory.createOutput(tmpFile, IOContext.DEFAULT)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@
import org.apache.lucene.store.FSDirectory;
import org.apache.lucene.store.Lock;
import org.apache.lucene.store.LockObtainFailedException;
import org.apache.lucene.store.NIOFSDirectory;
import org.apache.lucene.store.NativeFSLockFactory;
import org.apache.lucene.store.SimpleFSDirectory;
import org.opensearch.OpenSearchException;
import org.opensearch.Version;
import org.opensearch.cluster.metadata.IndexMetadata;
Expand Down Expand Up @@ -501,7 +501,7 @@ public static void acquireFSLockForPaths(IndexSettings indexSettings, Path... sh
// resolve the directory the shard actually lives in
Path p = shardPaths[i].resolve("index");
// open a directory (will be immediately closed) on the shard's location
dirs[i] = new SimpleFSDirectory(p, indexSettings.getValue(FsDirectoryFactory.INDEX_LOCK_FACTOR_SETTING));
dirs[i] = new NIOFSDirectory(p, indexSettings.getValue(FsDirectoryFactory.INDEX_LOCK_FACTOR_SETTING));
// create a lock for the "write.lock" file
try {
locks[i] = dirs[i].obtainLock(IndexWriter.WRITE_LOCK_NAME);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
import org.apache.lucene.store.IOContext;
import org.apache.lucene.store.IndexInput;
import org.apache.lucene.store.IndexOutput;
import org.apache.lucene.store.SimpleFSDirectory;
import org.apache.lucene.store.NIOFSDirectory;
import org.opensearch.ExceptionsHelper;
import org.opensearch.common.collect.Tuple;
import org.opensearch.common.lucene.store.IndexOutputOutputStream;
Expand Down Expand Up @@ -323,7 +323,7 @@ public final T read(NamedXContentRegistry namedXContentRegistry, Path file) thro
}

protected Directory newDirectory(Path dir) throws IOException {
return new SimpleFSDirectory(dir);
return new NIOFSDirectory(dir);
}


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
import org.apache.lucene.search.Weight;
import org.apache.lucene.store.AlreadyClosedException;
import org.apache.lucene.store.Directory;
import org.apache.lucene.store.SimpleFSDirectory;
import org.apache.lucene.store.NIOFSDirectory;
import org.apache.lucene.util.Bits;
import org.apache.lucene.util.BytesRef;
import org.apache.lucene.util.BytesRefIterator;
Expand Down Expand Up @@ -228,16 +228,16 @@ private static IndexWriter createIndexWriter(Directory directory, boolean openEx
*/
public static void deleteAll(Path[] dataPaths) throws IOException {
for (Path dataPath : dataPaths) {
Lucene.cleanLuceneIndex(new SimpleFSDirectory(dataPath.resolve(METADATA_DIRECTORY_NAME)));
Lucene.cleanLuceneIndex(new NIOFSDirectory(dataPath.resolve(METADATA_DIRECTORY_NAME)));
}
}

// exposed for tests
Directory createDirectory(Path path) throws IOException {
// it is possible to disable the use of MMapDirectory for indices, and it may be surprising to users that have done so if we still
// use a MMapDirectory here, which might happen with FSDirectory.open(path). Concurrency is of no concern here so a
// SimpleFSDirectory is fine:
return new SimpleFSDirectory(path);
// NIOFSDirectory is fine:
return new NIOFSDirectory(path);
}

public Path[] getDataPaths() {
Expand Down Expand Up @@ -277,7 +277,7 @@ public static NodeMetadata nodeMetadata(Path... dataPaths) throws IOException {
for (final Path dataPath : dataPaths) {
final Path indexPath = dataPath.resolve(METADATA_DIRECTORY_NAME);
if (Files.exists(indexPath)) {
try (DirectoryReader reader = DirectoryReader.open(new SimpleFSDirectory(dataPath.resolve(METADATA_DIRECTORY_NAME)))) {
try (DirectoryReader reader = DirectoryReader.open(new NIOFSDirectory(dataPath.resolve(METADATA_DIRECTORY_NAME)))) {
final Map<String, String> userData = reader.getIndexCommit().getUserData();
assert userData.get(NODE_VERSION_KEY) != null;

Expand Down Expand Up @@ -308,12 +308,12 @@ public static void overrideVersion(Version newVersion, Path... dataPaths) throws
for (final Path dataPath : dataPaths) {
final Path indexPath = dataPath.resolve(METADATA_DIRECTORY_NAME);
if (Files.exists(indexPath)) {
try (DirectoryReader reader = DirectoryReader.open(new SimpleFSDirectory(dataPath.resolve(METADATA_DIRECTORY_NAME)))) {
try (DirectoryReader reader = DirectoryReader.open(new NIOFSDirectory(dataPath.resolve(METADATA_DIRECTORY_NAME)))) {
final Map<String, String> userData = reader.getIndexCommit().getUserData();
assert userData.get(NODE_VERSION_KEY) != null;

try (IndexWriter indexWriter =
createIndexWriter(new SimpleFSDirectory(dataPath.resolve(METADATA_DIRECTORY_NAME)), true)) {
createIndexWriter(new NIOFSDirectory(dataPath.resolve(METADATA_DIRECTORY_NAME)), true)) {
final Map<String, String> commitData = new HashMap<>(userData);
commitData.put(NODE_VERSION_KEY, Integer.toString(newVersion.id));
indexWriter.setLiveCommitData(commitData.entrySet());
Expand Down
Loading