dsmiley commented on code in PR #4320: URL: https://github.com/apache/solr/pull/4320#discussion_r3328795004
########## solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-10.adoc: ########## @@ -57,6 +57,7 @@ The universal connection string is now supported in the following components and * Added `withDefaultSolrConnection` and `withCollectionSolrConnection` methods to `StreamFactory` in the SolrJ Streaming module. * Solr SQL JDBC driver now supports both HTTP-based (`jdbc:solr:http://solr1.example.com:8983/solr?collection=COLLECTION_NAME`) and ZooKeeper-based (`jdbc:solr://zoo.example.com:2181/solr?collection=COLLECTION_NAME`) JDBC URLs. * Added `--solr-connection` option to the CLI tools. +* Updated `-s` short option to invoke `--solr-connection` instead of `--solr-url`, which is a backwards compatible change. Review Comment: should mention "CLI" here ########## solr/packaging/test/test_zk.bats: ########## Review Comment: @epugh what's going on here? "solr zk" uses an HTTP URL to read ZK ?! My expectation is that "solr zk" is HTTP free, Solr node free. ########## solr/core/src/java/org/apache/solr/cli/ExportTool.java: ########## @@ -281,16 +281,16 @@ public void streamDocListInfo(long numFound, long start, Float maxScore) {} @Override public void runImpl(CommandLine cli) throws Exception { String url; - if (cli.hasOption(CommonCLIOptions.SOLR_URL_OPTION)) { + if (CLIUtils.hasConnectionOption(cli)) { if (!cli.hasOption(COLLECTION_NAME_OPTION)) { throw new IllegalArgumentException( - "Must specify -c / --name parameter with --solr-url to post documents."); + "Must specify -c / --name parameter with a connection target to export documents."); } url = CLIUtils.normalizeSolrUrl(cli) + "/solr/" + cli.getOptionValue(COLLECTION_NAME_OPTION); } else { - // think about support --zk-host someday. - throw new IllegalArgumentException("Must specify --solr-url."); + throw new IllegalArgumentException( + "Must specify a connection target via -s/--solr-connection, --solr-url, or --zk-host."); Review Comment: Does the CLI mechanism have a way of enforcing required options like this so that we don't have to do this? If not...I suspect this is going to need to be repeated and we should have a `CLIUtils` method like getRequiredSolrConnection(), likely conveniently returning the parsed string. Just a thought; no conviction on the idea / name. -- 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]
