SOLR-16911: Remove ability to change hostContext, keep it as /solr #1810
Conversation
…tedSearchTestCase
…es to it. We no longer need hostContext to be defined in solr.xml since it's always /solr.
… update method signatures.
|
Locally at least all the bats tests pass except for |
| @@ -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" + '/'; | |||
There was a problem hiding this 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".
There was a problem hiding this comment.
Can you elaborate on why this fixes it?
There was a problem hiding this 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...
There was a problem hiding this comment.
@psalagnac WDYT of the changes to this test?
There was a problem hiding this comment.
Thanks for tagging @psalagnac... I think getting this resolved is the last thing that needs doing... I am wondering if I need to create some sort of .bats test that validates the happy path of leaderelection...?
There was a problem hiding this comment.
Thanks for pining me on this.
This is related to other David's comment 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/";
| } | ||
| return out; | ||
| static String generateNodeName(final String hostName, final String hostPort) { | ||
| return hostName + ':' + hostPort + '_' + "solr"; |
There was a problem hiding this 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.
There was a problem hiding this 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
| @@ -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" + '/'; | |||
There was a problem hiding this comment.
Can you elaborate on why this fixes it?
solr/test-framework/src/java/org/apache/solr/BaseDistributedSearchTestCase.java
Outdated
Show resolved
Hide resolved
…archTestCase.java Co-authored-by: David Smiley <dsmiley@apache.org>
| @@ -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); | |||
There was a problem hiding this comment.
If we don't use path anymore, this line could be removed.
[EDIT] Nevermind, I wasn't looking at the latest version.
|
@dsmiley @psalagnac all the tests pass. Thoughts on comittting this? |
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:
mainbranch../gradlew check.