solr icon indicating copy to clipboard operation
solr copied to clipboard

SOLR-6962: bin/solr stop -a should complain about missing parameter

Open epugh opened this issue 1 year ago • 1 comments

https://issues.apache.org/jira/browse/SOLR-6962

Description

make sure if there is a -a, there is a following parameter.

There IS an argument that maybe the -a should be used instead as a short cut for --all, as that is more likely to be typed. However we can decide that in a separate ticket.

Solution

add check

Tests

manually

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

epugh avatar Aug 27 '24 10:08 epugh

@rahulgoswami I would love your review of the windows changes...

epugh avatar Aug 27 '24 13:08 epugh

@rahulgoswami I would love your review of the windows changes...

Reviewing this now...

rahulgoswami avatar Sep 08 '24 16:09 rahulgoswami

I tried reproducing the issue. Built the main branch and did "solr start -e techproducts". Thereafter tried "solr stop -a" but I get a nice message shown below instead of the hanging behavior the Jira talks about.

I see the branch is still unmerged. What am I missing ? How can I reproduce this ?

solr stop -a

ERROR: Must specify the port when trying to stop Solr, or use --all to stop all running nodes on this host.

Probably my most used feature with -a option is passing the jdwp parameters for remote debugging

solr start -p 8983 -s "C:\Work\Git\ApacheSolr\solr\solr\packaging\build\dev\example\techproducts\solr" -a "-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=18983"

Tried this without providing any value for -a , but that doesn't hang either (and runs fine in fact). Could this be a Linux specific issue?!

rahulgoswami avatar Sep 08 '24 21:09 rahulgoswami

This could be a linux specific issue. The stop command is written all in script (not in java code), so the implmentation on linux is parallel to windows.

Could you also try this branch? I wonder if the edits I made to bin/solr.cmd are actually not needed?

epugh avatar Sep 09 '24 10:09 epugh

I tried main as well as the tag for the latest 9.x release "releases/solr/9.7.0" on Windows. I am observing the same behavior in both cases where "solr stop -a" doesn't hang and gives a meaningful message. Also tried this branch and it seems fine, although with a different message as per the changes made in this PR. I think the solr.cmd changes are not required.

rahulgoswami avatar Sep 11 '24 03:09 rahulgoswami

thanks @rahulgoswami I am going to back out the solr.cmd changes!

epugh avatar Sep 11 '24 10:09 epugh

Okaym, I dug more into this, and I think I see another problem! It turns out that we parse the args for the start/stop/restart commands separate from actually invoking the command.

This means you don't get an error message for passing a start arg, like --jvm-opts to a stop commmand. That's annoying.. and a reason to move these commands to Java code and use commons-cli more!

epugh avatar Sep 11 '24 10:09 epugh

Okay, I just did some testing of bin/solr and this is what I get (and expect):

 dev git:(SOLR-6962) ✗ bin/solr stop -a

ERROR: Jvm options are required when using the -a option!

Usage: solr stop [-k key] [-p port] [-V]

and for fun:

➜  dev git:(SOLR-6962) bin/solr stop --jvm-opts

ERROR: Jvm options are required when using the --jvm-opts option!

Usage: solr stop [-k key] [-p port] [-V]

@rahulgoswami can you take one more looksee at the change I made to bin/solr.cmd and we can call this done! Thanks for your help!

epugh avatar Sep 11 '24 10:09 epugh

Opened PR https://github.com/epugh/solr/pull/10 against this branch.

currently below usage of -a in solr start doesn't raise any alert and starts solr successfully:

solr start -a -p 8983 -s "C:\Work\Git\ApacheSolr\solr\solr\packaging\build\dev\example\techproducts\solr"

PR will make the behaviour consistent with Linux. Now displays "ERROR: JVM options are required when using the -a or --jvm-opts option"

Also I have a different opinion on solr stop behavior. In the new implementation on Linux, solr stop -a would say "Jvm options are required when using the -a option!" . But truly what the user's intent is to stop all instances .

On Windows, the user gets a message "ERROR: Must specify the port when trying to stop Solr, or use --all to stop all running nodes on this host." which imo guides them better than the Linux implementation. For this reason I have retained this Windows message in my PR. Thoughts?

Happy to make the change for Linux too, but would need your help for validation. :)

rahulgoswami avatar Sep 15 '24 05:09 rahulgoswami