gerlowskija commented on code in PR #1670: URL: https://github.com/apache/solr/pull/1670#discussion_r1256262139
########## solr/core/src/java/org/apache/solr/cli/HealthcheckTool.java: ########## @@ -60,11 +60,11 @@ public List<Option> getOptions() { SolrCLI.OPTION_SOLRURL, SolrCLI.OPTION_ZKHOST, Option.builder("c") - .argName("COLLECTION") + .longOpt("name") + .argName("NAME") .hasArg() .required(false) Review Comment: [-1] AFAICT from [this bin/solr code](https://github.com/apache/solr/pull/1670/files#diff-49c29c8f653341f48008c38f5f2cf970fa430cdca29181c819d7bdcfcc722980L1003), the collection parameter should be required for the healthcheck tool. Prior to this commit, we enforced this requirement in the bash/Windows-scripts, but now that we don't parse args or enforce required params there any more - we should update the `.required(...)` call here to be correct. (Assuming I'm reading the old `bin/solr` code correctly at least; always possible I'm wrong.) ########## solr/bin/solr: ########## @@ -937,8 +814,8 @@ function stop_solr() { if [ $# -eq 1 ]; then case $1 in - -help|-h) - print_usage "" + -help|-h) + run_tool "" Review Comment: [Q] Does nothing in `tidy`/`check` complain about trailing whitespace? I don't care much, but I'm surprised given how we've leaned into code-formatting in recent years. ########## solr/core/src/java/org/apache/solr/cli/HealthcheckTool.java: ########## @@ -60,11 +60,11 @@ public List<Option> getOptions() { SolrCLI.OPTION_SOLRURL, SolrCLI.OPTION_ZKHOST, Option.builder("c") - .argName("COLLECTION") + .longOpt("name") Review Comment: [Q] I've picked HealthcheckTool's handling of its "collection" parameter as a concrete example, but the concern below applies in a number of places. ---- Historically, `bin/solr healthcheck` has accepted the collection name using either `-c` or `-collection`. But with this PR, AFAICT it now accepts `-c` and `--collection` (note the double-dash in the longopt). Personally, I like the new syntax better - double-dashes on longopts are pretty ubiquitous in other CLI tools, and Solr's usage of `-longoptName` always struck me as quirky. That said though, it's a backwards incompatible change so we need to roll it out carefully. If you **do** plan on committing some form of this to branch_9x, do you have plans for addressing backcompat? (i.e. Would the branch_9x version accept both `--collection` and `-collection` as longopt forms?) If you **don't** plan on committing some form of this to branch_9x, things are better from a backcompat perspective but we'd still need to call out the syntax changes in CHANGES.txt or our Upgrade Notes, presumably?Do you have a general sense or (even better) a list of the syntax changes that'd need documented? -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org