Re: [PR] SOLR-8282: Switch CoreContainer#getSolrHome to return Path instead of String [solr]

2025-02-22 Thread via GitHub 


dsmiley commented on PR #3204:
URL: https://github.com/apache/solr/pull/3204#issuecomment-2676562570

   Excellent!  I love how focused this PR is.
   
   Can you please edit the JIRA association to be 
[SOLR-16903](https://issues.apache.org/jira/browse/SOLR-16903) -- PR title & 
CHANGES.txt.  There's already a CHANGES.txt entries for 16903.   Needn't 
specify every single API endpoint / category since by 10.0, I think we hope we 
can basically say, "Changed all Java APIs using File to NIO Path" and not get 
into further details.


-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] SOLR-8282: Switch CoreContainer#getSolrHome to return Path instead of String [solr]

2025-02-21 Thread via GitHub 


epugh commented on code in PR #3204:
URL: https://github.com/apache/solr/pull/3204#discussion_r1966069823


##
solr/core/src/java/org/apache/solr/handler/admin/SystemInfoHandler.java:
##
@@ -145,7 +145,7 @@ public void handleRequestBody(SolrQueryRequest req, 
SolrQueryResponse rsp) throw
   rsp.add("zkHost", 
getCoreContainer(req).getZkController().getZkServerAddress());
 }
 if (cc != null) {
-  rsp.add("solr_home", cc.getSolrHome());
+  rsp.add("solr_home", cc.getSolrHome().toString());

Review Comment:
   ;-)



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] SOLR-8282: Switch CoreContainer#getSolrHome to return Path instead of String [solr]

2025-02-21 Thread via GitHub 


AndreyBozhko commented on code in PR #3204:
URL: https://github.com/apache/solr/pull/3204#discussion_r1966067522


##
solr/solrj/src/test/org/apache/solr/client/solrj/request/TestCoreAdmin.java:
##
@@ -80,7 +80,7 @@ public void testCustomUlogDir() throws Exception {
   cores.getAllowPaths().add(dataDir.toPath()); // Allow the test dir
   cores.getAllowPaths().add(newCoreInstanceDir.toPath()); // Allow the 
test dir
 
-  File instanceDir = new File(cores.getSolrHome());
+  File instanceDir = cores.getSolrHome().toFile();

Review Comment:
   Yes, it's possible to replace FileUtils here with 
`org.apache.commons.io.file.PathUtils`. For now I changed just the callsites 
for CoreContainer#getSolrHome, to simplify the review.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] SOLR-8282: Switch CoreContainer#getSolrHome to return Path instead of String [solr]

2025-02-21 Thread via GitHub 


mlbiscoc commented on code in PR #3204:
URL: https://github.com/apache/solr/pull/3204#discussion_r1966047995


##
solr/core/src/java/org/apache/solr/core/ZkContainer.java:
##
@@ -87,7 +87,7 @@ public void initZooKeeper(final CoreContainer cc, CloudConfig 
config) {
 // TODO: remove after updating to an slf4j based zookeeper
 System.setProperty("zookeeper.jmx.log4j.disable", "true");
 
-String solrHome = cc.getSolrHome();
+String solrHome = cc.getSolrHome().toString();

Review Comment:
   If I am reading this right, this can be removed and the call below doesn't 
need `Paths.get()` but can just be 
`cc.getSolrHome().resolve(zoo_data).toString()`



##
solr/core/src/java/org/apache/solr/handler/admin/SystemInfoHandler.java:
##
@@ -145,7 +145,7 @@ public void handleRequestBody(SolrQueryRequest req, 
SolrQueryResponse rsp) throw
   rsp.add("zkHost", 
getCoreContainer(req).getZkController().getZkServerAddress());
 }
 if (cc != null) {
-  rsp.add("solr_home", cc.getSolrHome());
+  rsp.add("solr_home", cc.getSolrHome().toString());

Review Comment:
   `toString()` might not be necessary here because of this 
[textWriter](https://github.com/apache/solr/blob/d2fdb16a78195e5eb40caf48761afeb1bce041ee/solr/solrj/src/java/org/apache/solr/common/util/TextWriter.java#L78)
 I think but this probably needs to be confirmed.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] SOLR-8282: Switch CoreContainer#getSolrHome to return Path instead of String [solr]

2025-02-21 Thread via GitHub 


AndreyBozhko commented on code in PR #3204:
URL: https://github.com/apache/solr/pull/3204#discussion_r1966064254


##
solr/core/src/java/org/apache/solr/core/ZkContainer.java:
##
@@ -87,7 +87,7 @@ public void initZooKeeper(final CoreContainer cc, CloudConfig 
config) {
 // TODO: remove after updating to an slf4j based zookeeper
 System.setProperty("zookeeper.jmx.log4j.disable", "true");
 
-String solrHome = cc.getSolrHome();
+String solrHome = cc.getSolrHome().toString();

Review Comment:
   Thanks, updated.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] SOLR-8282: Switch CoreContainer#getSolrHome to return Path instead of String [solr]

2025-02-21 Thread via GitHub 


epugh commented on code in PR #3204:
URL: https://github.com/apache/solr/pull/3204#discussion_r1965995115


##
solr/solrj/src/test/org/apache/solr/client/solrj/request/TestCoreAdmin.java:
##
@@ -80,7 +80,7 @@ public void testCustomUlogDir() throws Exception {
   cores.getAllowPaths().add(dataDir.toPath()); // Allow the test dir
   cores.getAllowPaths().add(newCoreInstanceDir.toPath()); // Allow the 
test dir
 
-  File instanceDir = new File(cores.getSolrHome());
+  File instanceDir = cores.getSolrHome().toFile();

Review Comment:
   Could we stay in path land if we change FileUtils?



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] SOLR-8282: Switch CoreContainer#getSolrHome to return Path instead of String [solr]

2025-02-21 Thread via GitHub 


epugh commented on code in PR #3204:
URL: https://github.com/apache/solr/pull/3204#discussion_r1965992291


##
solr/core/src/java/org/apache/solr/core/ZkContainer.java:
##
@@ -87,7 +87,7 @@ public void initZooKeeper(final CoreContainer cc, CloudConfig 
config) {
 // TODO: remove after updating to an slf4j based zookeeper
 System.setProperty("zookeeper.jmx.log4j.disable", "true");
 
-String solrHome = cc.getSolrHome();
+String solrHome = cc.getSolrHome().toString();

Review Comment:
   Just want to confirm that we are still stuck with String here?   I see below 
the `Paths.get`



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]