-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Changes from all commits
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 |
---|---|---|
@@ -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; | ||
|
||
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 |
---|---|---|
|
@@ -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() | ||
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. Is this redundant considering the |
||
+ " 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 |
---|---|---|
|
@@ -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; | ||
|
@@ -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() | ||
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. Redundant logging (as commented above)? 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. This message is posted once, when the plugin is loaded and the Factory instance is created. The other message is posted every time the |
||
+ " 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); | ||
} | ||
|
||
|
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. | ||
*/ | ||
} | ||
} |
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.
Can the implementations
SmbMmapFsDirectoryFactory
,SmbNIOFsDirectoryFactory
andSmbSimpleFsDirectoryFactory
be in the same package something likesmbfs
or simplysmb
? Similarly for all the test classes.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.
Yes, I agree. I think maybe we do that refactor in a followup PR? How about in the removal PR?