Re: [PR] SOLR-17501: Move out CLI utils from SolrCLI [solr]
malliaridis merged PR #2744: URL: https://github.com/apache/solr/pull/2744 -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17501: Move out CLI utils from SolrCLI [solr]
malliaridis commented on code in PR #2744:
URL: https://github.com/apache/solr/pull/2744#discussion_r1838018198
##
solr/core/src/java/org/apache/solr/cli/CLIUtils.java:
##
@@ -0,0 +1,357 @@
+/*
+ * 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 static org.apache.solr.common.SolrException.ErrorCode.FORBIDDEN;
+import static org.apache.solr.common.SolrException.ErrorCode.UNAUTHORIZED;
+import static org.apache.solr.common.params.CommonParams.NAME;
+import static org.apache.solr.common.params.CommonParams.SYSTEM_INFO_PATH;
+
+import java.io.IOException;
+import java.net.SocketException;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import org.apache.commons.cli.CommandLine;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.SolrRequest;
+import org.apache.solr.client.solrj.SolrServerException;
+import org.apache.solr.client.solrj.impl.CloudHttp2SolrClient;
+import org.apache.solr.client.solrj.impl.CloudSolrClient;
+import org.apache.solr.client.solrj.impl.Http2SolrClient;
+import org.apache.solr.client.solrj.impl.SolrZkClientTimeout;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.CoreAdminRequest;
+import org.apache.solr.client.solrj.request.GenericSolrRequest;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.SolrZkClient;
+import org.apache.solr.common.cloud.ZkStateReader;
+import org.apache.solr.common.params.CommonParams;
+import org.apache.solr.common.util.EnvUtils;
+import org.apache.solr.common.util.NamedList;
+
+/**
+ * Utility class that holds various helper methods for the CLI.
+ *
+ * @since 10.0
+ */
+public final class CLIUtils {
+
+ private CLIUtils() {}
+
+ public static String RED = "\u001B[31m";
+
+ public static String GREEN = "\u001B[32m";
+
+ public static String YELLOW = "\u001B[33m";
+
+ private static final long MAX_WAIT_FOR_CORE_LOAD_NANOS =
+ TimeUnit.NANOSECONDS.convert(1, TimeUnit.MINUTES);
+
+ public static String getDefaultSolrUrl() {
+// note that ENV_VAR syntax (and the env vars too) are mapped to env.var
sys props
+String scheme = EnvUtils.getProperty("solr.url.scheme", "http");
+String host = EnvUtils.getProperty("solr.tool.host", "localhost");
+String port = EnvUtils.getProperty("jetty.port", "8983"); // from
SOLR_PORT env
+return String.format(Locale.ROOT, "%s://%s:%s",
scheme.toLowerCase(Locale.ROOT), host, port);
+ }
+
+ /**
+ * Determine if a request to Solr failed due to a communication error, which
is generally
+ * retry-able.
+ */
+ public static boolean checkCommunicationError(Exception exc) {
+Throwable rootCause = SolrException.getRootCause(exc);
+return (rootCause instanceof SolrServerException || rootCause instanceof
SocketException);
+ }
+
+ public static void checkCodeForAuthError(int code) {
+if (code == UNAUTHORIZED.code || code == FORBIDDEN.code) {
+ throw new SolrException(
+ SolrException.ErrorCode.getErrorCode(code),
+ "Solr requires authentication for request. Please supply valid
credentials. HTTP code="
+ + code);
+}
+ }
+
+ public static boolean exceptionIsAuthRelated(Exception exc) {
+return (exc instanceof SolrException
+&& Arrays.asList(UNAUTHORIZED.code,
FORBIDDEN.code).contains(((SolrException) exc).code()));
+ }
+
+ public static SolrClient getSolrClient(String solrUrl, String credentials,
boolean barePath) {
+// today we require all urls to end in /solr, however in the future we
will need to support the
+// /api url end point instead. Eventually we want to have this method
always
+// return a bare url, and then individual calls decide if they are /solr
or /api
+// The /solr/ check is because sometimes a full url is passed in, like
+// http://localhost:8983/solr/films_shard1_repli
Re: [PR] SOLR-17501: Move out CLI utils from SolrCLI [solr]
malliaridis commented on code in PR #2744:
URL: https://github.com/apache/solr/pull/2744#discussion_r1838016144
##
solr/core/src/java/org/apache/solr/cli/CLIUtils.java:
##
@@ -0,0 +1,357 @@
+/*
+ * 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 static org.apache.solr.common.SolrException.ErrorCode.FORBIDDEN;
+import static org.apache.solr.common.SolrException.ErrorCode.UNAUTHORIZED;
+import static org.apache.solr.common.params.CommonParams.NAME;
+import static org.apache.solr.common.params.CommonParams.SYSTEM_INFO_PATH;
+
+import java.io.IOException;
+import java.net.SocketException;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import org.apache.commons.cli.CommandLine;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.SolrRequest;
+import org.apache.solr.client.solrj.SolrServerException;
+import org.apache.solr.client.solrj.impl.CloudHttp2SolrClient;
+import org.apache.solr.client.solrj.impl.CloudSolrClient;
+import org.apache.solr.client.solrj.impl.Http2SolrClient;
+import org.apache.solr.client.solrj.impl.SolrZkClientTimeout;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.CoreAdminRequest;
+import org.apache.solr.client.solrj.request.GenericSolrRequest;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.SolrZkClient;
+import org.apache.solr.common.cloud.ZkStateReader;
+import org.apache.solr.common.params.CommonParams;
+import org.apache.solr.common.util.EnvUtils;
+import org.apache.solr.common.util.NamedList;
+
+/**
+ * Utility class that holds various helper methods for the CLI.
+ *
+ * @since 10.0
+ */
+public final class CLIUtils {
+
+ private CLIUtils() {}
+
+ public static String RED = "\u001B[31m";
+
+ public static String GREEN = "\u001B[32m";
+
+ public static String YELLOW = "\u001B[33m";
+
+ private static final long MAX_WAIT_FOR_CORE_LOAD_NANOS =
+ TimeUnit.NANOSECONDS.convert(1, TimeUnit.MINUTES);
+
+ public static String getDefaultSolrUrl() {
+// note that ENV_VAR syntax (and the env vars too) are mapped to env.var
sys props
+String scheme = EnvUtils.getProperty("solr.url.scheme", "http");
+String host = EnvUtils.getProperty("solr.tool.host", "localhost");
+String port = EnvUtils.getProperty("jetty.port", "8983"); // from
SOLR_PORT env
+return String.format(Locale.ROOT, "%s://%s:%s",
scheme.toLowerCase(Locale.ROOT), host, port);
+ }
+
+ /**
+ * Determine if a request to Solr failed due to a communication error, which
is generally
+ * retry-able.
+ */
+ public static boolean checkCommunicationError(Exception exc) {
+Throwable rootCause = SolrException.getRootCause(exc);
+return (rootCause instanceof SolrServerException || rootCause instanceof
SocketException);
+ }
+
+ public static void checkCodeForAuthError(int code) {
+if (code == UNAUTHORIZED.code || code == FORBIDDEN.code) {
+ throw new SolrException(
+ SolrException.ErrorCode.getErrorCode(code),
+ "Solr requires authentication for request. Please supply valid
credentials. HTTP code="
+ + code);
+}
+ }
+
+ public static boolean exceptionIsAuthRelated(Exception exc) {
+return (exc instanceof SolrException
+&& Arrays.asList(UNAUTHORIZED.code,
FORBIDDEN.code).contains(((SolrException) exc).code()));
+ }
+
+ public static SolrClient getSolrClient(String solrUrl, String credentials,
boolean barePath) {
+// today we require all urls to end in /solr, however in the future we
will need to support the
+// /api url end point instead. Eventually we want to have this method
always
+// return a bare url, and then individual calls decide if they are /solr
or /api
+// The /solr/ check is because sometimes a full url is passed in, like
+// http://localhost:8983/solr/films_shard1_repli
Re: [PR] SOLR-17501: Move out CLI utils from SolrCLI [solr]
malliaridis commented on code in PR #2744:
URL: https://github.com/apache/solr/pull/2744#discussion_r1837998002
##
solr/core/src/java/org/apache/solr/cli/CLIUtils.java:
##
@@ -0,0 +1,357 @@
+/*
+ * 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 static org.apache.solr.common.SolrException.ErrorCode.FORBIDDEN;
+import static org.apache.solr.common.SolrException.ErrorCode.UNAUTHORIZED;
+import static org.apache.solr.common.params.CommonParams.NAME;
+import static org.apache.solr.common.params.CommonParams.SYSTEM_INFO_PATH;
+
+import java.io.IOException;
+import java.net.SocketException;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import org.apache.commons.cli.CommandLine;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.SolrRequest;
+import org.apache.solr.client.solrj.SolrServerException;
+import org.apache.solr.client.solrj.impl.CloudHttp2SolrClient;
+import org.apache.solr.client.solrj.impl.CloudSolrClient;
+import org.apache.solr.client.solrj.impl.Http2SolrClient;
+import org.apache.solr.client.solrj.impl.SolrZkClientTimeout;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.CoreAdminRequest;
+import org.apache.solr.client.solrj.request.GenericSolrRequest;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.SolrZkClient;
+import org.apache.solr.common.cloud.ZkStateReader;
+import org.apache.solr.common.params.CommonParams;
+import org.apache.solr.common.util.EnvUtils;
+import org.apache.solr.common.util.NamedList;
+
+/**
+ * Utility class that holds various helper methods for the CLI.
+ *
+ * @since 10.0
Review Comment:
I wasn't sure if this is a practice followed in this project and if this has
any use case right now. I saw it in a few other places, but it is definitely
not used widely. It does also make backporting harder, so I believe it is not
recommended. Will remove it. :)
##
solr/core/src/java/org/apache/solr/cli/ApiTool.java:
##
@@ -79,7 +79,7 @@ protected String callGet(String url, String credentials)
throws Exception {
URI uri = new URI(url.replace("+", "%20"));
String solrUrl = getSolrUrlFromUri(uri);
String path = uri.getPath();
-try (var solrClient = SolrCLI.getSolrClient(solrUrl, credentials)) {
+try (var solrClient = CLIUtils.getSolrClient(solrUrl, credentials)) {
Review Comment:
I feel that we are slowly getting there with all the migrations and
cleanups. :)
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17501: Move out CLI utils from SolrCLI [solr]
epugh commented on code in PR #2744:
URL: https://github.com/apache/solr/pull/2744#discussion_r1836934124
##
solr/core/src/java/org/apache/solr/cli/CLIUtils.java:
##
@@ -0,0 +1,357 @@
+/*
+ * 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 static org.apache.solr.common.SolrException.ErrorCode.FORBIDDEN;
+import static org.apache.solr.common.SolrException.ErrorCode.UNAUTHORIZED;
+import static org.apache.solr.common.params.CommonParams.NAME;
+import static org.apache.solr.common.params.CommonParams.SYSTEM_INFO_PATH;
+
+import java.io.IOException;
+import java.net.SocketException;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import org.apache.commons.cli.CommandLine;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.SolrRequest;
+import org.apache.solr.client.solrj.SolrServerException;
+import org.apache.solr.client.solrj.impl.CloudHttp2SolrClient;
+import org.apache.solr.client.solrj.impl.CloudSolrClient;
+import org.apache.solr.client.solrj.impl.Http2SolrClient;
+import org.apache.solr.client.solrj.impl.SolrZkClientTimeout;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.CoreAdminRequest;
+import org.apache.solr.client.solrj.request.GenericSolrRequest;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.SolrZkClient;
+import org.apache.solr.common.cloud.ZkStateReader;
+import org.apache.solr.common.params.CommonParams;
+import org.apache.solr.common.util.EnvUtils;
+import org.apache.solr.common.util.NamedList;
+
+/**
+ * Utility class that holds various helper methods for the CLI.
+ *
+ * @since 10.0
Review Comment:
I don't actually think we use @since annotations...
##
solr/core/src/java/org/apache/solr/cli/ApiTool.java:
##
@@ -79,7 +79,7 @@ protected String callGet(String url, String credentials)
throws Exception {
URI uri = new URI(url.replace("+", "%20"));
String solrUrl = getSolrUrlFromUri(uri);
String path = uri.getPath();
-try (var solrClient = SolrCLI.getSolrClient(solrUrl, credentials)) {
+try (var solrClient = CLIUtils.getSolrClient(solrUrl, credentials)) {
Review Comment:
In some ways, I am surprised that we don't have a existing "factory" style
method like this to get a solr client based on solrUrl and some credentials ;-)
##
solr/core/src/java/org/apache/solr/cli/CLIUtils.java:
##
@@ -0,0 +1,357 @@
+/*
+ * 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 static org.apache.solr.common.SolrException.ErrorCode.FORBIDDEN;
+import static org.apache.solr.common.SolrException.ErrorCode.UNAUTHORIZED;
+import static org.apache.solr.common.params.CommonParams.NAME;
+import static org.apache.solr.common.params.CommonParams.SYSTEM_INFO_PATH;
+
+import java.io.IOException;
+import java.net.SocketException;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Loca
Re: [PR] SOLR-17501: Move out CLI utils from SolrCLI [solr]
malliaridis commented on PR #2744: URL: https://github.com/apache/solr/pull/2744#issuecomment-2467885311 When I added the tests I noticed a few cases where we may require to patch things. To be precise: - URI is very lenient and allows many invalid strings without throwing URISyntaxException. Using URL in `portFromUrl` (that is used currently only by `StatusTool`) may be a good improvement - `checkCommunicationError` is `false` if a `SolrServerException` is thrown with a different root cause (e.g. NPE). I tried to avoid any changes unrelated to the ticket and we still have many untested methods in this part. -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17501: Move out CLI utils from SolrCLI [solr]
malliaridis commented on PR #2744: URL: https://github.com/apache/solr/pull/2744#issuecomment-2460051147 I'll also would wait for https://github.com/apache/solr/pull/2725 to be merged first before completing thins one. -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17501: Move out CLI utils from SolrCLI [solr]
malliaridis commented on PR #2744: URL: https://github.com/apache/solr/pull/2744#issuecomment-2422403472 This PR is related and should be blocked by #2778 for now. -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17501: Move out CLI utils from SolrCLI [solr]
malliaridis commented on PR #2744: URL: https://github.com/apache/solr/pull/2744#issuecomment-2422402462 > Is this able to backwards ported to 9x? Seems good stuff! I think I could create a separate PR for backporting if we want these changes in 9x as well. I think a cherry-pick won't do in this case as I was expecting. -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17501: Move out CLI utils from SolrCLI [solr]
epugh commented on PR #2744: URL: https://github.com/apache/solr/pull/2744#issuecomment-2420464388 Is this able to backwards ported to 9x? Seems good stuff! -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
