gerlowskija commented on code in PR #1808:
URL: https://github.com/apache/solr/pull/1808#discussion_r1286378310
##########
solr/core/src/java/org/apache/solr/cli/SolrCLI.java:
##########
@@ -474,6 +509,8 @@ public static String resolveSolrUrl(CommandLine cli) throws
Exception {
solrUrl =
ZkStateReader.from(cloudSolrClient).getBaseUrlForNodeName(firstLiveNode);
}
}
+ } else {
+ solrUrl = resolveSolrUrl(solrUrl);
Review Comment:
[-1] I think the solr URL will miss normalization here if the user provided
a zkHost, unless I'm missing something?
##########
solr/core/src/java/org/apache/solr/cli/SolrCLI.java:
##########
@@ -444,8 +453,34 @@ private static void printHelp() {
print("Pass -help or -h after any COMMAND to see command-specific usage
information,");
print("such as: ./solr start -help or ./solr stop -h");
}
+
/**
- * Get the base URL of a live Solr instance from either the solrUrl
command-line option from
+ * Strips off the end of solrUrl any /solr when a legacy solrUrl like
http://localhost:8983/solr
+ * is used, and warns those users. In the future we'll have url's with /api
as well.
+ *
+ * @param solrUrl with /solr stripped off.
Review Comment:
[Q] Are these Javadocs right? It looks like the main point of this method
is to do the "/solr"-stripping, but this Javadoc makes it sound like the
`solrUrl` param is supposed to already be stripped?
##########
solr/core/src/test/org/apache/solr/cli/SolrCLITest.java:
##########
@@ -19,7 +19,14 @@
import org.apache.solr.SolrTestCase;
import org.junit.Test;
-public class SolrCliUptimeTest extends SolrTestCase {
+public class SolrCLITest extends SolrTestCase {
+ @Test
+ public void testResolveSolrUrl() {
+ assertEquals(SolrCLI.resolveSolrUrl("http://localhost:8983/solr";),
"http://localhost:8983";);
+ assertEquals(SolrCLI.resolveSolrUrl("http://localhost:8983";),
"http://localhost:8983";);
+ assertEquals(SolrCLI.resolveSolrUrl("http://localhost:8983/";),
"http://localhost:8983";);
Review Comment:
[0] Seeing these three cases makes me a little twitchy without
`http://localhost:8983/solr/` to complete the set 😛
##########
solr/core/src/java/org/apache/solr/cli/SolrCLI.java:
##########
@@ -444,8 +453,34 @@ private static void printHelp() {
print("Pass -help or -h after any COMMAND to see command-specific usage
information,");
print("such as: ./solr start -help or ./solr stop -h");
}
+
/**
- * Get the base URL of a live Solr instance from either the solrUrl
command-line option from
+ * Strips off the end of solrUrl any /solr when a legacy solrUrl like
http://localhost:8983/solr
+ * is used, and warns those users. In the future we'll have url's with /api
as well.
+ *
+ * @param solrUrl with /solr stripped off.
+ * @return the truncated if need solrUrl.
+ */
+ public static String resolveSolrUrl(String solrUrl) {
+ if (solrUrl != null) {
+ if (solrUrl.indexOf("/solr") > -1) { //
+ String newSolrUrl = solrUrl.substring(0, solrUrl.indexOf("/solr"));
+ CLIO.out(
+ "'solrUrl' needn't include Solr's context-root (e.g. \"/solr\");
correcting from ["
Review Comment:
[-0] It's good we're telling users how we massage their input (so they know
the "correct" value), but IMO we should put more stress on the "and we're gonna
stop doing this for you starting in X.Y" half of the message. What do you
think of something like:
> WARNING: URLs provided to this tool needn't include Solr's context-root
(e.g. "/solr"). Such URLs are deprecated and support for them will be removed
in a future release. Correcting from ...
##########
solr/core/src/test/org/apache/solr/cli/SolrCLITest.java:
##########
@@ -19,7 +19,14 @@
import org.apache.solr.SolrTestCase;
import org.junit.Test;
-public class SolrCliUptimeTest extends SolrTestCase {
+public class SolrCLITest extends SolrTestCase {
+ @Test
+ public void testResolveSolrUrl() {
+ assertEquals(SolrCLI.resolveSolrUrl("http://localhost:8983/solr";),
"http://localhost:8983";);
+ assertEquals(SolrCLI.resolveSolrUrl("http://localhost:8983";),
"http://localhost:8983";);
+ assertEquals(SolrCLI.resolveSolrUrl("http://localhost:8983/";),
"http://localhost:8983";);
Review Comment:
[0] Seeing these three cases makes me a little twitchy without
`http://localhost:8983/solr/` to complete the set 😛
##########
solr/core/src/java/org/apache/solr/cli/SolrCLI.java:
##########
@@ -444,8 +453,34 @@ private static void printHelp() {
print("Pass -help or -h after any COMMAND to see command-specific usage
information,");
print("such as: ./solr start -help or ./solr stop -h");
}
+
/**
- * Get the base URL of a live Solr instance from either the solrUrl
command-line option from
+ * Strips off the end of solrUrl any /solr when a legacy solrUrl like
http://localhost:8983/solr
+ * is used, and warns those users. In the future we'll have url's with /api
as well.
+ *
+ * @param solrUrl with /solr stripped off.
+ * @return the truncated if need solrUrl.
+ */
+ public static String resolveSolrUrl(String solrUrl) {
Review Comment:
[0] Not sure about "resolve" here naming-wise as its somewhat vague. Maybe
"normalize"?
--
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
