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/28 12:58:00 UTC
[GitHub] [solr] epugh opened a new pull request, #1810: SOLR-16911: Remove ability to change hostContext, keep it as /solr
epugh opened a new pull request, #1810:
URL: https://github.com/apache/solr/pull/1810
https://issues.apache.org/jira/browse/SOLR-16911
# Description
Discovered during work on on SolrCLI that we support different hostContext than `/solr`. Remove that.
# Solution
Please provide a short description of the approach taken to implement your solution.
# Tests
Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.
# 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] psalagnac commented on a diff in pull request #1810: SOLR-16911: Remove ability to change hostContext, keep it as /solr
Posted by "psalagnac (via GitHub)" <gi...@apache.org>.
psalagnac commented on code in PR #1810:
URL: https://github.com/apache/solr/pull/1810#discussion_r1283794021
##########
solr/solrj/src/java/org/apache/solr/common/util/Utils.java:
##########
@@ -712,7 +712,7 @@ public static String getBaseUrlForNodeName(
}
final String hostAndPort = nodeName.substring(0, _offset);
final String path = URLDecoder.decode(nodeName.substring(1 + _offset), UTF_8);
Review Comment:
If we don't use path anymore, this line could be removed.
[EDIT] Nevermind, I wasn't looking at the latest version.
--
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 #1810: SOLR-16911: Remove ability to change hostContext, keep it as /solr
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1810:
URL: https://github.com/apache/solr/pull/1810#discussion_r1278016018
##########
solr/core/src/test/org/apache/solr/cloud/LeaderElectionTest.java:
##########
@@ -258,8 +258,8 @@ public void testCancelElection() throws Exception {
Thread.sleep(1000);
String urlScheme = zkStateReader.getClusterProperty(URL_SCHEME, "http");
- String url1 = Utils.getBaseUrlForNodeName("127.0.0.1:80_solr/1", urlScheme) + "/";
- String url2 = Utils.getBaseUrlForNodeName("127.0.0.1:80_solr/2", urlScheme) + "/";
+ String url1 = Utils.getBaseUrlForNodeName("127.0.0.1:80_solr/1", urlScheme) + "/" + "1" + '/';
Review Comment:
I have some concerns about how this test works.... I kind of worked around it, however the naming of the urls for leader election seemed "odd".
--
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 #1810: SOLR-16911: Remove ability to change hostContext, keep it as /solr
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh merged PR #1810:
URL: https://github.com/apache/solr/pull/1810
--
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 #1810: SOLR-16911: Remove ability to change hostContext, keep it as /solr
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1810:
URL: https://github.com/apache/solr/pull/1810#issuecomment-1666235732
@dsmiley @psalagnac all the tests pass. Thoughts on comittting 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] psalagnac commented on a diff in pull request #1810: SOLR-16911: Remove ability to change hostContext, keep it as /solr
Posted by "psalagnac (via GitHub)" <gi...@apache.org>.
psalagnac commented on code in PR #1810:
URL: https://github.com/apache/solr/pull/1810#discussion_r1283793370
##########
solr/core/src/test/org/apache/solr/cloud/LeaderElectionTest.java:
##########
@@ -258,8 +258,8 @@ public void testCancelElection() throws Exception {
Thread.sleep(1000);
String urlScheme = zkStateReader.getClusterProperty(URL_SCHEME, "http");
- String url1 = Utils.getBaseUrlForNodeName("127.0.0.1:80_solr/1", urlScheme) + "/";
- String url2 = Utils.getBaseUrlForNodeName("127.0.0.1:80_solr/2", urlScheme) + "/";
+ String url1 = Utils.getBaseUrlForNodeName("127.0.0.1:80_solr/1", urlScheme) + "/" + "1" + '/';
Review Comment:
Thanks for pining me on this.
This is related to other David's [comment](https://github.com/apache/solr/pull/1810/files/49fe85375b74cd76d7bc08bcc0d32781632bc0d6#r1279827024) on the path removal in `Utils.getBaseUrlForNodeName`.
The change in the test does not make sense to me since the core name is here twice, but it is only once in the result URL. We just want to build the core url from the node base URL. I think it should be like this:
```
String url1 = Utils.getBaseUrlForNodeName("127.0.0.1:80_solr", urlScheme) + "/1/";
```
--
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] psalagnac commented on a diff in pull request #1810: SOLR-16911: Remove ability to change hostContext, keep it as /solr
Posted by "psalagnac (via GitHub)" <gi...@apache.org>.
psalagnac commented on code in PR #1810:
URL: https://github.com/apache/solr/pull/1810#discussion_r1283794021
##########
solr/solrj/src/java/org/apache/solr/common/util/Utils.java:
##########
@@ -712,7 +712,7 @@ public static String getBaseUrlForNodeName(
}
final String hostAndPort = nodeName.substring(0, _offset);
final String path = URLDecoder.decode(nodeName.substring(1 + _offset), UTF_8);
Review Comment:
If we don't use path anymore, this line could be removed.
--
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 #1810: SOLR-16911: Remove ability to change hostContext, keep it as /solr
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1810:
URL: https://github.com/apache/solr/pull/1810#discussion_r1279847230
##########
solr/solrj/src/java/org/apache/solr/common/util/Utils.java:
##########
@@ -712,7 +712,7 @@ public static String getBaseUrlForNodeName(
}
final String hostAndPort = nodeName.substring(0, _offset);
final String path = URLDecoder.decode(nodeName.substring(1 + _offset), UTF_8);
Review Comment:
Good catch. So, at one time we needed the path variable to decide if you had `/solr/mysubsolr` etc.. but now, we have just `/solr` so this can be eliminated actually!
--
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 #1810: SOLR-16911: Remove ability to change hostContext, keep it as /solr
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1810:
URL: https://github.com/apache/solr/pull/1810#issuecomment-1656227489
Locally at least all the bats tests pass except for `test_placement_plugin.bats` who may actually be a bit flaky...
--
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] HoustonPutman commented on a diff in pull request #1810: SOLR-16911: Remove ability to change hostContext, keep it as /solr
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #1810:
URL: https://github.com/apache/solr/pull/1810#discussion_r1279844263
##########
solr/core/src/java/org/apache/solr/cloud/ZkController.java:
##########
@@ -2366,35 +2358,11 @@ public LeaderElector getOverseerElector() {
*
* @param hostName - must not be null or the empty string
* @param hostPort - must consist only of digits, must not be null or the empty string
- * @param hostContext - should not begin or end with a slash (leading/trailin slashes will be
- * ignored), must not be null, may be the empty string to denote the root context
* @lucene.experimental
* @see ZkStateReader#getBaseUrlForNodeName
*/
- static String generateNodeName(
- final String hostName, final String hostPort, final String hostContext) {
- return hostName
- + ':'
- + hostPort
- + '_'
- + URLEncoder.encode(trimLeadingAndTrailingSlashes(hostContext), StandardCharsets.UTF_8);
- }
-
- /**
- * Utility method for trimming and leading and/or trailing slashes from its input. May return the
- * empty string. May return null if and only if the input is null.
- */
- public static String trimLeadingAndTrailingSlashes(final String in) {
- if (null == in) return in;
-
- String out = in;
- if (out.startsWith("/")) {
- out = out.substring(1);
- }
- if (out.endsWith("/")) {
- out = out.substring(0, out.length() - 1);
- }
- return out;
+ static String generateNodeName(final String hostName, final String hostPort) {
+ return hostName + ':' + hostPort + '_' + "solr";
Review Comment:
Yeah if this happens it should be a separate jira and definitely only be Solr 10x. I think leaving it would be fine, but a discussion for a different PR anyways
--
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] dsmiley commented on a diff in pull request #1810: SOLR-16911: Remove ability to change hostContext, keep it as /solr
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1810:
URL: https://github.com/apache/solr/pull/1810#discussion_r1279760576
##########
solr/core/src/java/org/apache/solr/cloud/ZkController.java:
##########
@@ -2366,35 +2358,11 @@ public LeaderElector getOverseerElector() {
*
* @param hostName - must not be null or the empty string
* @param hostPort - must consist only of digits, must not be null or the empty string
- * @param hostContext - should not begin or end with a slash (leading/trailin slashes will be
- * ignored), must not be null, may be the empty string to denote the root context
* @lucene.experimental
* @see ZkStateReader#getBaseUrlForNodeName
*/
- static String generateNodeName(
- final String hostName, final String hostPort, final String hostContext) {
- return hostName
- + ':'
- + hostPort
- + '_'
- + URLEncoder.encode(trimLeadingAndTrailingSlashes(hostContext), StandardCharsets.UTF_8);
- }
-
- /**
- * Utility method for trimming and leading and/or trailing slashes from its input. May return the
- * empty string. May return null if and only if the input is null.
- */
- public static String trimLeadingAndTrailingSlashes(final String in) {
- if (null == in) return in;
-
- String out = in;
- if (out.startsWith("/")) {
- out = out.substring(1);
- }
- if (out.endsWith("/")) {
- out = out.substring(0, out.length() - 1);
- }
- return out;
+ static String generateNodeName(final String hostName, final String hostPort) {
+ return hostName + ':' + hostPort + '_' + "solr";
Review Comment:
I'd love to consider dropping the `_solr` here but this may need to be a 10.0 only thing. Deserves a JIRA issue if we can get this committed.
##########
solr/solrj/src/java/org/apache/solr/common/util/Utils.java:
##########
@@ -712,7 +712,7 @@ public static String getBaseUrlForNodeName(
}
final String hostAndPort = nodeName.substring(0, _offset);
final String path = URLDecoder.decode(nodeName.substring(1 + _offset), UTF_8);
Review Comment:
"path" isn't needed now? Or just leave this code as it was?
##########
solr/test-framework/src/java/org/apache/solr/BaseDistributedSearchTestCase.java:
##########
@@ -189,40 +134,19 @@ public static void clearSolrDisableShardsWhitelist() throws Exception {
}
private static String getHostContextSuitableForServletContext() {
- String ctx = System.getProperty("hostContext", "/solr");
- if (ctx == null || ctx.isEmpty()) ctx = "/solr";
- if (ctx.endsWith("/")) ctx = ctx.substring(0, ctx.length() - 1);
- if (!ctx.startsWith("/")) ctx = "/" + ctx;
+ String ctx = "/solr";
+
return ctx;
Review Comment:
```suggestion
return "/solr";
```
Or just inline this thing! It's just a constant string after all.
##########
solr/core/src/test/org/apache/solr/cloud/LeaderElectionTest.java:
##########
@@ -258,8 +258,8 @@ public void testCancelElection() throws Exception {
Thread.sleep(1000);
String urlScheme = zkStateReader.getClusterProperty(URL_SCHEME, "http");
- String url1 = Utils.getBaseUrlForNodeName("127.0.0.1:80_solr/1", urlScheme) + "/";
- String url2 = Utils.getBaseUrlForNodeName("127.0.0.1:80_solr/2", urlScheme) + "/";
+ String url1 = Utils.getBaseUrlForNodeName("127.0.0.1:80_solr/1", urlScheme) + "/" + "1" + '/';
Review Comment:
Can you elaborate on why this fixes it?
##########
solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java:
##########
@@ -516,10 +509,12 @@ private static CloudConfig fillSolrCloudSection(NamedList<Object> nl, String def
hostPort = parseInt("jetty.port", System.getProperty("jetty.port", "8983"));
}
String hostName = required("solrcloud", "host", removeValue(nl, "host"));
- String hostContext = required("solrcloud", "hostContext", removeValue(nl, "hostContext"));
- CloudConfig.CloudConfigBuilder builder =
- new CloudConfig.CloudConfigBuilder(hostName, hostPort, hostContext);
+ // We no longer require or support the hostContext property, but legacy users may have it, so
+ // remove it from the list.
+ removeValue(nl, "hostContext");
Review Comment:
log a warning if found so users know to drop it
--
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 #1810: SOLR-16911: Remove ability to change hostContext, keep it as /solr
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1810:
URL: https://github.com/apache/solr/pull/1810#discussion_r1279855289
##########
solr/core/src/test/org/apache/solr/cloud/LeaderElectionTest.java:
##########
@@ -258,8 +258,8 @@ public void testCancelElection() throws Exception {
Thread.sleep(1000);
String urlScheme = zkStateReader.getClusterProperty(URL_SCHEME, "http");
- String url1 = Utils.getBaseUrlForNodeName("127.0.0.1:80_solr/1", urlScheme) + "/";
- String url2 = Utils.getBaseUrlForNodeName("127.0.0.1:80_solr/2", urlScheme) + "/";
+ String url1 = Utils.getBaseUrlForNodeName("127.0.0.1:80_solr/1", urlScheme) + "/" + "1" + '/';
Review Comment:
So, I actually am worried this entire idea may fail.. At least the way I read this test, when we do leaderelection, we name our nodes `{some_ip}/solr/1` and `{same_ip}/solr/2` and `{same_ip}/solr/3` and then something happens. However, I don't know if that is ACTUALLY how it happens, or maybe just how this test was written, since I've never dug into leader election in any way.... I also was wondering if maybe we need some sort of `.bats` test of leader election to prove that this change didn't break leader election in the real world.. I mean, technically this LeaderElectionTest.java passes... but...
--
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