SOLR-18118: Add in --prompts to bin/solr start -e cloud to automatically respond to prompts#4127
SOLR-18118: Add in --prompts to bin/solr start -e cloud to automatically respond to prompts#4127epugh wants to merge 8 commits intoapache:mainfrom
Conversation
|
@rahulgoswami could you check out the |
malliaridis
left a comment
There was a problem hiding this comment.
--prompt=2,8983,8984,"mycollection",2,2,_default
This seems kind of cryptic and the order can easily be messed up. It is very easy to make mistakes, imagine following scenarios:
User confuses the order of ports and shards / replicas, the prompt:
--prompts=2,2,2,"mycollection",8983,8984,_defautOr the user used
--prompts=2,8983,8984,"mycollection",2,2,_defautand now comes up with "oh, lets try 3 instead of 2 nodes" and just updates the prompts like
--prompts=3,8983,8984,"mycollection",2,2,_defautor user may think using it like
--prompts 3 8983 8984 "mycollection" 2 2 "_default"So I feel like there are too many wrong inputs possible.
I would rather prefer more params where we clearly distinguish the input data too. If it is supposed for automation, we still should maintain the readability and avoid "magic numbers". I would personally prefer options like --node-count=2, --node-ports=8983,8984 and so on, so that it is at least clear what the user is setting and the order doesn't matter.
Although not exactly the same, clig.dev says:
If you’ve got two or more arguments for different things, you’re probably doing something wrong.
source
The windows script also fails with
> bin\solr.cmd start -e cloud --prompts 3,8983,8984,8985,"my_collection",6,3,"_default"
Invalid command-line option: 3
Usage: solr start [-f] [--user-managed] [...]It seems that CMD is splitting the params by commas, so it fails after trying to parse the argument 3. In order to make it work I had to make the following changes:
:set_prompt
set "PASS_TO_RUN_EXAMPLE=--prompts %2 !PASS_TO_RUN_EXAMPLE!"
SHIFT # <-- Add a second shift to remove the argument of --prompts
SHIFT
goto parse_argsAdditionally update --prompt to --prompts (see other comments), and then run the command as
> bin\solr.cmd start -e cloud --prompts "3,8983,8984,8985,my_collection,6,3,_default" so that it is not split by comma (annoying, I know).
With these changes it worked on Windows, but I do not like it so much. The texts that are printed give the false impression that the user is supposed to make some prompt (they should be suppressed instead).
The --prompts is also specific to --e cloud, is it supposed to work for the other examples as well? If not, should it be listed in the start command, if it is only valid for -e cloud?
|
yeah, so there are some weasel words in the description of |
|
Partial review since I haven't had a chance to try out the .cmd changes yet. I have to agree with @malliaridis assessment that there is no way one would remember this order. On the flip side, introducing a CLI parameter for each individual value feels like an overkill. I do feel this can be mitigated through documentation both in the .adoc and especially in the CLI options itself. May I also suggest changing "--prompt" to something like "--default-cloud-options" or "--cloud-options-list" ? The current choice of name wasn't intuitive to me at all until I looked at the diff. |
| + | ||
| For example, when using the "cloud" example, can start a three node cluster on specific ports: | ||
| + | ||
| *Example*: `bin/solr start -e cloud --prompts 3,9000,9001,9002,"mycollection",2,2,_defaults` |
There was a problem hiding this comment.
Can we document the order ?
[number of nodes, comma separated port for each node, collection-name...]
There was a problem hiding this comment.
what if we call it --example-prompts? Or --example-inputs? The docs say --no-prompt is for anything that takes user supplied prompts... --user-inputs?.
There was a problem hiding this comment.
--user-inputs could work. --prompt-inputs could be another candidate?
There was a problem hiding this comment.
is "prompts" too overloaded in our AI times? How about --example-inputs to relate it to our examples?
There was a problem hiding this comment.
though of course then we have --no-prompts... Now I come back to --prompt-inputs.
| .argName("VALUES") | ||
| .desc( | ||
| "Provide comma-separated values for prompts. Same as --no-prompt but uses provided values instead of defaults. " | ||
| + "Example: --prompts 3,8983,8984,8985,\"gettingstarted\",2,2,_default") |
There was a problem hiding this comment.
Can we document the order here too like in the .adoc? I feel documenting here might be more important, since if my usage pattern is any indicator of a wider usage pattern, users might prefer the CLI options telling them the expectation rather than them having to look up documentation for the same.
There was a problem hiding this comment.
or could --prompts take a json {"nodes":3,"port":"8983"}? Or take name of a properties file? It's an expert option so need not be beautiful
There was a problem hiding this comment.
yeah.. I worry about building something that is more code then the value... and, the number of items changes depending on inputs. so if it's one node, you are 1,8983,gettingstarted,2,2,_default, and if it's four, 4,9000,9001,9003,9002,"gettingstarted\",2,2,_default. Now imagaine we add some more prompts, then it will all change...
There was a problem hiding this comment.
Passing a formatted json in CLI could quickly get tricky with the quote escaping etc, especially on Windows. I like the properties file idea. Somewhat like we have for basic auth in solr.in.sh and solr.in.cmd with "SOLR_AUTHENTICATION_OPTS" (https://solr.apache.org/guide/solr/latest/deployment-guide/basic-authentication-plugin.html#using-the-solr-control-script-with-basic-auth). It can take the actual credentials as well as path to a file.
I am ok if the properties file enhancement comes as a second wave (one can hope!) and we could still use it against the same --prompts param.
|
Maybe we should add a |
| IF "%1"=="--jettyconfig" goto set_addl_jetty_config | ||
| IF "%1"=="-y" goto set_noprompt | ||
| IF "%1"=="--no-prompt" goto set_noprompt | ||
| IF "%1"=="--prompt"s goto set_prompts |
There was a problem hiding this comment.
misplaced quote in "--prompts" breaks command
https://issues.apache.org/jira/browse/SOLR-18118
Description
Populate the prompts that bin/solr start -e cloud makes with preset answers. Prompts.... in the old school sense of the word ;-)
This would be MOST valuable if we could backport it to 9x, so we can test 9 against 10. However, even if it only makes it into 10.1, well, we can test 11 and 10 ;-)
Solution
A bit of refactoring in how we read in choices. A new
--prompt=2,8983,8984,"mycollection",2,2,_defaultsetting that provides comma delimited values.Tests
Added a test and manual testing.