Re: [PR] SOLR-XXXXX: Support different filesystem implementations with EmbeddedSolrServer [solr]
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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-XXXXX: Support different filesystem implementations with EmbeddedSolrServer [solr]
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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-XXXXX: Support different filesystem implementations with EmbeddedSolrServer [solr]
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-XXXXX: Support different filesystem implementations with EmbeddedSolrServer [solr]
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-XXXXX: Support different filesystem implementations with EmbeddedSolrServer [solr]
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-XXXXX: Support different filesystem implementations with EmbeddedSolrServer [solr]
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-X: 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-XXXXX: Support different filesystem implementations with EmbeddedSolrServer [solr]
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-X: 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-XXXXX: Support different filesystem implementations with EmbeddedSolrServer [solr]
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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-XXXXX: Support different filesystem implementations with EmbeddedSolrServer [solr]
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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-XXXXX: Support different filesystem implementations with EmbeddedSolrServer [solr]
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-XXXXX: Support different filesystem implementations with EmbeddedSolrServer [solr]
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-XXXXX: Support different filesystem implementations with EmbeddedSolrServer [solr]
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
