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

SOLR-16903: Switch CoreContainer#getSolrHome to return Path instead of String #3204

Merged
merged 8 commits into from
Feb 25, 2025
Merged

SOLR-16903: Switch CoreContainer#getSolrHome to return Path instead of String #3204

merged 8 commits into from
Feb 25, 2025

Conversation

AndreyBozhko
Copy link
Contributor

@AndreyBozhko AndreyBozhko commented Feb 21, 2025 •
edited
Loading

https://issues.apache.org/jira/browse/SOLR-16903

Description

This is only a part of SOLR-16903, prompted by the comment here: #3146 (comment)

Solution

Updated public getter to return Path.

Suggested review order commit-by-commit.

Tests

Existing tests.

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute 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.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Reference Guide

epugh
epugh reviewed Feb 21, 2025
View reviewed changes
solr/core/src/java/org/apache/solr/core/ZkContainer.java Outdated
@@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, updated.

epugh
epugh approved these changes Feb 21, 2025
View reviewed changes
Copy link
Contributor

@epugh epugh 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 comment or two, but if the tests path, this looks nice.

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();
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we stay in path land if we change FileUtils?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

mlbiscoc
mlbiscoc reviewed Feb 21, 2025
View reviewed changes
solr/core/src/java/org/apache/solr/core/ZkContainer.java Outdated
@@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

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

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 Outdated
@@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

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

toString() might not be necessary here because of this textWriter I think but this probably needs to be confirmed.

Copy link
Contributor

Choose a reason for hiding this comment

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

But also that TextWriter is doing a .toAbsolutePath() which doesn't seem right to me. If SolrHome is always absolute this wouldn't matter, if it's relative, then possible the response would change. Maybe ignoring what I said above is the safest option.

Copy link
Contributor

Choose a reason for hiding this comment

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

;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Good eye! I agree with removing toString here. Whether toAbsolutePath should be called or not is another question from this PR; I'm ambivalent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense - the JavaBinCodec looks to be doing the same thing.

solr/solr/solrj/src/java/org/apache/solr/common/util/JavaBinCodec.java

Lines 411 to 413 in 76c09a3

if (val instanceof Path) {
writeStr(((Path) val).toAbsolutePath().toString());
return true;

@dsmiley
Copy link
Contributor

dsmiley commented Feb 23, 2025

Excellent! I love how focused this PR is.

Can you please edit the JIRA association to be 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.

@AndreyBozhko AndreyBozhko changed the title SOLR-8282: Switch CoreContainer#getSolrHome to return Path instead of String SOLR-16903: Switch CoreContainer#getSolrHome to return Path instead of String Feb 23, 2025
@AndreyBozhko
Copy link
Contributor Author

Sounds good, I updated the PR.

@dsmiley
Copy link
Contributor

dsmiley commented Feb 24, 2025

I plan to merge this tonight; it's very straightforward

@dsmiley dsmiley merged commit 53fe9cd into apache:main Feb 25, 2025
3 of 4 checks passed
@AndreyBozhko AndreyBozhko deleted the solr-home-path branch February 25, 2025 15:10
@AndreyBozhko AndreyBozhko mentioned this pull request Feb 25, 2025
Draft
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epugh epugh epugh approved these changes

@dsmiley dsmiley dsmiley left review comments

@mlbiscoc mlbiscoc mlbiscoc approved these changes

Assignees
No one assigned
Labels
cat:cloud cat:metrics cat:packagemanager client:solrj test-framework tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AndreyBozhko @dsmiley @epugh @mlbiscoc