You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "AndreyBozhko (via GitHub)" <gi...@apache.org> on 2025/01/29 23:18:30 UTC
[PR] SOLR-XXXXX: Support different filesystem implementations with EmbeddedSolrServer [solr]
AndreyBozhko opened a new pull request, #3146:
URL: https://github.com/apache/solr/pull/3146
https://issues.apache.org/jira/browse/SOLR-XXXXX
<!--
_(If you are a project committer then you may remove some/all of the following template.)_
Before creating a pull request, please file an issue in the ASF Jira system for Solr:
* https://issues.apache.org/jira/projects/SOLR
For something minor (i.e. that wouldn't be worth putting in release notes), you can skip JIRA.
To create a Jira issue, you will need to create an account there first.
The title of the PR should reference the Jira issue number in the form:
* SOLR-####: <short description of problem or changes>
SOLR must be fully capitalized. A short description helps people scanning pull requests for items they can work on.
Properly referencing the issue in the title ensures that Jira is correctly updated with code review comments and commits. -->
# 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.
# Tests
Added a test for instantiating EmbeddedSolrServer using a ZipPath instance.
# Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) and my code conforms to the standards described there to the best of my ability.
- [ ] I have created a Jira issue and added the issue ID to my pull request title.
- [x] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
- [x] I have developed this patch against the `main` branch.
- [x] I have run `./gradlew check`.
- [x] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Add test case for using non-default filesystem with EmbeddedSolrServer [solr]
Posted by "AndreyBozhko (via GitHub)" <gi...@apache.org>.
AndreyBozhko commented on PR #3146:
URL: https://github.com/apache/solr/pull/3146#issuecomment-2797463057
GHA failure due to unrelated test BasicDistributedZk2Test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-XXXXX: Support different filesystem implementations with EmbeddedSolrServer [solr]
Posted by "AndreyBozhko (via GitHub)" <gi...@apache.org>.
AndreyBozhko commented on code in PR #3146:
URL: https://github.com/apache/solr/pull/3146#discussion_r1934788370
##########
solr/core/src/test/org/apache/solr/client/solrj/embedded/TestEmbeddedSolrServerConstructors.java:
##########
@@ -58,4 +63,63 @@ public void testNodeConfigConstructor() throws Exception {
assertEquals(1, server.query("newcore", new SolrQuery("*:*")).getResults().getNumFound());
}
}
+
+ @SuppressWarnings("removal")
+ @Test
+ public void testPathConstructorZipFS() throws Exception {
+ assumeTrue(
+ """
+ Test only works without Security Manager due to SecurityConfHandlerLocal
+ missing permission to read /1/2/3/4/security.json file""",
+ System.getSecurityManager() == null);
Review Comment:
To make the test work regardless of the security manager, this line https://github.com/apache/solr/blob/a5b0b98a79559021e44d00b43fff6c020c54f829/solr/core/src/java/org/apache/solr/handler/admin/SecurityConfHandlerLocal.java#L43 needs to be updated to handle Path in a more idiomatic way.
However, that requires updating CoreContainer#getSolrHome to return a Path instead of a String - so it looks like a much bigger change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Add test case for using non-default filesystem with EmbeddedSolrServer [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley merged PR #3146:
URL: https://github.com/apache/solr/pull/3146
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Add test case for using non-default filesystem with EmbeddedSolrServer [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #3146:
URL: https://github.com/apache/solr/pull/3146#issuecomment-2971184178
Judt did; sure, why not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Add test case for using non-default filesystem with EmbeddedSolrServer [solr]
Posted by "AndreyBozhko (via GitHub)" <gi...@apache.org>.
AndreyBozhko commented on PR #3146:
URL: https://github.com/apache/solr/pull/3146#issuecomment-2766336610
I updated the PR to only add the unit test, as a demonstration of what's possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-XXXXX: Support different filesystem implementations with EmbeddedSolrServer [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #3146:
URL: https://github.com/apache/solr/pull/3146#issuecomment-2683567891
> Or perhaps do that only after accumulating more Java NIO cleanups.
-- this, I think. _Then_ maybe commit the unit test as it's kind of neat; shows how to do something, and that's it's possible. I could imagine a use-case to automate embedded Solr index building maybe. Nonetheless; the primary / real support for Solr NIO is the DirectoryFactory -- https://issues.apache.org/jira/browse/SOLR-17646
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-XXXXX: Support different filesystem implementations with EmbeddedSolrServer [solr]
Posted by "madrob (via GitHub)" <gi...@apache.org>.
madrob commented on code in PR #3146:
URL: https://github.com/apache/solr/pull/3146#discussion_r1934824591
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java:
##########
@@ -148,12 +147,11 @@ public String lookupZKManagedSchemaPath() {
*/
public Path lookupLocalManagedSchemaPath() {
final Path legacyManagedSchemaPath =
- Paths.get(
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-XXXXX: Support different filesystem implementations with EmbeddedSolrServer [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #3146:
URL: https://github.com/apache/solr/pull/3146#discussion_r1935720303
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java:
##########
@@ -148,12 +147,11 @@ public String lookupZKManagedSchemaPath() {
*/
public Path lookupLocalManagedSchemaPath() {
final Path legacyManagedSchemaPath =
- Paths.get(
Review 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.
##########
solr/core/src/test/org/apache/solr/client/solrj/embedded/TestEmbeddedSolrServerConstructors.java:
##########
@@ -58,4 +63,63 @@ public void testNodeConfigConstructor() throws Exception {
assertEquals(1, server.query("newcore", new SolrQuery("*:*")).getResults().getNumFound());
}
}
+
+ @SuppressWarnings("removal")
+ @Test
+ public void testPathConstructorZipFS() throws Exception {
+ assumeTrue(
+ """
+ Test only works without Security Manager due to SecurityConfHandlerLocal
+ missing permission to read /1/2/3/4/security.json file""",
+ System.getSecurityManager() == null);
Review Comment:
CoreContainer.solrHome is a Path so hopefully it won't be too huge to switch it's getter to use Path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-XXXXX: Support different filesystem implementations with EmbeddedSolrServer [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #3146:
URL: https://github.com/apache/solr/pull/3146#discussion_r1936130337
##########
solr/CHANGES.txt:
##########
@@ -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)
Review Comment:
I believe he put this here as a TODO. We have quite a number of existing JIRAs to pick from for this theme :-).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-XXXXX: Support different filesystem implementations with EmbeddedSolrServer [solr]
Posted by "AndreyBozhko (via GitHub)" <gi...@apache.org>.
AndreyBozhko commented on code in PR #3146:
URL: https://github.com/apache/solr/pull/3146#discussion_r1937640688
##########
solr/core/src/test/org/apache/solr/client/solrj/embedded/TestEmbeddedSolrServerConstructors.java:
##########
@@ -58,4 +63,63 @@ public void testNodeConfigConstructor() throws Exception {
assertEquals(1, server.query("newcore", new SolrQuery("*:*")).getResults().getNumFound());
}
}
+
+ @SuppressWarnings("removal")
+ @Test
+ public void testPathConstructorZipFS() throws Exception {
+ assumeTrue(
+ """
+ Test only works without Security Manager due to SecurityConfHandlerLocal
+ missing permission to read /1/2/3/4/security.json file""",
+ System.getSecurityManager() == null);
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Add test case for using non-default filesystem with EmbeddedSolrServer [solr]
Posted by "AndreyBozhko (via GitHub)" <gi...@apache.org>.
AndreyBozhko commented on PR #3146:
URL: https://github.com/apache/solr/pull/3146#issuecomment-2971060042
Hi @dsmiley I believe this unit test now works as expected - do you think it's OK to merge it in?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-XXXXX: Support different filesystem implementations with EmbeddedSolrServer [solr]
Posted by "AndreyBozhko (via GitHub)" <gi...@apache.org>.
AndreyBozhko commented on PR #3146:
URL: https://github.com/apache/solr/pull/3146#issuecomment-2624829745
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-XXXXX: Support different filesystem implementations with EmbeddedSolrServer [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #3146:
URL: https://github.com/apache/solr/pull/3146#discussion_r1935674798
##########
solr/CHANGES.txt:
##########
@@ -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)
Review Comment:
DOn't we have a `GH` or `GITHUB` type name for a PR with no JIRA?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-XXXXX: Support different filesystem implementations with EmbeddedSolrServer [solr]
Posted by "AndreyBozhko (via GitHub)" <gi...@apache.org>.
AndreyBozhko commented on code in PR #3146:
URL: https://github.com/apache/solr/pull/3146#discussion_r1970159304
##########
solr/core/src/test/org/apache/solr/client/solrj/embedded/TestEmbeddedSolrServerConstructors.java:
##########
@@ -58,4 +63,63 @@ public void testNodeConfigConstructor() throws Exception {
assertEquals(1, server.query("newcore", new SolrQuery("*:*")).getResults().getNumFound());
}
}
+
+ @SuppressWarnings("removal")
+ @Test
+ public void testPathConstructorZipFS() throws Exception {
+ assumeTrue(
+ """
+ Test only works without Security Manager due to SecurityConfHandlerLocal
+ missing permission to read /1/2/3/4/security.json file""",
+ System.getSecurityManager() == null);
Review Comment:
Removed the assumption about the SM now that #3204 is merged - the tests should be able to pass now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-XXXXX: Support different filesystem implementations with EmbeddedSolrServer [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #3146:
URL: https://github.com/apache/solr/pull/3146#discussion_r1970369309
##########
solr/core/src/java/org/apache/solr/schema/ManagedIndexSchemaFactory.java:
##########
@@ -148,12 +147,11 @@ public String lookupZKManagedSchemaPath() {
*/
public Path lookupLocalManagedSchemaPath() {
final Path legacyManagedSchemaPath =
- Paths.get(
Review Comment:
Separate PR (no JIRA), but IMO java.nio.file.Paths should probably be added to forbiddenapis config so we don't use it whatsoever. Only has 2 methods introduced in Java 7 that simply call to Java 11 Path.of. IntelliJ makes trivial work of inlining those methods. So Paths is obsolete but doesn't _really_ telly you that in it's javadocs. Heck; same with the old Hashtable :-)
##########
solr/core/src/test/org/apache/solr/client/solrj/embedded/TestEmbeddedSolrServerConstructors.java:
##########
@@ -58,4 +63,58 @@ public void testNodeConfigConstructor() throws Exception {
assertEquals(1, server.query("newcore", new SolrQuery("*:*")).getResults().getNumFound());
}
}
+
+ @Test
+ public void testPathConstructorZipFS() throws Exception {
+ Path dataDir = createTempDir("data-dir");
+ Path archive = createTempFile("configset", ".zip");
+ Files.delete(archive);
+
+ // When :
+ // Prepare a zip archive which contains
+ // the configset files as shown below:
+ //
+ // configset.zip
+ // └── 1
+ // └── 2
+ // └── 3
+ // └── 4
+ // ├── data
+ // │ └── core1
+ // │ ├── conf
+ // │ │ ├── managed-schema.xml
Review Comment:
BTW if you actually did want to launch Solr from a ZIP; the choice of a "managed" schema is dubious as it implies the mutability of the schema, yet the zip is read-only. In my experience, I've only seen people actually use a managed schema as-intended for POCs. I suppose you worked around this with the memory based StorageIO thing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-XXXXX: Support different filesystem implementations with EmbeddedSolrServer [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #3146:
URL: https://github.com/apache/solr/pull/3146#issuecomment-2624996827
Probably need to wait for @mlbiscoc in https://github.com/apache/solr/pull/2924 which is touching lots of stuff. Then pick a particular String based API endpoint and convert it in an isolated PR relating to that one method or class (if there are multiple). I think it's easier to review like this rather than touching everything.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-XXXXX: Support different filesystem implementations with EmbeddedSolrServer [solr]
Posted by "AndreyBozhko (via GitHub)" <gi...@apache.org>.
AndreyBozhko commented on PR #3146:
URL: https://github.com/apache/solr/pull/3146#issuecomment-2683540007
Indeed, this PR came out of the POC I did some time ago.
So if people think there's some value in the unit test I added, then we can move forward with this PR.
Otherwise, I can close it, and open a separate PR just for the changes to the ManagedIndexSchemaFactory. Or perhaps do that only after accumulating more Java NIO cleanups.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org