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-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

Reply via email to