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: [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]
