SOLR-17501: Move out CLI utils from SolrCLI #2744

malliaridis · 2024-10-06T15:59:57Z

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

Description

To reduce the scope and dependencies to SolrCLI.java, utility / helper methods should be extracted from SolrCLI into a separate class. This way, SolrCLI acts only as an entrypoint and CLI initiation class.

Solution

Several static methods and attributes have been moved to CLIUtils.java.

Changes from this PR are not planned for 9x (this would require additional intervention due to #2725).

Tests

Added a few tests to existing code to reduce technical debt.

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 · 2024-10-17T20:11:43Z

Is this able to backwards ported to 9x? Seems good stuff!

malliaridis · 2024-10-18T12:48:42Z

Is this able to backwards ported to 9x? Seems good stuff!

I think I could create a separate PR for backporting if we want these changes in 9x as well. I think a cherry-pick won't do in this case as I was expecting.

malliaridis · 2024-10-18T12:49:14Z

This PR is related and should be blocked by #2778 for now.

# Conflicts: # solr/core/src/java/org/apache/solr/cli/AssertTool.java # solr/core/src/java/org/apache/solr/cli/ConfigSetDownloadTool.java # solr/core/src/java/org/apache/solr/cli/ConfigSetUploadTool.java # solr/core/src/java/org/apache/solr/cli/CreateTool.java # solr/core/src/java/org/apache/solr/cli/PostTool.java # solr/core/src/java/org/apache/solr/cli/SolrCLI.java # solr/core/src/java/org/apache/solr/cli/StatusTool.java # solr/core/src/java/org/apache/solr/cli/ZkRmTool.java

malliaridis · 2024-11-06T15:21:12Z

I'll also wait for #2725 to be merged before completing thins one.

# Conflicts: # solr/core/src/java/org/apache/solr/cli/AssertTool.java # solr/core/src/java/org/apache/solr/cli/ClusterTool.java # solr/core/src/java/org/apache/solr/cli/ConfigSetDownloadTool.java # solr/core/src/java/org/apache/solr/cli/ConfigSetUploadTool.java # solr/core/src/java/org/apache/solr/cli/ConfigTool.java # solr/core/src/java/org/apache/solr/cli/CreateTool.java # solr/core/src/java/org/apache/solr/cli/DeleteTool.java # solr/core/src/java/org/apache/solr/cli/ExportTool.java # solr/core/src/java/org/apache/solr/cli/HealthcheckTool.java # solr/core/src/java/org/apache/solr/cli/LinkConfigTool.java # solr/core/src/java/org/apache/solr/cli/PostLogsTool.java # solr/core/src/java/org/apache/solr/cli/PostTool.java # solr/core/src/java/org/apache/solr/cli/RunExampleTool.java # solr/core/src/java/org/apache/solr/cli/SnapshotCreateTool.java # solr/core/src/java/org/apache/solr/cli/SnapshotDeleteTool.java # solr/core/src/java/org/apache/solr/cli/SnapshotDescribeTool.java # solr/core/src/java/org/apache/solr/cli/SnapshotListTool.java # solr/core/src/java/org/apache/solr/cli/SolrCLI.java # solr/core/src/java/org/apache/solr/cli/ZkCpTool.java # solr/core/src/java/org/apache/solr/cli/ZkLsTool.java # solr/core/src/java/org/apache/solr/cli/ZkMkrootTool.java # solr/core/src/java/org/apache/solr/cli/ZkMvTool.java # solr/core/src/java/org/apache/solr/cli/ZkRmTool.java

malliaridis · 2024-11-11T10:57:53Z

When I added the tests I noticed a few cases where we may require to patch things. To be precise:

URI is very lenient and allows many invalid strings without throwing URISyntaxException. Using URL in portFromUrl (that is used currently only by StatusTool) may be a good improvement
checkCommunicationError is false if a SolrServerException is thrown with a different root cause (e.g. NPE).

I tried to avoid any changes unrelated to the ticket and we still have many untested methods in this part.

epugh

I put in a couple of comments. Looks like a good direction to take to make SolrCLI less heavy.

epugh · 2024-11-11T16:20:29Z

solr/core/src/java/org/apache/solr/cli/ApiTool.java

@@ -79,7 +79,7 @@ protected String callGet(String url, String credentials) throws Exception {
    URI uri = new URI(url.replace("+", "%20"));
    String solrUrl = getSolrUrlFromUri(uri);
    String path = uri.getPath();
-    try (var solrClient = SolrCLI.getSolrClient(solrUrl, credentials)) {
+    try (var solrClient = CLIUtils.getSolrClient(solrUrl, credentials)) {


In some ways, I am surprised that we don't have a existing "factory" style method like this to get a solr client based on solrUrl and some credentials ;-)

I feel that we are slowly getting there with all the migrations and cleanups. :)

solr/core/src/java/org/apache/solr/cli/CLIUtils.java

epugh · 2024-11-11T16:38:16Z

solr/core/src/java/org/apache/solr/cli/CLIUtils.java

+    return "solrcloud".equals(systemInfo.get("mode"));
+  }
+
+  public static Path getConfigSetsDir(Path solrInstallDir) {


i feel like this logic is elsewhere too? But maybe that is in the Test code...

I believe it was previously in the CLI scripts, and there is one more reference to this directory in RunExampleTool that uses the server dir instead of the install dir:

786 File configsetsDir = new File(serverDir, "solr/configsets");

If we move at some point all path computation to the CLI via options and defaults, we could probably improve and merge these parts. See for reference SolrContextOptions.kt.

@SInCE

- Fix typo - Cleanup TODOs - Remove @SInCE reference

…ovements/cli-utils

Introduce CLIUtils class

a84e589

github-actions bot added tests cat:cloud cat:cli cat:packagemanager labels Oct 6, 2024

malliaridis added 2 commits October 7, 2024 19:11

Replace deprecated SolrCLI references

4b13223

Remove deprecations from SolrCLI

906d505

malliaridis force-pushed the improvements/cli-utils branch from 2d0a510 to 906d505 Compare October 7, 2024 17:11

malliaridis changed the title ~~SOLR-XXXXX: Move out CLI utils from SolrCLI~~ SOLR-17501: Move out CLI utils from SolrCLI Oct 17, 2024

malliaridis added 2 commits November 6, 2024 15:43

Move port extraction to CLIUtils

894672d

Fix remaining merge conflicts

9e35099

malliaridis force-pushed the improvements/cli-utils branch from 1f6dda1 to 9e35099 Compare November 6, 2024 15:33

malliaridis added 2 commits November 11, 2024 10:48

Add a few tests

c3abf3a

malliaridis marked this pull request as ready for review November 11, 2024 10:58

malliaridis requested review from epugh and janhoy November 11, 2024 10:58

epugh approved these changes Nov 11, 2024

View reviewed changes

malliaridis added 4 commits November 12, 2024 13:29

Apply review requests

4e8cc7f

- Fix typo - Cleanup TODOs - Remove @SInCE reference

Run tidy

28df368

Merge remote-tracking branch 'apache/main' into fork/malliaridis/impr…

893f212

…ovements/cli-utils

Fix merge issues

d02eb61

malliaridis merged commit e1f56b3 into apache:main Nov 19, 2024
3 of 4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-17501: Move out CLI utils from SolrCLI #2744

SOLR-17501: Move out CLI utils from SolrCLI #2744

malliaridis commented Oct 6, 2024 •

edited

Loading

epugh commented Oct 17, 2024

malliaridis commented Oct 18, 2024 •

edited

Loading

malliaridis commented Oct 18, 2024

malliaridis commented Nov 6, 2024 •

edited

Loading

malliaridis commented Nov 11, 2024

epugh left a comment

epugh Nov 11, 2024

malliaridis Nov 12, 2024

epugh Nov 11, 2024

malliaridis Nov 12, 2024

SOLR-17501: Move out CLI utils from SolrCLI #2744

SOLR-17501: Move out CLI utils from SolrCLI #2744

Conversation

malliaridis commented Oct 6, 2024 • edited Loading

Description

Solution

Tests

Checklist

epugh commented Oct 17, 2024

malliaridis commented Oct 18, 2024 • edited Loading

malliaridis commented Oct 18, 2024

malliaridis commented Nov 6, 2024 • edited Loading

malliaridis commented Nov 11, 2024

epugh left a comment

Choose a reason for hiding this comment

epugh Nov 11, 2024

Choose a reason for hiding this comment

malliaridis Nov 12, 2024

Choose a reason for hiding this comment

epugh Nov 11, 2024

Choose a reason for hiding this comment

malliaridis Nov 12, 2024

Choose a reason for hiding this comment

malliaridis commented Oct 6, 2024 •

edited

Loading

malliaridis commented Oct 18, 2024 •

edited

Loading

malliaridis commented Nov 6, 2024 •

edited

Loading