malliaridis commented on code in PR #2721:
URL: https://github.com/apache/solr/pull/2721#discussion_r1775057041
##########
solr/bin/solr.cmd:
##########
@@ -362,23 +362,21 @@ goto done
@echo.
@echo --no-prompt Don't prompt for input; accept all defaults when running
examples that accept user input
@echo.
-@echo -v and -q Verbose (-v) or quiet (-q) logging. Sets default log
level to DEBUG or WARN instead of INFO
-@echo.
-@echo -V/--verbose Verbose messages from this script
+@echo -d/--debug and -q/--quiet Debug (-d) or quiet (-q) logging. Sets
default log level to DEBUG or WARN instead of INFO
@echo.
goto done
:stop_usage
@echo.
-@echo Usage: solr stop [-k key] [-p port] [-V]
+@echo Usage: solr stop [-k key] [-p port] [-d]
@echo.
@echo -k key Stop key; default is solrrocks
@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.
-@echo -V/--verbose Verbose messages from this script
+@echo -d/--debug Debug messages from this script
Review Comment:
If we go in the CLI classes with `--debug` without `-d`, then we should
reflect the changes here as well to avoid any inconsistencies.
##########
solr/bin/solr:
##########
@@ -425,21 +425,19 @@ function print_usage() {
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."
echo " It is possible to override this warning
with the '--force' parameter."
echo ""
- echo " -v and -q Verbose (-v) or quiet (-q) logging. Sets
default log level of Solr to DEBUG or WARN instead of INFO"
- echo ""
- echo " -V/--verbose Verbose messages from this script"
+ echo " -d/--debug or -q/--quiet Debug (-d) or quiet (-q) logging. Sets
default log level of Solr to DEBUG or WARN instead of INFO"
echo ""
elif [ "$CMD" == "stop" ]; then
echo ""
- echo "Usage: solr stop [-k key] [-p port] [-V]"
+ echo "Usage: solr stop [-k key] [-p port] [-d]"
echo ""
echo " -k <key> Stop key; default is solrrocks"
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 ""
- echo " -V/--verbose Verbose messages from this script"
+ echo " -d/--debug Debug messages from this script"
Review Comment:
Same for removing `-d` applies here too.
##########
solr/core/src/java/org/apache/solr/cli/CreateTool.java:
##########
@@ -224,17 +224,18 @@ protected void createCore(CommandLine cli, SolrClient
solrClient) throws Excepti
FileUtils.copyDirectoryToDirectory(confDir.toFile(),
coreInstanceDir.toFile());
- echoIfVerbose(
+ echoIfDebug(
"\nCopying configuration to new core instance directory:\n"
+ coreInstanceDir.toAbsolutePath(),
cli);
}
- echoIfVerbose("\nCreating new core '" + coreName + "' using
CoreAdminRequest", cli);
+ echoIfDebug("\nCreating new core '" + coreName + "' using
CoreAdminRequest", cli);
try {
CoreAdminResponse res = CoreAdminRequest.createCore(coreName, coreName,
solrClient);
- if (cli.hasOption(SolrCLI.OPTION_VERBOSE.getOpt())) {
+ if (cli.hasOption((SolrCLI.OPTION_DEBUG.getOpt()))
+ || cli.hasOption(SolrCLI.OPTION_VERBOSE.getOpt())) {
Review Comment:
Same improvement with `OptionGroup` applies here as well.
##########
solr/core/src/java/org/apache/solr/cli/ToolBase.java:
##########
@@ -45,7 +46,9 @@ protected void echo(final String msg) {
@Override
public int runTool(CommandLine cli) throws Exception {
- verbose = cli.hasOption(SolrCLI.OPTION_VERBOSE.getOpt());
+ debug =
+ cli.hasOption((SolrCLI.OPTION_DEBUG.getOpt()))
+ || cli.hasOption(SolrCLI.OPTION_VERBOSE.getOpt());
Review Comment:
`OptionGroup` simplification.
##########
solr/packaging/test/test_bats.bats:
##########
@@ -35,7 +35,7 @@ teardown_file() {
# Conversely, on shutdown, we do need this to execute strictly
# because using "run" will eat failing test exit codes
solr stop --all
- # DEBUG : (echo -n "# " ; solr stop -V --all) >&3
+ # DEBUG : (echo -n "# " ; solr stop -d --all) >&3
Review Comment:
We should use the extended form (`--debug`) here instead
##########
solr/bin/solr:
##########
@@ -425,21 +425,19 @@ function print_usage() {
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."
echo " It is possible to override this warning
with the '--force' parameter."
echo ""
- echo " -v and -q Verbose (-v) or quiet (-q) logging. Sets
default log level of Solr to DEBUG or WARN instead of INFO"
- echo ""
- echo " -V/--verbose Verbose messages from this script"
+ echo " -d/--debug or -q/--quiet Debug (-d) or quiet (-q) logging. Sets
default log level of Solr to DEBUG or WARN instead of INFO"
echo ""
elif [ "$CMD" == "stop" ]; then
echo ""
- echo "Usage: solr stop [-k key] [-p port] [-V]"
+ echo "Usage: solr stop [-k key] [-p port] [-d]"
Review Comment:
support for `-d` is not covered but mentioned in the output. Should probably
be removed here as well.
##########
solr/bin/solr.cmd:
##########
@@ -398,10 +396,11 @@ 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"=="-V" goto set_verbose
-IF "%1"=="--verbose" goto set_verbose
+IF "%1"=="-V" goto set_debug
+IF "%1"=="--verbose" goto set_debug
Review Comment:
`--debug` (and if `-d` is kept `-d` as well) should probably also added to
the list.
##########
solr/core/src/java/org/apache/solr/cli/SolrCLI.java:
##########
@@ -642,8 +665,13 @@ private static void printHelp() {
print(
" Omit '-z localhost:2181' from the above command if you have defined
ZK_HOST in solr.in.sh.");
print("");
- print("Pass --help or -h after any COMMAND to see command-specific usage
information,");
- print("such as: ./solr start --help or ./solr stop -h");
+ print("Global Options:");
+ print(" -v, --version Print version information and quit");
+ print(" -d, --debug Enable debug mode");
Review Comment:
`-d` should probably not included here.
##########
solr/core/src/java/org/apache/solr/cli/CreateTool.java:
##########
@@ -362,7 +363,8 @@ protected void createCollection(CloudSolrClient
cloudSolrClient, CommandLine cli
"Failed to create collection '" + collectionName + "' due to: " +
sse.getMessage());
}
- if (cli.hasOption(SolrCLI.OPTION_VERBOSE.getOpt())) {
+ if (cli.hasOption((SolrCLI.OPTION_DEBUG.getOpt()))
+ || cli.hasOption(SolrCLI.OPTION_VERBOSE.getOpt())) {
Review Comment:
... and here (`OptionGroup`).
##########
solr/core/src/java/org/apache/solr/cli/AssertTool.java:
##########
@@ -131,7 +131,9 @@ public List<Option> getOptions() {
@Override
public int runTool(CommandLine cli) throws Exception {
- verbose = cli.hasOption(SolrCLI.OPTION_VERBOSE.getOpt());
+ debug =
+ cli.hasOption((SolrCLI.OPTION_DEBUG.getOpt()))
+ || cli.hasOption(SolrCLI.OPTION_VERBOSE.getOpt());
Review Comment:
With an `OptionGroup` this could be simplified to `debug =
cli.hasOption(SolrCLI.OPTION_DEBUG)`, where `SolrCLI.OPTION_DEBUG` is the
`OptionGroup` that includes verbose and debug options.
Note that the help outputs may be different if using option groups.
##########
solr/core/src/java/org/apache/solr/cli/ToolBase.java:
##########
@@ -33,8 +33,9 @@ protected ToolBase(PrintStream stdout) {
this.stdout = stdout;
}
- protected void echoIfVerbose(final String msg, CommandLine cli) {
- if (cli.hasOption(SolrCLI.OPTION_VERBOSE.getOpt())) {
+ protected void echoIfDebug(final String msg, CommandLine cli) {
+ if (cli.hasOption((SolrCLI.OPTION_DEBUG.getOpt()))
+ || cli.hasOption(SolrCLI.OPTION_VERBOSE.getOpt())) {
Review Comment:
`OptionGroup` simplification.
##########
solr/core/src/java/org/apache/solr/cli/ConfigTool.java:
##########
@@ -92,12 +92,24 @@ public List<Option> getOptions() {
"Name of the Config API property to apply the action to, such
as: 'updateHandler.autoSoftCommit.maxTime'.")
.build(),
Option.builder("v")
- .longOpt("value")
+ .deprecated(
+ DeprecatedAttributes.builder()
+ .setForRemoval(true)
+ .setSince("9.8")
+ .setDescription("Use --value instead")
+ .get())
.argName("VALUE")
.hasArg()
.required(false)
.desc("Set the property to this value; accepts JSON objects and
strings.")
.build(),
+ Option.builder()
+ .longOpt("value")
+ .argName("VALUE")
+ .hasArg()
+ .required(false) // should be true when -v is removed.
+ .desc("Set the property to this value; accepts JSON objects and
strings.")
+ .build(),
Review Comment:
`OptionGroup` could also be used here and the group may be marked as
required, forcing the user to provide either option. This way, later checks
(`if (value == null)`)are not necessary and
`SolrCLI.getOptionWithDeprecatedAndDefault` may be replaced with
`cli.getOption(optionGroup, null)`.
--
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]
