Repository navigation
SOLR-17697: Implement picocli for assert, cluster, config, api, export, postlogs, run_example, post, stream and the snapshot-* commands - #4931
Closed
serhiy-bzhezytskyy wants to merge 13 commits into
Conversation
CLIUtils.resolveSolrUrl/resolveSolrConnection resolve a picocli tool's ConnectionOptions (--solr-url/--solr-connection/--zk-host) the same way CLIUtils.normalizeSolrUrl(CommandLine)/getSolrConnection(CommandLine) do for the commons-cli path, so each newly-converted tool does not need to re-implement that resolution itself.
Reuses the existing ConnectionOptions/CredentialsOptions mixins. post-tool.adoc/stream-tool.adoc's rename is a separate, unrelated ticket; this only wires the already-split PostToolParams into picocli fields. Verified against a live Solr instance across all three connection forms (--solr-url, --solr-connection, --zk-host) and the no-connection default, matching the commons-cli path's output byte for byte, including its stderr fallback warning.
Mirrors CLIUtils.getSolrConnection(CommandLine)'s cluster-probe fallback locally, since StreamTool (unlike most tools) needs both a resolved CloudSolrClientConnection for local-mode streaming and a plain Solr URL for remote mode. Verified live across --solr-url, --solr-connection, --zk-host and the no-connection default, in both --execution local and remote, matching the commons-cli path's output and stderr warnings.
Four ArgGroups (root/not-root, started/not-started, exists/not-exists, cloud/not-cloud) replicate the commons-cli OptionGroups' mutual exclusivity. callTool() inlines the same try/catch as the commons-cli path's runTool() override, since AssertTool is the one tool that maps a failed assertion to exit code 100 rather than ToolBase's default of 1.
Uses the new CLIUtils.resolveSolrConnection helper; healthcheck only works in cloud mode, so a null resolution still prints the same error and exits 1 as the commons-cli path.
This tool only ever exposed --zk-host (not the full connection group), so its own resolveZkHost() mirrors CLIUtils.getZkHost(CommandLine) directly rather than reusing ConnectionOptions. Verified live: writes to ZooKeeper's /clusterprops.json exactly as the commons-cli path does.
--value drops its "-v" short form: that letter is already ToolBase's --verbose, and the two silently collide in the commons-cli path today (VALUE_OPTION is added after VERBOSE_OPTION, so -v currently means --value there, not --verbose); picocli refuses the duplicate outright. Verified live against a real Solr instance, including the "value is required unless the action is unset-*" validation.
Verified live: same GenericSolrRequest/JsonMapResponseParser path, same output, against a real Solr instance.
The connection group is required (ArgGroup multiplicity "1"): the commons-cli path throws IllegalArgumentException when no connection target is given, so this makes picocli enforce the same requirement declaratively instead.
Same mandatory-connection-group treatment as ExportTool. Verified live against a real log file; a pre-existing LogRecordReader parsing quirk on certain QTime lines (NPE on a null params string) reproduces identically on both parsers, confirming it predates this conversion.
--port keeps no explicit defaultValue: its paramLabel "port" matches CliDefaultValueProvider's <port> case (solr.port.listen sysprop / SOLR_PORT_LISTEN env var, else 8983), and --zk-host's paramLabel "zkHost" does the same for the <zkHost> case, mirroring the commons-cli path's fallbacks without duplicating them.
snapshot-create, snapshot-delete, snapshot-describe, snapshot-export and snapshot-list, bundled as one commit since they share the same shape (collection name + optional snapshot name + connection group) and are one cohesive unit of the CLI's snapshot lifecycle. snapshot-export keeps accepting --snapshot-name only to reject it with the same explanation as the commons-cli path (removed non-incremental backup format). Verified the full lifecycle live: create, list, describe, delete.
Wires PostTool, StreamTool, AssertTool, HealthcheckTool, ClusterTool, ConfigTool, ApiTool, ExportTool, PostLogsTool, RunExampleTool and the five snapshot-* tools into SolrCLI's @command(subcommands = ...), the one file every tool conversion in this batch shares.
github-actions
Bot
added
documentation
github-actions
Bot
removed
client:solrj
module:extraction
module:jwt-auth
module:ltr
module:opentelemetry
module:s3-repository
tool:build
module:gcs-repository
module:sql
admin-ui
tests
cat:search
cat:cloud
cat:index
cat:api
cat:schema
scripts
module:clustering
cat:security
cat:packagemanager
cat:metrics
module:langid
module:scripting
module:cuvs
module:language-models
module:cross-dc
labels
Contributor
|
@serhiy-bzhezytskyy this got closed unintentionally after merging the feature-branch PR. I expected GH to auto move this PR tonto main as it usually does, but no. Please try to edit target branch yourself on this PR, if you can't do it I'm afraid you'll have to open a new PR. Also FYI, each remaining tool or group of tools that are not yet ported now have separate JIRA issues, see https://issues.apache.org/jira/issues/?jql=project%20%3D%20SOLR%20AND%20text%20~%20%22port%20picocli%22 Ideally there will be one PR per JIRA/Tool for easier review. This huge PR adds 1317 lines of code and what will happen is that it wil be hard to get reviewers for it. Also, I made a LLM prompt to capture some best practices from other ports, see link in each JIRA. Feel free to ask your LLM to consider that info - perhaps there are useful improvements to be found. |
Contributor
Author
|
@janhoy hey, thanks, i will do as you suggested - split PR per tool/jira and etc |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://issues.apache.org/jira/browse/SOLR-17697
@janhoy @dsmiley @epugh
Converts 11 of the remaining unconverted tools to picocli:
assert,cluster,config,api,export,postlogs,run_example(bin/solr start -e),post,stream, and the fivesnapshot-*commands (snapshot-create,snapshot-delete,snapshot-describe,snapshot-export,snapshot-list).PackageToolis intentionally left out - #4739 is already open for it.Each tool reuses the existing
ConnectionOptions/CredentialsOptions/ZkConnectionOptionsmixins where applicable. Two new shared helpers were added toCLIUtils(resolveSolrUrl/resolveSolrConnection) so tools needing the commons-cli path'snormalizeSolrUrl(CommandLine)/getSolrConnection(CommandLine)cluster-probe behavior don't each reimplement it.Two small, unavoidable divergences from the commons-cli path, both because picocli enforces at construction time what commons-cli only silently shadows:
ConfigTool --valuedrops its-vshort form.-vis alreadyToolBase's--verbose, and the commons-cli path already has them colliding (VALUE_OPTIONis registered afterVERBOSE_OPTION, so-vcurrently resolves to--value, not--verbose); picocli refuses the duplicate outright.ExportTool/PostLogsTool's connection group is declaredmultiplicity = "1"(mandatory) instead of manually throwingIllegalArgumentExceptionwhen absent, since both already require a connection unconditionally.Verified:
./gradlew :solr:core:test --tests "org.apache.solr.cli.*"- 152 tests, 2 skipped, all green.--helpstarts cleanly underSolrCLIPicocliTest.testEveryCommandSupportsHelp.SOLR_PICOCLI=true, across all three connection forms plus the no-connection default:config(set/unset + validation),api(GET),cluster(ZK write, confirmed viazk cp), the fullsnapshot-*lifecycle (create, list, describe, delete, plus export's--snapshot-namerejection),assert(both outcomes, exit code 100, mutual exclusion),postlogs(including a pre-existingLogRecordReaderparsing quirk that reproduces identically on both parsers).AI-assisted (Claude Sonnet 5).