You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "epugh (via GitHub)" <gi...@apache.org> on 2023/07/25 22:15:37 UTC
[GitHub] [solr] epugh opened a new pull request, #1808: SOLR-16800: Move towards using the root of Solr as the -solrUrl in the CLI.
epugh opened a new pull request, #1808:
URL: https://github.com/apache/solr/pull/1808
https://issues.apache.org/jira/browse/SOLR-16800
# Description
In the future we'll have V2 API urls that are under /api, so having urls be /solr is a bit awkward. Allow -solrUrl http://localhost:8983 to work, and warn on http://localhost:8983/solr.
Some other cleanups as well.
# Solution
Use the SolrCLI.getSolrClient method through out the client Tool's.
# Tests
bats tests
# Checklist
Please review the following and check all that apply:
- [ ] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [ ] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [ ] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1808: SOLR-16800: Move towards using the root of Solr as the -solrUrl in the CLI.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1808:
URL: https://github.com/apache/solr/pull/1808#discussion_r1286973313
##########
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:
Yeah, I redid it, took out some nesting to make it easier to read, and fix this...
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1808: SOLR-16800: Move towards using the root of Solr as the -solrUrl in the CLI.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1808:
URL: https://github.com/apache/solr/pull/1808#discussion_r1285218105
##########
solr/core/src/java/org/apache/solr/cli/SolrCLI.java:
##########
@@ -383,6 +383,11 @@ public static boolean exceptionIsAuthRelated(Exception exc) {
}
public static SolrClient getSolrClient(String solrUrl) {
+ // today we require all urls to end in /solr, however in the future we will need to support the
+ // /api url end point instead.
+ if (!solrUrl.endsWith(("/solr"))) {
Review Comment:
okay, I've pulled in `main` that has the now hardcoded hostContext...
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1808: SOLR-16800: Move towards using the root of Solr as the -solrUrl in the CLI.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1808:
URL: https://github.com/apache/solr/pull/1808#discussion_r1276630658
##########
solr/core/src/java/org/apache/solr/cli/SolrCLI.java:
##########
@@ -383,6 +383,11 @@ public static boolean exceptionIsAuthRelated(Exception exc) {
}
public static SolrClient getSolrClient(String solrUrl) {
+ // today we require all urls to end in /solr, however in the future we will need to support the
+ // /api url end point instead.
+ if (!solrUrl.endsWith(("/solr"))) {
Review Comment:
Oh wait... why do tests now have solr urls that end in `/solr`??? Now I wonder if they also stand up Solr servers that don't use `/api` for the v2 api endpoint?
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1808: SOLR-16800: Move towards using the root of Solr as the -solrUrl in the CLI.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1808:
URL: https://github.com/apache/solr/pull/1808#discussion_r1286961989
##########
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:
Happy to "de-twitch" you!
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1808: SOLR-16800: Move towards using the root of Solr as the -solrUrl in the CLI.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1808:
URL: https://github.com/apache/solr/pull/1808#discussion_r1286963177
##########
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:
doh!
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1808: SOLR-16800: Move towards using the root of Solr as the -solrUrl in the CLI.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1808:
URL: https://github.com/apache/solr/pull/1808#discussion_r1276281337
##########
solr/core/src/java/org/apache/solr/cli/SolrCLI.java:
##########
@@ -383,6 +383,11 @@ public static boolean exceptionIsAuthRelated(Exception exc) {
}
public static SolrClient getSolrClient(String solrUrl) {
+ // today we require all urls to end in /solr, however in the future we will need to support the
+ // /api url end point instead.
+ if (!solrUrl.endsWith(("/solr"))) {
Review Comment:
So this kind of made me sad... basically I was trying to write both code to handle legacy /solr type urls, but then, since everything TODAY requries a /solr url, then to inject it here. Have we figured out how in the future to hit a V2 api that has an /api end point instead via a SolrClient? My hope is once that is solved then we remove this logic.
Having said that, if there is better code to use here, would love some of that! I hope that before 10x gets released, this code will be eliminated, so I kind of just put this in "for now".... I don't expect to backport this to 9x.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh merged pull request #1808: SOLR-16800: Move towards using the root of Solr as the -solrUrl in the CLI.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh merged PR #1808:
URL: https://github.com/apache/solr/pull/1808
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] gerlowskija commented on a diff in pull request #1808: SOLR-16800: Move towards using the root of Solr as the -solrUrl in the CLI.
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #1808:
URL: https://github.com/apache/solr/pull/1808#discussion_r1274883703
##########
solr/core/src/java/org/apache/solr/cli/SolrCLI.java:
##########
@@ -445,8 +450,31 @@ 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");
}
+
+ /**
+ * 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) { //
+ solrUrl = solrUrl.substring(0, solrUrl.indexOf("/solr"));
+ CLIO.out(
+ "The solrUrl parameter only needs to only point to the root of Solr ("
Review Comment:
[-0] I think this message would read more clearly if you dropped the second "only".
[0] Maybe add some indication in this message that Solr is able to auto-correct the URL for folks, so they don't see the warning and think the command has failed. e.g.
> 'solrUrl' needn't include Solr's context-root (e.g. "/solr"); correcting from [old] to [new]
##########
solr/core/src/java/org/apache/solr/cli/SolrCLI.java:
##########
@@ -383,6 +383,11 @@ public static boolean exceptionIsAuthRelated(Exception exc) {
}
public static SolrClient getSolrClient(String solrUrl) {
+ // today we require all urls to end in /solr, however in the future we will need to support the
+ // /api url end point instead.
+ if (!solrUrl.endsWith(("/solr"))) {
Review Comment:
[0] Not sure whether this is intended or not, but if the provided URL was `http://localhost:8983/solr/` (not the ending '/') we'd still append`/solr`. I wonder if there isn't some Java utility for adding path segments to a URL that can be a little smarter about some of the edge cases (handling adjacent forward slashes, etc.)
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #1808: SOLR-16800: Move towards using the root of Solr as the -solrUrl in the CLI.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1808:
URL: https://github.com/apache/solr/pull/1808#issuecomment-1666953093
@chatman I've got some optimizations to the Package tool if you want to look. @gerlowskija I'd love another looksee now that hostContext has been sorted....
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #1808: SOLR-16800: Move towards using the root of Solr as the -solrUrl in the CLI.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1808:
URL: https://github.com/apache/solr/pull/1808#issuecomment-1658993106
This PR is held up on getting SOLR-16911, PR #1810 committed first....
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] gerlowskija commented on a diff in pull request #1808: SOLR-16800: Move towards using the root of Solr as the -solrUrl in the CLI.
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1808: SOLR-16800: Move towards using the root of Solr as the -solrUrl in the CLI.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1808:
URL: https://github.com/apache/solr/pull/1808#discussion_r1276296189
##########
solr/core/src/java/org/apache/solr/cli/SolrCLI.java:
##########
@@ -445,8 +450,31 @@ 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");
}
+
+ /**
+ * 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) { //
+ solrUrl = solrUrl.substring(0, solrUrl.indexOf("/solr"));
+ CLIO.out(
+ "The solrUrl parameter only needs to only point to the root of Solr ("
Review Comment:
Love it... pushing up fix.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1808: SOLR-16800: Move towards using the root of Solr as the -solrUrl in the CLI.
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1808:
URL: https://github.com/apache/solr/pull/1808#discussion_r1276584581
##########
solr/core/src/java/org/apache/solr/cli/SolrCLI.java:
##########
@@ -383,6 +383,11 @@ public static boolean exceptionIsAuthRelated(Exception exc) {
}
public static SolrClient getSolrClient(String solrUrl) {
+ // today we require all urls to end in /solr, however in the future we will need to support the
+ // /api url end point instead.
+ if (!solrUrl.endsWith(("/solr"))) {
Review Comment:
Ugh... One test has "http://127.0.0.1:51902/uf_" as the URL.....
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org