SOLR-18515 | PackageTool Migration to PicoCLI

[jaykay12]

Description

Implementing PicoCLI for the PackageTool

Past PR for reference (some code review took place here) - #4739

Solution

Used Prompt provided in the Jira & got initial migration done by Cursor.

Majoryly, common code which was being used in the commons-cli call path, which could be used in picocli as well, is being refactored to private functions.

#4739 (comment)

Tests

Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.

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
• I have added a changelog entry for my change

[jaykay12]

@janhoy can please trigger the workflows on this PR?

[pending-on-me] test these commands locally with respect to main

[jaykay12]

List-Available

Command	main branch	picoCLI enabled feature branch
Happy Path
-	-	-
Failure Path

[jaykay12]

Add-Repo

Command	main branch	picoCLI enabled feature branch
Happy Path
-	-	-
Failure Path

[jaykay12]

Add-Key

Command	main branch	picoCLI enabled feature branch
Happy Path
-	-	-
Failure Path

[jaykay12]

List-Installed

Command	main branch	picoCLI enabled feature branch
Happy Path
-	-	-
Failure Path

[jaykay12]

Install

Command	main branch	picoCLI enabled feature branch
Functional Correctness
-	-	-
Happy Path
-	-	-
Failure Path

[jaykay12]

Uninstall

This was not being getting tested properly from local, raised bug fix PR: #5052 for the same & continued testing using that patch over main & feature branch.

Command	main branch	picoCLI enabled feature branch
Functional Correctness(Happy Path)
-	-	-
Failure Path

[jaykay12]

List Deployed

Command	main branch	picoCLI enabled feature branch
Happy Path
-	-	-
Failure Path

[jaykay12]

Deploy

Command	main branch	picoCLI enabled feature branch
Happy Path
-	-	-
Failure Path

[jaykay12]

Undeploy

Command	main branch	picoCLI enabled feature branch
Happy Path
-	-	-
Failure Path

[dsmiley]

wow; one ref guide page per command? It has a javadoc feel to me that I wonder should be published separately, not unlike how javadoc is separated from the ref guide. Maybe we don't need ref guide pages if the CLI is self documenting. Less to maintain!
CC @epugh @janhoy

[janhoy]

The BATS tests fail, not because of this PR but a pre-existing bug, already fixed on main. I'll merge in main here now.

[Copilot]

🟡 Changes recommended

Connection resolution can mix clusters, and generated documentation displays invalid command syntax.

2 open findings

• Prevent Mixed-Cluster Solr and ZooKeeper Client Selection · New
• Preserve Hyphenated Picocli Command Names in Generated Docs · New

What changed in this PR

Migrates PackageTool to native picocli subcommands while retaining commons-cli compatibility.

Changes:

• Adds nine package subcommands with shared connection and manager handling.
• Reuses integration tests across both parser paths.
• Adds generated CLI documentation and a changelog entry.

File	Description
solr-package.adoc	Documents the package command.
solr-package-uninstall.adoc	Documents uninstall.
solr-package-undeploy.adoc	Documents undeploy.
solr-package-list-installed.adoc	Documents installed-package listing.
solr-package-list-deployed.adoc	Documents deployment listing.
solr-package-list-available.adoc	Documents available-package listing.
solr-package-install.adoc	Documents installation.
solr-package-deploy.adoc	Documents deployment.
solr-package-add-repo.adoc	Documents repository addition.
solr-package-add-key.adoc	Documents trusted-key addition.
cli/​index.adoc	Lists migrated package commands.
deployment-nav.adoc	Adds package documentation navigation.
PackageToolTest.java	Makes tests reusable between parsers.
PackageToolPicocliTest.java	Runs shared tests through picocli.
SolrCLI.java	Registers PackageTool.
PackageUninstall.java	Implements uninstall parsing.
PackageUndeploy.java	Implements undeploy parsing.
PackageTool.java	Refactors shared package operations.
PackageSubCommand.java	Provides shared subcommand wiring.
PackageListInstalled.java	Implements installed-package listing.
PackageListDeployed.java	Implements deployment listing.
PackageListAvailable.java	Implements available-package listing.
PackageInstall.java	Implements installation parsing.
PackageDeploy.java	Implements deployment parsing.
PackageAddRepo.java	Implements repository addition.
PackageAddKey.java	Implements trusted-key addition.
ConnectionOptions.java	Adds package connection resolution.
SOLR-18515-package-tool-picocli.yml	Records the migration.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[jaykay12]

@janhoy will that be okay, if i take this up in a follow-up PR?

delta changes here will be clearly visible in the evidences of that PR. That PR will follow-up immediately once this is merged.

[jaykay12]

comments addressed ✅
local testing done for all 9 subcommands ✅

@janhoy can you trigger the workflows now? precommit check is passing on local for the most latest changes. verified ✅

[janhoy]

I merged latest main into the branch to resolve the conflicts with the other picocli ports. Running the full org.apache.solr.cli.* test package turned up one real problem, and a convention pass against the already-merged ports turned up a few more. Details below.

Flaky failure in PackageToolPicocliTest.testPackageTool

PackageToolPicocliTest fails roughly a third of the time (e.g. -Ptests.seed=CE8CB19A405A7699 fails ~3 of 4 runs), while PackageToolTest passes with the same seed. The failing step is the second install question-answer in the random "auto-update to latest" branch, which returns exit code 1 with Failed verification after deployment.

Root cause is a pre-existing race that the commons-cli tool masked: RepositoryManager.install verifies the $LATEST-pegged collections immediately after installing the new version, a few milliseconds before the nodes have reloaded the package, so verify sees Actual: 1.0.0, expected: 1.1.0. The old PackageTool.install() was void, so the legacy path printed installation failed and still exited 0. Your PackageInstall correctly propagates the result, which is the right behavior, but it exposes the race.

I think the fix belongs in RepositoryManager.install: retry the post-install verification with a bounded wait (the reload is asynchronous by design) instead of verifying once. Happy to discuss if you see a better option. Note that testDeployValidationMessages creating validation-test on conf1 also makes it share PKG_VERSIONS with abc, so list-deployed reports it as deployed; isolating it on its own configset is a small tidy-up but does not fix the race on its own.

[janhoy]

Flaky failure in PackageToolPicocliTest.testPackageTool

Turns out this is far from a flaky test. It's a race condition with zk watches. My plan is to let Claude make a fix, apply it, merge this to main and then let @jaykay12 handle follow-ups along with the pre-existing bin/solr package list available bug. Hope that is ok with you Jalaz. This is the last tool to land so I want to prep them all for 10.2

[janhoy]

🤖 Bug fix and pr comment below written by Claude Fable

---

Flaky PackageToolPicocliTest.testPackageTool and the fix

Running org.apache.solr.cli.* on this branch, PackageToolPicocliTest failed in roughly a third of runs while PackageToolTest always passed. With -Ptests.seed=CE8CB19A405A7699 the picocli test failed 7 of 8 runs. The failing step is the second install question-answer in the random "auto-update to latest" branch of testPackageTool, which exits 1 after Actual: 1.0.0, expected: 1.1.0 / Failed verification after deployment.

This is not a bug in the port. The commons-cli PackageTool.install() was void, so the legacy path printed installation failed and still exited 0, which hid a pre-existing server-side race. The new PackageInstall correctly propagates the result, which is why the test now sees it.

The race. RepositoryManager.install adds the new version through POST /api/cluster/package {add: ...} and then immediately verifies the collections pegged to $LATEST. On the server, PackageAPI.Edit.add writes packages.json and calls notifyAllNodesToSync(znodeVersion), and each node answers via Read.syncToVersion. That method considered a node synced as soon as pkgs.znodeVersion had reached the expected version. But that field is bumped by the node's own ZK watcher thread at the start of refreshPackages, before SolrPackageLoader.refreshPackageConf() has built the new version's classloader and reloaded the plugins. So when the watcher fires first, which is the common case, the sync request returns instantly while the reload is still in flight, add returns to the client, and the verify reads the old plugin version. The explicit refresh command had the same hole: notifyListeners would run against a SolrPackage whose version list the watcher thread had not finished updating, so it reloaded nothing. I first tried posting a refresh from the client before verifying, the way the deploy path does, and it failed deterministically for exactly that reason.

The fix (separate commit on this branch, 3ca62e67c71) is server-side and small:

• SolrPackageLoader.refreshPackageConf() and notifyListeners() are now synchronized, so a sync or refresh request blocks until any application of packages.json already started by the ZK watcher has completed, and is a no-op afterwards.
• PackageAPI.Read.syncToVersion always goes through refreshPackageConf() once the expected version is visible, instead of skipping it when the watcher had already bumped the version number.

Lock order stays one-directional (loader monitor, then PackageListeners, then PackagePluginHolder), and nothing on the listener path calls back into the loader, so there is no new deadlock risk.

Verification. The failing seed now passes 8 of 8 runs, both PackageToolTest and PackageToolPicocliTest pass on repeated random seeds, and org.apache.solr.pkg.*, org.apache.solr.filestore.* and TestContainerPlugin all pass. gradlew check -x test is green.

A second small commit (7ee9561c11c) tidies the test: testDeployValidationMessages created validation-test on conf1, the same configset as abc, so after a deploy to abc the package also showed as deployed on validation-test via the shared PKG_VERSIONS params. It now gets its own configset so the two test methods do not influence each other. Both twins pass on repeated runs and list-deployed reports only abc.