jaykay12 commented on code in PR #5063:
URL: https://github.com/apache/solr/pull/5063#discussion_r4341892601
##########
solr/core/src/java/org/apache/solr/cli/CliDefaultValueProvider.java:
##########
@@ -16,21 +16,36 @@
*/
package org.apache.solr.cli;
+import java.lang.reflect.Field;
import org.apache.solr.common.util.EnvUtils;
import picocli.CommandLine;
-/** Provides default values for CLI arguments. */
+/**
+ * Fills picocli options from the environment (solr.in.sh variables, system
properties) when they
+ * are not given on the command line, keyed by the option's long name.
+ */
public class CliDefaultValueProvider implements
CommandLine.IDefaultValueProvider {
+
@Override
public String defaultValue(CommandLine.Model.ArgSpec argSpec) throws
Exception {
- return switch (argSpec.paramLabel()) {
- case "<zkHost>" -> EnvUtils.getProperty("zkHost");
- case "<solrConnection>" -> EnvUtils.getProperty("solr.connection");
- case "<solrUrl>" -> EnvUtils.getProperty("solr.url");
+ if (!(argSpec instanceof CommandLine.Model.OptionSpec option)) {
+ return null;
+ }
+ return switch (option.longestName()) {
+ case "--zk-host" -> EnvUtils.getProperty("zkHost");
+ case "--solr-connection" -> EnvUtils.getProperty("solr.connection");
+ // Only the shared option means a base URL; a tool's own --solr-url
(ApiTool's full endpoint
+ // URL) must not be filled from SOLR_URL
+ case "--solr-url" ->
+ declaredIn(option, ConnectionOptions.class) ?
EnvUtils.getProperty("solr.url") : null;
Review Comment:
[Q] why do we need this? separate utility function of declaredIn() ?
It was not earlier required.
##########
solr/core/src/java/org/apache/solr/cli/CLIUtils.java:
##########
@@ -247,48 +247,6 @@ public static String normalizeSolrUrl(CommandLine cli)
throws Exception {
return solrUrl;
}
- /**
- * The picocli counterpart of {@link #normalizeSolrUrl(CommandLine)}: {@code
--solr-url}, else the
- * connection named by {@code --solr-connection} or {@code --zk-host} (or
the matching property
- * when the option is absent), else the default URL with the same warning.
- *
- * @param connectionOptions the parsed connection group, or null if none of
its options was given
- */
- static String resolveSolrUrl(ConnectionOptions connectionOptions, String
credentials)
Review Comment:
👏🏽 for the cleanup.
##########
solr/core/src/java/org/apache/solr/cli/DeleteTool.java:
##########
@@ -164,30 +157,11 @@ public void runImpl(CommandLine cli) throws Exception {
@Override
public int callTool() throws Exception {
- String zkHostArg =
- (connectionOptions != null)
- ? connectionOptions.effectiveZkHost()
- : EnvUtils.getProperty("zkHost");
- String solrUrlArg = (connectionOptions != null) ?
connectionOptions.effectiveSolrUrl() : null;
- String credentials = (credentialsOptions != null) ?
credentialsOptions.credentials : null;
-
- String resolvedSolrUrl;
- if (solrUrlArg != null) {
- resolvedSolrUrl = CLIUtils.normalizeSolrUrl(solrUrlArg);
- } else if (zkHostArg != null) {
- resolvedSolrUrl =
- CLIUtils.solrUrlFromConnection(
- CloudSolrClient.CloudSolrClientConnection.parse(zkHostArg),
credentials);
- } else {
- resolvedSolrUrl = CLIUtils.getDefaultSolrUrl();
- CLIO.err(
- "Neither --zk-host or --solr-url parameters, nor ZK_HOST env var
provided, so assuming solr url is "
- + resolvedSolrUrl
- + ".");
- }
Review Comment:
impact of this PR can be sensed by this refactoring, so many lines of code
being getting replaced with a shared utility function, kudos 🚀
[pending-on-me] to do local testing of this particular tool between main
branch & this branch.
##########
solr/core/src/java/org/apache/solr/cli/PackageTool.java:
##########
@@ -245,10 +244,11 @@ void runWithManagers(
Configurator.setRootLevel(Level.OFF);
try {
- String solrUrl = ConnectionOptions.resolveSolrUrl(opts, credentials);
- String zkHost = ConnectionOptions.resolveZkHost(opts, solrUrl,
credentials);
-
- if (zkHost == null) {
+ String solrUrl = opts.resolveSolrUrl(credentials);
Review Comment:
[nit] if possible, can you rename variable name opts to connectionOptions
for consistent variable names?
##########
solr/core/src/test/org/apache/solr/cli/CliDefaultValueProviderTest.java:
##########
@@ -22,29 +22,102 @@
public class CliDefaultValueProviderTest extends SolrTestCase {
+ @CommandLine.Command(name = "shared")
Review Comment:
[pending-on-me] to review these tests.
##########
solr/core/src/java/org/apache/solr/cli/CliDefaultValueProvider.java:
##########
@@ -16,21 +16,36 @@
*/
package org.apache.solr.cli;
+import java.lang.reflect.Field;
import org.apache.solr.common.util.EnvUtils;
import picocli.CommandLine;
-/** Provides default values for CLI arguments. */
+/**
+ * Fills picocli options from the environment (solr.in.sh variables, system
properties) when they
+ * are not given on the command line, keyed by the option's long name.
+ */
public class CliDefaultValueProvider implements
CommandLine.IDefaultValueProvider {
+
@Override
public String defaultValue(CommandLine.Model.ArgSpec argSpec) throws
Exception {
- return switch (argSpec.paramLabel()) {
- case "<zkHost>" -> EnvUtils.getProperty("zkHost");
- case "<solrConnection>" -> EnvUtils.getProperty("solr.connection");
- case "<solrUrl>" -> EnvUtils.getProperty("solr.url");
Review Comment:
[Q] purely for my knowledge, can you help me with example commands on how
--solr-connection & --solr-url differs? & how they both serve different purpose
for us.
Pls take in consideration of any of the package tool subcommand, so that i
am able to corelate better.
##########
solr/core/src/java/org/apache/solr/cli/PackageTargetOptions.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cli;
+
+import picocli.CommandLine;
+
+/** Where a package (un)deploy applies: cluster-level plugins or the given
collections. */
+public class PackageTargetOptions {
+ @CommandLine.Option(
+ names = {"--cluster"},
+ description = "Specifies that this action should affect cluster-level
plugins only.")
+ public boolean cluster;
+
+ @CommandLine.Option(
+ names = {"--collections"},
+ paramLabel = "COLLECTIONS",
+ description =
+ "Specifies that this action should affect plugins for the given
collections only, excluding cluster level plugins.")
+ public String collections;
+
+ /** The error to print when neither target was given, or null if one was. */
+ String missingTargetMessage(String verb) {
Review Comment:
do we write unit tests for this kind of utility function?
##########
solr/core/src/java/org/apache/solr/cli/PackageDeploy.java:
##########
@@ -46,17 +46,7 @@ public class PackageDeploy extends PackageSubCommand {
description = "Package name, optionally with :version.")
private String packageNameAndVersion;
- @picocli.CommandLine.Option(
- names = {"--cluster"},
- description = "Specifies that this action should affect cluster-level
plugins only.")
- private boolean cluster;
-
- @picocli.CommandLine.Option(
- names = {"--collections"},
- paramLabel = "COLLECTIONS",
- description =
- "Specifies that this action should affect plugins for the given
collections only, excluding cluster level plugins.")
- private String collections;
+ @picocli.CommandLine.Mixin private PackageTargetOptions target;
Review Comment:
lets name this variable as targetOptions?
this will help us keep consistent code wrt ConnectionOptions,
credentialOptions etc which we have till now.
##########
solr/core/src/java/org/apache/solr/cli/StatusTool.java:
##########
@@ -72,37 +72,17 @@ public class StatusTool extends ToolBase {
description = "Wait up to the specified number of seconds to see Solr
running.")
private Integer maxWaitSecs;
- @picocli.CommandLine.ArgGroup(exclusive = true, multiplicity = "0..1")
- private TargetOptions targetOptions;
-
- static class TargetOptions {
- @picocli.CommandLine.Option(
- names = {"-p", "--port"},
- description = "Port on localhost to check status for")
- private Integer port;
-
- @picocli.CommandLine.Option(
- names = {"-s", "--solr-connection"},
- description =
- "Zookeeper or HTTP(s) connection string; unnecessary if
SOLR_CONNECTION is defined in solr.in.sh; otherwise, defaults to "
- + CommonCLIOptions.DefaultValues.ZK_HOST
- + '.')
- private String solrConnection;
-
- @picocli.CommandLine.Option(
- names = {"--solr-url"},
- description =
- "Base Solr URL, which can be used to determine the zk-host if
that's not known")
- private String solrUrl;
-
- @picocli.CommandLine.Option(
- names = {"-z", "--zk-host"},
- description =
- "Zookeeper connection string; unnecessary if ZK_HOST is defined in
solr.in.sh; otherwise, defaults to "
- + CommonCLIOptions.DefaultValues.ZK_HOST
- + '.')
- private String zkHost;
- }
+ @picocli.CommandLine.Option(
+ names = {"-p", "--port"},
+ paramLabel = "PORT",
+ description = "Port on localhost to check status for")
+ private Integer portOpt;
Review Comment:
[Q] any specific reason, why we don't keep port under ConnectionOptions?
##########
solr/core/src/test/org/apache/solr/cli/ConnectionOptionsTest.java:
##########
@@ -16,31 +16,156 @@
*/
package org.apache.solr.cli;
-import org.apache.solr.cloud.SolrCloudTestCase;
-import org.junit.BeforeClass;
+import java.io.IOException;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.util.List;
+import java.util.concurrent.Callable;
+import org.apache.solr.SolrTestCase;
import org.junit.Test;
+import picocli.CommandLine;
-public class ConnectionOptionsTest extends SolrCloudTestCase {
+/** Parsing-level tests of {@link ConnectionOptions}; nothing here contacts
Solr or ZooKeeper. */
+public class ConnectionOptionsTest extends SolrTestCase {
Review Comment:
[pending-on-me] to review these tests.
##########
solr/core/src/java/org/apache/solr/cli/ConnectionOptions.java:
##########
@@ -17,141 +17,259 @@
package org.apache.solr.cli;
import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Locale;
import java.util.Map;
import org.apache.solr.client.solrj.SolrClient;
import org.apache.solr.client.solrj.impl.CloudSolrClient;
-import org.apache.solr.common.util.EnvUtils;
import picocli.CommandLine;
/**
- * Picocli ArgGroup for mutually-exclusive Solr URL / ZooKeeper connection
options.
+ * The {@code --solr-connection}, {@code --zk-host} and {@code --solr-url}
options shared by every
+ * picocli command that talks to Solr; add it to a command with {@code
@CommandLine.Mixin}.
*
- * <p>Use as the type of an {@code @ArgGroup(exclusive = true, multiplicity =
"0..1")} field to
- * ensure the user provides at most one of {@code --solr-connection}, {@code
--solr-url} or {@code
- * --zk-host}.
+ * <p>Options not given on the command line are filled from {@code
SOLR_CONNECTION}, {@code ZK_HOST}
+ * and {@code SOLR_URL} by {@link CliDefaultValueProvider}. The resolvers
prefer an option passed
+ * explicitly over those environment values, and reject more than one
explicitly passed option, so
+ * {@code ZK_HOST=... bin/solr create --solr-url ...} targets the URL while
{@code --zk-host a
+ * --solr-url b} is an error.
*/
-class ConnectionOptions {
+public class ConnectionOptions {
Review Comment:
[pending-on-me] to review the changes inside.
--
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]
