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.

Sign up for GitHub

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

Jump to bottom

[DEPRECATE] SimpleFS in favor of NIOFS #1073

Merged
merged 5 commits into from
Aug 19, 2021
Merged

[DEPRECATE] SimpleFS in favor of NIOFS #1073

merged 5 commits into from
Aug 19, 2021

Conversation

nknize
Copy link
Collaborator

@nknize nknize commented Aug 11, 2021 •
edited
Loading

Lucene 9 removes support for SimpleFS File System format. This PR deprecates the SimpleFS format in favor of NIOFS.

resolves #1072

@nknize nknize added v1.1.0 Issues, PRs, related to the 1.1.0 release deprecate labels Aug 11, 2021
@nknize
Copy link
Collaborator Author

nknize commented Aug 11, 2021

start gradle check

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success d9dfc048a1341b2c24ff00793b13aa43e3edf01c

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed d9dfc048a1341b2c24ff00793b13aa43e3edf01c

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success d9dfc048a1341b2c24ff00793b13aa43e3edf01c

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success d9dfc048a1341b2c24ff00793b13aa43e3edf01c
Log 383

Reports 383

@nknize
Copy link
Collaborator Author

nknize commented Aug 16, 2021

start gradle check

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success 15e3e4cbd3956ddb0c4c16b2fa244cc46ccdfeb3

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 15e3e4cbd3956ddb0c4c16b2fa244cc46ccdfeb3

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success 15e3e4cbd3956ddb0c4c16b2fa244cc46ccdfeb3

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 15e3e4cbd3956ddb0c4c16b2fa244cc46ccdfeb3
Log 405

Reports 405

@nknize
Copy link
Collaborator Author

nknize commented Aug 16, 2021

start gradle check

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 478ae30c0160d3230712e88f28d24e129e42db0b

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success 478ae30c0160d3230712e88f28d24e129e42db0b

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success 478ae30c0160d3230712e88f28d24e129e42db0b

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 478ae30c0160d3230712e88f28d24e129e42db0b
Log 406

Reports 406

@nknize
Copy link
Collaborator Author

nknize commented Aug 16, 2021

start gradle check

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 478ae30c0160d3230712e88f28d24e129e42db0b
Log 407

Reports 407

@nknize
Copy link
Collaborator Author

nknize commented Aug 16, 2021

start gradle check

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 478ae30c0160d3230712e88f28d24e129e42db0b
Log 408

Reports 408

@nknize nknize mentioned this pull request Aug 17, 2021
Closed
2 tasks
adnapibar
adnapibar approved these changes Aug 18, 2021
View reviewed changes
Copy link
Contributor

@adnapibar adnapibar left a comment

Choose a reason for hiding this comment

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

LGTM 👍 A few minor comments.

What about the usage in the OpenSearch project, are we going to replace those? For instance,

OpenSearch/server/src/main/java/org/opensearch/common/settings/KeyStoreWrapper.java

Line 43 in 4906a3c

import org.apache.lucene.store.SimpleFSDirectory;

...ns/store-smb/src/main/java/org/opensearch/index/store/smbniofs/SmbNIOFsDirectoryFactory.java
* compatible open source license.
*/

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

@adnapibar adnapibar Aug 18, 2021 •
edited
Loading

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?

...re-smb/src/main/java/org/opensearch/index/store/smbsimplefs/SmbSimpleFsDirectoryFactory.java
@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 •
edited
Loading

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 ?

plugins/store-smb/src/main/java/org/opensearch/plugin/store/smb/SMBStorePlugin.java
@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.

server/src/main/java/org/opensearch/cluster/metadata/MetadataUpdateSettingsService.java Outdated Show resolved Hide resolved
@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success 6dd13ee8ad07ccde0ade96c35d6eb95c58f8e7e2

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 6dd13ee8ad07ccde0ade96c35d6eb95c58f8e7e2

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success 6dd13ee8ad07ccde0ade96c35d6eb95c58f8e7e2

@nknize
[DEPRECATE] SimpleFS in favor of NIOFS
6882eae 
Lucene 9 removes support for SimpleFS File System format. This commit deprecates
the SimpleFS format in favor of NIOFS.

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
@nknize
Add javadocs
a5d0270 
Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
@nknize
Clean up warnings and docs
efaf804 
Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
@nknize
Feedback from PR
ea5190e 
Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
@nknize
Switch the rest of SimpleFS usage to NIOFS
846f78a 
Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
@opensearch-ci-bot
Copy link
Collaborator

❌   DCO Check Failed 6bf03cfa224b07c4d06889cca37f7faa70b5dcae
Run ./dev-tools/signoff-check.sh remotes/origin/main 6bf03cfa224b07c4d06889cca37f7faa70b5dcae to check locally
Use git commit with -s to add 'Signed-of-by: {EMAIL}' on impacted commits

@nknize
Copy link
Collaborator Author

nknize commented Aug 19, 2021

What about the usage in the OpenSearch project, are we going to replace those? For instance,

OpenSearch/server/src/main/java/org/opensearch/common/settings/KeyStoreWrapper.java

Line 43 in 4906a3c

import org.apache.lucene.store.SimpleFSDirectory;

I was going to leave them for full replacement in the Lucene 9 upgrade, but I think it's a better idea to take care of it now. So that's done

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success 846f78a

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 846f78a

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success 846f78a

@nknize
Copy link
Collaborator Author

nknize commented Aug 19, 2021

start gradle check

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 846f78a
Log 411

Reports 411

@nknize nknize merged commit ff7e790 into opensearch-project:main Aug 19, 2021
nknize added a commit to nknize/OpenSearch that referenced this pull request Aug 20, 2021
@nknize
[DEPRECATE] SimpleFS in favor of NIOFS (opensearch-project#1073)
0f06e0e 
Lucene 9 removes support for SimpleFS File System format. This commit deprecates
the SimpleFS format in favor of NIOFS.

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
@nknize nknize added the pending backport Identifies an issue or PR that still needs to be backported label Aug 20, 2021
nknize added a commit that referenced this pull request Aug 20, 2021
@nknize
[DEPRECATE] SimpleFS in favor of NIOFS (#1073) (#1114)
c15e687 
Lucene 9 removes support for SimpleFS File System format. This commit deprecates
the SimpleFS format in favor of NIOFS.

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
@nknize nknize removed the pending backport Identifies an issue or PR that still needs to be backported label Aug 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adnapibar adnapibar adnapibar approved these changes

@setiah setiah Awaiting requested review from setiah

Assignees
No one assigned
Labels
deprecate v1.1.0 Issues, PRs, related to the 1.1.0 release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Deprecate SimpleFS File System Format
3 participants
@nknize @opensearch-ci-bot @adnapibar