SOLR-XXXXX: Support different filesystem implementations with EmbeddedSolrServer #3146
Conversation
| """ | ||
| Test only works without Security Manager due to SecurityConfHandlerLocal | ||
| missing permission to read /1/2/3/4/security.json file""", | ||
| System.getSecurityManager() == null); |
There was a problem hiding this comment.
To make the test work regardless of the security manager, this line
However, that requires updating CoreContainer#getSolrHome to return a Path instead of a String - so it looks like a much bigger change.
There was a problem hiding this comment.
CoreContainer.solrHome is a Path so hopefully it won't be too huge to switch it's getter to use Path.
There was a problem hiding this comment.
OK, I think I will keep this PR as draft for now, and work on updating CoreContainer#getSolrHome to return Path first. Is there an existing JIRA for that?
| @@ -148,12 +147,11 @@ public String lookupZKManagedSchemaPath() { | |||
| */ | |||
| public Path lookupLocalManagedSchemaPath() { | |||
| final Path legacyManagedSchemaPath = | |||
| Paths.get( | |||
There was a problem hiding this comment.
We should try to remove usages of Paths.get generally, I remember it having some wonky behaviour in the past. Happy to see this coming in.
There was a problem hiding this comment.
Absolutely; same with Path.of. Ultimately we should ForbiddenApi check this so that it's rare to call this except for a few authorized places.
There was a problem hiding this comment.
This is really about SOLR-17548 / SOLR-16903 / SOLR-8282 / SOLR-17548. I don't think we need separate JIRAs for separate File->Path or String->Path matters, but separate PRs is great. Not only do we need to convert File -> Path, but we need to change most String based paths to Path. Perhaps PR, by PR we focus on one particular String. I don't think this PR is really about EmbeddedSolrServer (you put that in the title).
| @@ -148,12 +147,11 @@ public String lookupZKManagedSchemaPath() { | |||
| */ | |||
| public Path lookupLocalManagedSchemaPath() { | |||
| final Path legacyManagedSchemaPath = | |||
| Paths.get( | |||
There was a problem hiding this comment.
Absolutely; same with Path.of. Ultimately we should ForbiddenApi check this so that it's rare to call this except for a few authorized places.
| """ | ||
| Test only works without Security Manager due to SecurityConfHandlerLocal | ||
| missing permission to read /1/2/3/4/security.json file""", | ||
| System.getSecurityManager() == null); |
There was a problem hiding this comment.
CoreContainer.solrHome is a Path so hopefully it won't be too huge to switch it's getter to use Path.
|
Thanks - I just opened SOLR-17640 before reading your comment. Do you suggest this should go under SOLR-16903 instead? Or would you prefer to see a more comprehensive refactoring for Paths#get usage in the codebase? |
| @@ -34,6 +34,8 @@ Improvements | |||
|
|
|||
| * SOLR-17516: `LBHttp2SolrClient` is now generic, adding support for `HttpJdkSolrClient`. (James Dyer) | |||
|
|
|||
| * SOLR-XXXXX: Support different filesystem implementations with EmbeddedSolrServer (Andrey Bozhko) | |||
There was a problem hiding this comment.
DOn't we have a GH or GITHUB type name for a PR with no JIRA?
There was a problem hiding this comment.
I believe he put this here as a TODO. We have quite a number of existing JIRAs to pick from for this theme :-).
https://issues.apache.org/jira/browse/SOLR-XXXXX
Description
EmbeddedSolrServer has a public constructor
EmbeddedSolrServer(Path solrHome, String defaultCoreName)- which suggests that any Path implementation is acceptable (e.g., ZipPath, S3Path, and not just UnixPath/WindowsPath).Solution
Updated ManagedIndexSchemaFactory code that deals with schema paths to be more idiomatic, and correctly handle Path instances associated with non-default file systems.
Tests
Added a test for instantiating EmbeddedSolrServer using a ZipPath instance.
Checklist
Please review the following and check all that apply:
mainbranch../gradlew check.