Copilot commented on code in PR #4269:
URL: https://github.com/apache/solr/pull/4269#discussion_r3044558520
##########
solr/packaging/test/test_create.bats:
##########
@@ -42,5 +42,5 @@ teardown() {
@test "multiple connection options are prevented" {
run solr create -c COLL_NAME2 --solr-url http://localhost:${SOLR_PORT} -z
localhost:${ZK_PORT}
- assert_output --partial "The option 'z' was specified but an option from
this group has already been selected: 's'"
+ assert_output --regexp "(mutually exclusive|already been selected)"
Review Comment:
The new regexp `(mutually exclusive|already been selected)` is broad enough
that it could match unrelated validation errors, making this test less
reliable. Tighten it to also mention the specific conflicting options
(`--solr-url`/`--zk-host` or `-s`/`-z`) so the test asserts the intended
exclusivity behavior.
```suggestion
assert_output --regexp
"(--solr-url.*(-z|--zk-host)|(-z|--zk-host).*--solr-url).*(mutually
exclusive|already been selected)|(mutually exclusive|already been
selected).*(--solr-url.*(-z|--zk-host)|(-z|--zk-host).*--solr-url)"
```
##########
solr/core/src/java/org/apache/solr/cli/CreateTool.java:
##########
@@ -123,45 +183,45 @@ public Options getOptions() {
@Override
public void runImpl(CommandLine cli) throws Exception {
try (var solrClient = CLIUtils.getSolrClient(cli)) {
+ CreateParams params =
+ new CreateParams(
+ cli.getOptionValue(COLLECTION_NAME_OPTION),
+ cli.getOptionValue(CONF_DIR_OPTION,
DefaultValues.DEFAULT_CONFIG_SET),
+ cli.getOptionValue(CONF_NAME_OPTION),
+ cli.getOptionValue(CommonCLIOptions.SOLR_URL_OPTION,
CLIUtils.getDefaultSolrUrl()),
+ cli.getOptionValue(CommonCLIOptions.CREDENTIALS_OPTION),
+ cli.getParsedOptionValue(SHARDS_OPTION, 1),
+ cli.getParsedOptionValue(REPLICATION_FACTOR_OPTION, 1));
if (CLIUtils.isCloudMode(solrClient)) {
- createCollection(cli);
+ createCollection(CLIUtils.getZkHost(cli), params);
} else {
- createCore(cli, solrClient);
+ createCore(params, solrClient);
}
}
}
- protected void createCore(CommandLine cli, SolrClient solrClient) throws
Exception {
- String coreName = cli.getOptionValue(COLLECTION_NAME_OPTION);
- String solrUrl =
- cli.getOptionValue(CommonCLIOptions.SOLR_URL_OPTION,
CLIUtils.getDefaultSolrUrl());
-
- final String solrInstallDir = System.getProperty("solr.install.dir");
- final String confDirName =
- cli.getOptionValue(CONF_DIR_OPTION, DefaultValues.DEFAULT_CONFIG_SET);
-
+ private void createCore(CreateParams params, SolrClient solrClient) throws
Exception {
// we allow them to pass a directory instead of a configset name
- Path configsetDir = Path.of(confDirName);
- Path solrInstallDirPath = Path.of(solrInstallDir);
+ Path configsetDir = Path.of(params.confDir);
+ Path solrInstallDirPath = Path.of(System.getProperty("solr.install.dir"));
Review Comment:
`CreateParams` is declared as a Java `record`, but the code is accessing
components as fields (e.g., `params.confDir`, `params.name`). Record components
are accessed via accessor methods (`params.confDir()`, `params.name()`, etc.);
the current code will not compile. Update all `params.*` usages accordingly (or
convert `CreateParams` to a regular class).
##########
solr/core/src/java/org/apache/solr/cli/DeleteTool.java:
##########
@@ -93,31 +131,34 @@ public Options getOptions() {
@Override
public void runImpl(CommandLine cli) throws Exception {
try (var solrClient = CLIUtils.getSolrClient(cli)) {
+ DeleteParams params =
+ new DeleteParams(
+ cli.getOptionValue(COLLECTION_NAME_OPTION),
+ cli.getOptionValue(CommonCLIOptions.CREDENTIALS_OPTION),
+ cli.hasOption(DELETE_CONFIG_OPTION),
+ cli.hasOption(FORCE_OPTION));
if (CLIUtils.isCloudMode(solrClient)) {
- deleteCollection(cli);
+ deleteCollection(CLIUtils.getZkHost(cli), params);
} else {
- deleteCore(cli, solrClient);
+ deleteCore(params, solrClient);
}
}
}
- protected void deleteCollection(CommandLine cli) throws Exception {
+ private void deleteCollection(String zkHost, DeleteParams params) throws
Exception {
var builder =
new HttpJettySolrClient.Builder()
.withIdleTimeout(30, TimeUnit.SECONDS)
.withConnectionTimeout(15, TimeUnit.SECONDS)
.withKeyStoreReloadInterval(-1, TimeUnit.SECONDS)
- .withOptionalBasicAuthCredentials(
- cli.getOptionValue(CommonCLIOptions.CREDENTIALS_OPTION));
-
- String zkHost = CLIUtils.getZkHost(cli);
+ .withOptionalBasicAuthCredentials(params.credentials);
Review Comment:
`DeleteParams` is a Java `record`, but this code accesses record components
as fields (`params.credentials`, `params.name`, etc.). This won’t compile; use
the generated accessor methods (`params.credentials()`, `params.name()`, ...),
or change `DeleteParams` to a regular class.
```suggestion
.withOptionalBasicAuthCredentials(params.credentials());
```
##########
solr/core/src/java/org/apache/solr/cli/DeleteTool.java:
##########
@@ -197,28 +236,61 @@ protected void deleteCollection(CloudSolrClient
cloudSolrClient, CommandLine cli
}
}
- echo(String.format(Locale.ROOT, "\nDeleted collection '%s'",
collectionName));
+ echo(String.format(Locale.ROOT, "\nDeleted collection '%s'", params.name));
}
- protected void deleteCore(CommandLine cli, SolrClient solrClient) throws
Exception {
- String coreName = cli.getOptionValue(COLLECTION_NAME_OPTION);
-
- echo("\nDeleting core '" + coreName + "' using V2 Cores API\n");
+ private void deleteCore(DeleteParams params, SolrClient solrClient) throws
Exception {
+ echo("\nDeleting core '" + params.name + "' using V2 Cores API\n");
try {
- var req = new CoresApi.UnloadCore(coreName);
+ var req = new CoresApi.UnloadCore(params.name);
req.setDeleteIndex(true);
req.setDeleteDataDir(true);
req.setDeleteInstanceDir(true);
var response = req.process(solrClient);
echoIfVerbose(response);
} catch (SolrServerException sse) {
- throw new Exception("Failed to delete core '" + coreName + "' due to: "
+ sse.getMessage());
+ throw new Exception(
+ "Failed to delete core '" + params.name + "' due to: " +
sse.getMessage());
}
}
@Override
public int callTool() throws Exception {
- throw new UnsupportedOperationException("This tool does not yet support
PicoCli");
+ String zkHostArg = (connectionOptions != null) ? connectionOptions.zkHost
: null;
+ String solrUrlArg = (connectionOptions != null) ?
connectionOptions.solrUrl : null;
+ DeleteParams params =
+ new DeleteParams(name, credentialsOptions.credentials, deleteConfig,
force);
+
+ if (zkHostArg != null) {
+ deleteCollection(zkHostArg, params);
+ } else {
+ String resolvedSolrUrl;
+ if (solrUrlArg != null) {
+ resolvedSolrUrl = CLIUtils.normalizeSolrUrl(solrUrlArg);
+ } 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:
Same as `create`: if `connectionOptions` is null, the picocli path always
falls back to the default Solr URL and does not consider `ZK_HOST` / `zkHost`
as a way to target SolrCloud. This is a behavior change vs the commons-cli path
and can cause deletions to run against the wrong instance or fail to locate the
intended cluster.
##########
solr/core/src/java/org/apache/solr/cli/CreateTool.java:
##########
@@ -355,6 +412,56 @@ private void
printDefaultConfigsetWarningIfNecessary(CommandLine cli) {
@Override
public int callTool() throws Exception {
- throw new UnsupportedOperationException("This tool does not yet support
PicoCli");
+ String zkHostArg = (connectionOptions != null) ? connectionOptions.zkHost
: null;
+ String solrUrlArg = (connectionOptions != null) ?
connectionOptions.solrUrl : null;
+
+ if (zkHostArg != null) {
+ CreateParams params =
+ new CreateParams(
+ name,
+ confDir,
+ confName,
+ null,
+ credentialsOptions.credentials,
+ shards,
+ replicationFactor);
+ createCollection(zkHostArg, params);
+ } else {
+ String resolvedSolrUrl;
+ if (solrUrlArg != null) {
+ resolvedSolrUrl = CLIUtils.normalizeSolrUrl(solrUrlArg);
+ } 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:
In the picocli path, when neither `--solr-url` nor `--zk-host` is specified,
this code always assumes the default Solr URL and never consults the `ZK_HOST`
env var / `zkHost` system property. That differs from the existing commons-cli
behavior (`CLIUtils.normalizeSolrUrl`/`getZkHost`) and can break users who rely
on `ZK_HOST` to target a remote cluster. Consider reusing the existing
resolution logic (e.g., `ZkConnectionOptions.resolveZkHost()` or the
`CLIUtils.getCliOptionOrPropValue`/`getZkHost` precedence) so picocli and
commons-cli behave the same.
--
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]
