gerlowskija commented on code in PR #1768:
URL: https://github.com/apache/solr/pull/1768#discussion_r1651013286
##########
solr/bin/solr:
##########
@@ -420,14 +420,14 @@ function print_usage() {
echo " you could pass: -j
\"--include-jetty-dir=/etc/jetty/custom/server/\""
echo " In most cases, you should wrap the
additional parameters in double quotes."
echo ""
- echo " -noprompt Don't prompt for input; accept all defaults
when running examples that accept user input"
+ echo " --noprompt Don't prompt for input; accept all defaults
when running examples that accept user input"
Review Comment:
[Q] Should this be "kebab-ed" to `--no-prompt` here and elsewhere? I see
we've done similar things with `--url-scheme` etc.
##########
solr/bin/solr:
##########
@@ -420,14 +420,14 @@ function print_usage() {
echo " you could pass: -j
\"--include-jetty-dir=/etc/jetty/custom/server/\""
echo " In most cases, you should wrap the
additional parameters in double quotes."
echo ""
- echo " -noprompt Don't prompt for input; accept all defaults
when running examples that accept user input"
+ echo " --noprompt Don't prompt for input; accept all defaults
when running examples that accept user input"
echo ""
echo " -force If attempting to start Solr as the root
user, the script will exit with a warning that running Solr as \"root\" can
cause problems."
Review Comment:
[Q] Should the docs on "-force" be updated to specify '--'?
##########
solr/bin/solr:
##########
@@ -437,9 +437,9 @@ function print_usage() {
echo ""
echo " -p <port> Specify the port the Solr HTTP listener is bound to"
echo ""
- echo " -all Find and stop all running Solr servers on this host"
+ echo " --all Find and stop all running Solr servers on this host"
Review Comment:
[0] Not a problem for this PR, but looking at this code reminds me how odd
it is that `bin/solr` is very inconsistent in where it makes short vs long
options available.
e.g. Many args have both a short and longopt form, but not all. "-s", "-d",
"-p", "-e" don't advertise any longopt version, for instance. And conversely,
"--all", "--noprompt", and a few others don't seem to have any shortopt form.
##########
solr/bin/solr.cmd:
##########
@@ -535,37 +537,41 @@ IF "%1"=="-h" goto usage
IF "%1"=="-usage" goto usage
IF "%1"=="/?" goto usage
IF "%1"=="-f" goto set_foreground_mode
-IF "%1"=="-foreground" goto set_foreground_mode
+IF "%1"=="--foreground" goto set_foreground_mode
IF "%1"=="-V" goto set_verbose
-IF "%1"=="-verbose" goto set_verbose
+IF "%1"=="--verbose" goto set_verbose
IF "%1"=="-v" goto set_debug
IF "%1"=="-q" goto set_warn
IF "%1"=="-c" goto set_cloud_mode
IF "%1"=="-cloud" goto set_cloud_mode
IF "%1"=="-d" goto set_server_dir
-IF "%1"=="-dir" goto set_server_dir
+IF "%1"=="--dir" goto set_server_dir
IF "%1"=="-s" goto set_solr_home_dir
+IF "%1"=="--solr-home" goto set_solr_home_dir
IF "%1"=="-t" goto set_solr_data_dir
-IF "%1"=="-solr.home" goto set_solr_home_dir
+IF "%1"=="--solr-data" goto set_solr_data_dir
IF "%1"=="-e" goto set_example
-IF "%1"=="-example" goto set_example
-IF "%1"=="-host" goto set_host
+IF "%1"=="--example" goto set_example
+IF "%1"=="--host" goto set_host
IF "%1"=="-m" goto set_memory
-IF "%1"=="-memory" goto set_memory
+IF "%1"=="--memory" goto set_memory
IF "%1"=="-p" goto set_port
-IF "%1"=="-port" goto set_port
+IF "%1"=="--port" goto set_port
IF "%1"=="-z" goto set_zookeeper
-IF "%1"=="-zkhost" goto set_zookeeper
+IF "%1"=="--zk-host" goto set_zookeeper
IF "%1"=="-zkHost" goto set_zookeeper
+IF "%1"=="--zkHost" goto set_zookeeper
IF "%1"=="-s" goto set_solr_url
+IF "%1"=="--solr-url" goto set_solr_url
IF "%1"=="-solrUrl" goto set_solr_url
IF "%1"=="-a" goto set_addl_opts
-IF "%1"=="-addlopts" goto set_addl_opts
+IF "%1"=="--additional-options" goto set_addl_opts
IF "%1"=="-j" goto set_addl_jetty_config
-IF "%1"=="-jettyconfig" goto set_addl_jetty_config
-IF "%1"=="-noprompt" goto set_noprompt
+IF "%1"=="--jettyconfig" goto set_addl_jetty_config
Review Comment:
[Q] I asked up in `solr/bin/solr` whether there was a reason that settings
like `--jettyconfig` weren't fully "kebab'd" out. I won't duplicate all those
comments here in the Windows script, but they probably apply here as well.
##########
solr/core/src/java/org/apache/solr/cli/AuthTool.java:
##########
@@ -333,7 +334,7 @@ private int handleBasicAuth(CommandLine cli) throws
Exception {
} while (password.length() == 0);
}
- boolean blockUnknown =
Boolean.parseBoolean(cli.getOptionValue("blockUnknown", "true"));
+ boolean blockUnknown =
Boolean.parseBoolean(cli.getOptionValue("block-unknown", "true"));
Review Comment:
[0] It's probably a job for a different PR, but this diff really points out
how much duplication among all these string-literals. Should probably be
constants that only need changed in one place.
##########
solr/core/src/java/org/apache/solr/cli/RunExampleTool.java:
##########
@@ -217,7 +217,7 @@ public void runImpl(CommandLine cli) throws Exception {
: new File(serverDir.getParent(), "example");
if (!exampleDir.isDirectory())
throw new IllegalArgumentException(
- "Value of -exampleDir option is invalid! "
+ "Value of --exampleDir option is invalid! "
Review Comment:
[Q] Kebab-case?
##########
solr/core/src/java/org/apache/solr/cli/PackageTool.java:
##########
@@ -117,15 +117,18 @@ public void runImpl(CommandLine cli) throws Exception {
}
break;
case "list-deployed":
- if (cli.hasOption('c')) {
- String collection = cli.getArgs()[1];
+ if (cli.hasOption("collection")) {
+ String collection = cli.getOptionValue("collection");
Map<String, SolrPackageInstance> packages =
packageManager.getPackagesDeployed(collection);
PackageUtils.printGreen("Packages deployed on " + collection +
":");
for (String packageName : packages.keySet()) {
PackageUtils.printGreen("\t" + packages.get(packageName));
}
} else {
+ // nuance that we use a arg here instead of requiring a
--package parameter with a
+ // value
Review Comment:
[0] Looks like 'tidy' messed up your code formatting here.
##########
solr/bin/solr:
##########
@@ -1410,13 +1415,13 @@ if [ $# -gt 0 ]; then
SOLR_LOG_LEVEL=WARN
shift
;;
- -all)
+ --all|-all)
stop_all=true
shift
;;
- -force)
+ --force)
Review Comment:
[Q] On some of these lines it looks like we accept both the single and
double-dash variant of the arg. e.g. `--all|-all)` on L1418 above. But for
many others (like `--force` here), we accept only the double-dash variant. Is
this intentional or oversight?
If intentional, is there some pattern I'm missing around when we accept the
single-dash variant?
##########
solr/bin/solr:
##########
@@ -1375,31 +1380,31 @@ if [ $# -gt 0 ]; then
PASS_TO_RUN_EXAMPLE+=("-z" "$ZK_HOST")
shift 2
;;
- -a|-addlopts)
+ -a|--additional-options)
ADDITIONAL_CMD_OPTS="$2"
PASS_TO_RUN_EXAMPLE+=("-a" "$ADDITIONAL_CMD_OPTS")
shift 2
;;
- -j|-jettyconfig)
+ -j|--jettyconfig)
Review Comment:
[Q] Should this be "kebab-ed" out to `--jetty-config`?
##########
solr/bin/solr:
##########
@@ -1321,32 +1326,32 @@ if [ $# -gt 0 ]; then
SOLR_HOME="$2"
shift 2
;;
- -t|-data.home)
+ -t|--data-home)
Review Comment:
[Q] Should we also support `-data.home` here? That seems consistent with
what we've done in other places (e.g. `-solr.home` on L1161)
I guess I"m overall kindof confused where we're trying to remain backwards
compatible, and where we're not. I see enough places accepting the old options
that it's clear backwards-compatibility is an intention. So when it's absent,
I'm not sure whether that's done by oversight or whether there's some other
factor at play deciding where we're trying to be backwards compatible.
##########
solr/core/src/java/org/apache/solr/cli/ConfigSetDownloadTool.java:
##########
@@ -43,23 +43,25 @@ public ConfigSetDownloadTool(PrintStream stdout) {
@Override
public List<Option> getOptions() {
return List.of(
- Option.builder("n")
+ Option.builder()
Review Comment:
[Q] Why does the 'n' shortopt go away here? And should "confname" be
kebab-cased?
##########
solr/bin/solr:
##########
@@ -1002,40 +1002,45 @@ if [[ "$SCRIPT_CMD" == "zk" ]]; then
CONNECTION_PARAMS=""
if [[ -z "$ZK_HOST" ]]; then
- CONNECTION_PARAMS="-solrUrl ${ZK_SOLR_URL}"
+ if [[ -z "$ZK_SOLR_URL" ]]; then
+ CONNECTION_PARAMS=""
+ else
+ CONNECTION_PARAMS="--solr-url ${ZK_SOLR_URL}"
+ fi
+
else
- CONNECTION_PARAMS="-zkHost ${ZK_HOST}"
+ CONNECTION_PARAMS="--zk-host ${ZK_HOST}"
fi
case "$ZK_OP" in
upconfig)
- run_tool "$ZK_OP" --confname "$CONFIGSET_CONFNAME" -confdir
"$CONFIGSET_CONFDIR" $CONNECTION_PARAMS -configsetsDir
"$SOLR_TIP/server/solr/configsets" $VERBOSE
+ run_tool "$ZK_OP" --confname "$CONFIGSET_CONFNAME" --confdir
"$CONFIGSET_CONFDIR" $CONNECTION_PARAMS $VERBOSE
Review Comment:
[Q] Should "confname" and "confdir" get kebab-cased?
##########
solr/core/src/java/org/apache/solr/cli/ExportTool.java:
##########
@@ -97,35 +97,37 @@ public String getName() {
public List<Option> getOptions() {
return List.of(
Option.builder("url")
+ .longOpt("solr-collection-url")
Review Comment:
[Q] So this will accept both `--url` and `--solr-collection-url` with this
change, or does the "longOpt" value overrule the value provided in
`Option.builder(...)`?
##########
solr/bin/solr.cmd:
##########
@@ -1314,7 +1320,7 @@ IF "%FG%"=="1" (
"%JAVA%" %SOLR_SSL_OPTS% %AUTHC_OPTS% %SOLR_ZK_CREDS_AND_ACLS%
%SOLR_TOOL_OPTS% -Dsolr.install.dir="%SOLR_TIP%"
-Dsolr.default.confdir="%DEFAULT_CONFDIR%"^
-Dlog4j.configurationFile="file:///%DEFAULT_SERVER_DIR%\resources\log4j2-console.xml"
^
-classpath
"%DEFAULT_SERVER_DIR%\solr-webapp\webapp\WEB-INF\lib\*;%DEFAULT_SERVER_DIR%\lib\ext\*"
^
- org.apache.solr.cli.SolrCLI status -maxWaitSecs !SOLR_START_WAIT! -solrUrl
!SOLR_URL_SCHEME!://%SOLR_TOOL_HOST%:%SOLR_PORT%/solr
+ org.apache.solr.cli.SolrCLI status --max-wait-secs !SOLR_START_WAIT!
--solr-url !SOLR_URL_SCHEME!://%SOLR_TOOL_HOST%:%SOLR_PORT%
Review Comment:
[Q] I can't find any reference to `--max-wait-secs` in the Linux version of
this code. Might this be something that Solr supports on Windows but *not*
Linux?!?
Anyway, not an issue for this PR, just found it interesting...
##########
solr/core/src/java/org/apache/solr/cli/RunExampleTool.java:
##########
@@ -118,18 +120,21 @@ public List<Option> getOptions() {
.desc("Path to the Solr server directory.")
.longOpt("serverDir")
.build(),
- Option.builder("force")
+ Option.builder("f")
+ .longOpt("force")
.argName("FORCE")
.desc("Force option in case Solr is run as root.")
.build(),
- Option.builder("exampleDir")
+ Option.builder()
+ .longOpt("exampleDir")
Review Comment:
[Q] Kebab-case?
##########
solr/core/src/java/org/apache/solr/cli/SolrCLI.java:
##########
@@ -80,30 +83,72 @@ public class SolrCLI implements CLIO {
public static final String ZK_HOST = "localhost:9983";
+ public static final Option OPTION_ZKHOST_DEPRECATED =
+ Option.builder("zkHost")
+ .longOpt("zkHost")
+ .deprecated(
+ DeprecatedAttributes.builder()
+ .setForRemoval(true)
+ .setSince("9.7")
+ .setDescription("Use --zk-host instead")
+ .get())
+ .argName("HOST")
+ .hasArg()
+ .required(false)
+ .desc(
+ "Zookeeper connection string; unnecessary if ZK_HOST is defined
in solr.in.sh; otherwise, defaults to "
+ + ZK_HOST
+ + '.')
+ .build();
+
public static final Option OPTION_ZKHOST =
Option.builder("z")
- .longOpt("zkHost")
+ .longOpt("zk-host")
.argName("HOST")
.hasArg()
.required(false)
.desc(
"Zookeeper connection string; unnecessary if ZK_HOST is defined
in solr.in.sh; otherwise, defaults to "
+ ZK_HOST
+ '.')
- .longOpt("zkHost")
.build();
- public static final Option OPTION_SOLRURL =
+ public static final Option OPTION_SOLRURL_DEPRECATED =
Option.builder("solrUrl")
+ .longOpt("solrUrl")
+ .deprecated(
+ DeprecatedAttributes.builder()
+ .setForRemoval(true)
+ .setSince("9.7")
+ .setDescription("Use --solr-url instead")
+ .get())
.argName("HOST")
.hasArg()
.required(false)
.desc(
- "Base Solr URL, which can be used to determine the zkHost if
that's not known; defaults to: "
+ "Base Solr URL, which can be used to determine the zk-host if
that's not known; defaults to: "
+ + getDefaultSolrUrl()
+ + '.')
+ .build();
+ public static final Option OPTION_SOLRURL =
+ Option.builder("url")
+ .longOpt("solr-url")
+ .argName("HOST")
+ .hasArg()
+ .required(false)
+ .desc(
+ "Base Solr URL, which can be used to determine the zk-host if
that's not known; defaults to: "
+ getDefaultSolrUrl()
+ '.')
.build();
public static final Option OPTION_VERBOSE =
- Option.builder("verbose").required(false).desc("Enable more verbose
command output.").build();
+ Option.builder("v")
+ .longOpt("verbose")
+ .argName("verbose")
Review Comment:
[-0] Unless I'm wrong, we should be calling `.argName(..)` here, since
'verbose' is a flag that doesn't actually consume an argument.
##########
solr/core/src/java/org/apache/solr/cli/PackageTool.java:
##########
@@ -167,8 +170,8 @@ public void runImpl(CommandLine cli) throws Exception {
Pair<String, String> parsedVersion =
parsePackageVersion(cli.getArgList().get(1));
String packageName = parsedVersion.first();
String version = parsedVersion.second();
- boolean noprompt = cli.hasOption('y');
- boolean isUpdate = cli.hasOption("update") ||
cli.hasOption('u');
+ boolean noprompt = cli.hasOption("noprompt");
Review Comment:
[Q] Kebab-case?
##########
solr/core/src/test/org/apache/solr/cli/AuthToolTest.java:
##########
@@ -64,15 +64,15 @@ public void testEnableAuth() throws Exception {
String[] args = {
"auth",
"enable",
- "-zkHost",
+ "-z",
Review Comment:
[0] You've put in a lot of effort in this PR to make sure that we (at least
temporarily) maintain backcompat with older options. Given all that work, it
seems a shame to not validate it.
One way we could do that is by randomizing which form of different options
gets used in these tests. Some of the more common CLI parameters could be
replaced by calls to a method like:
```
public void getZooKeeperParameter() {
if (rarely()) return "-z";
if (rarely()) return "-zkhost";
return "--zk-host";
}
```
Not important enough to require on this PR, but worth considering if you
like the idea.
--
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]
