Fix SolrCloud behavior when using "hostContext" containing "_" or"/" characters. This fix also makes SolrCloud more accepting of hostContext values with leading/trailing slashes

git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1420497 13f79535-47bb-0310-9956-ffa450edef68

solr/CHANGES.txt

@@ -40,6 +40,12 @@ Upgrading from Solr 4.0.0-BETA
 Custom java parsing plugins need to migrade from throwing the internal
 ParseException to throwing SyntaxError.
 
+BaseDistributedSearchTestCase now randomizes the servlet context it uses when 
+creating Jetty instances.  Subclasses that assume a hard coded context of 
+"/solr" should either be fixed to use the "String context" variable, or should 
+take advantage of the new BaseDistributedSearchTestCase(String) constructor
+to explicitly specify a fixed servlet context path.  See SOLR-4136 for details.
+
 
 Detailed Change List
 ----------------------
@@ -321,6 +327,10 @@ Bug Fixes
 * SOLR-4127: Added explicit error message if users attempt Atomic document 
   updates with either updateLog or DistribUpdateProcessor. (hossman)
 
+* SOLR-4136: Fix SolrCloud behavior when using "hostContext" containing "_" 
+  or"/" characters.  This fix also makes SolrCloud more accepting of 
+  hostContext values with leading/trailing slashes. (hossman)
+
 Other Changes
 ----------------------
 

solr/core/src/java/org/apache/solr/cloud/OverseerCollectionProcessor.java

@@ -260,9 +260,8 @@ public class OverseerCollectionProcessor implements Runnable {
           ShardRequest sreq = new ShardRequest();
           params.set("qt", adminPath);
           sreq.purpose = 1;
-          // TODO: this does not work if original url had _ in it
+          String replica = zkStateReader.getZkClient()
-          // We should have a master list
+            .getBaseUrlForNodeName(nodeName);
-          String replica = nodeName.replaceAll("_", "/");
           if (replica.startsWith("http://")) replica = replica.substring(7);
           sreq.shards = new String[] {replica};
           sreq.actualShards = sreq.shards;

solr/core/src/java/org/apache/solr/cloud/ZkController.java

@@ -19,7 +19,9 @@ package org.apache.solr.cloud;
 
 import java.io.File;
 import java.io.IOException;
+import java.io.UnsupportedEncodingException;
 import java.net.InetAddress;
+import java.net.URLEncoder;
 import java.net.NetworkInterface;
 import java.util.Collections;
 import java.util.Enumeration;
@@ -145,18 +147,31 @@ public final class ZkController {
       TimeoutException, IOException {
     if (cc == null) throw new IllegalArgumentException("CoreContainer cannot be null.");
     this.cc = cc;
-    if (localHostContext.contains("/")) {
+    if (localHostContext.startsWith("/")) {
-      throw new IllegalArgumentException("localHostContext ("
+      // be forgiving and strip this off
-          + localHostContext + ") should not contain a /");
+      // this allows us to support users specifying hostContext="/" in 
+      // solr.xml to indicate the root context, instead of hostContext="" 
+      // which means the default of "solr"
+      localHostContext = localHostContext.substring(1);
     }
+    if (localHostContext.endsWith("/")) {
+      // be extra nice
+      localHostContext = localHostContext.substring(0,localHostContext.length()-1);
+    }
+    
     
     this.zkServerAddress = zkServerAddress;
     this.localHostPort = locaHostPort;
     this.localHostContext = localHostContext;
     this.localHost = getHostAddress(localHost);
+    this.baseURL = this.localHost + ":" + this.localHostPort + 
+      (this.localHostContext.isEmpty() ? "" : ("/" + this.localHostContext));
+
     this.hostName = getHostNameFromAddress(this.localHost);
-    this.nodeName = this.hostName + ':' + this.localHostPort + '_' + this.localHostContext;
+    this.nodeName = generateNodeName(this.hostName, 
-    this.baseURL = this.localHost + ":" + this.localHostPort + "/" + this.localHostContext;
+                                     this.localHostPort, 
+                                     this.localHostContext);
+
     this.leaderVoteWait = leaderVoteWait;
     this.clientTimeout = zkClientTimeout;
     zkClient = new SolrZkClient(zkServerAddress, zkClientTimeout, zkClientConnectTimeout,
@@ -1275,4 +1290,23 @@ public final class ZkController {
     return cmdDistribExecutor;
   }
 
+  /**
+   * Returns the nodeName that should be used based on the specified properties.
+   *
+   * @param hostName - must not be the empty string
+   * @param hostPort - must consist only of digits, must not be the empty string
+   * @param hostContext - should not begin or end with a slash, may be the empty string to denote the root context
+   * @lucene.experimental
+   * @see SolrZkClient#getBaseUrlForNodeName
+   */
+  static String generateNodeName(final String hostName,
+                                 final String hostPort,
+                                 final String hostContext) {
+    try {
+      return hostName + ':' + hostPort + '_' + 
+        URLEncoder.encode(hostContext, "UTF-8");
+    } catch (UnsupportedEncodingException e) {
+      throw new IllegalStateException("JVM Does not seem to support UTF-8", e);
+    }
+  }
 }

solr/core/src/test-files/solr/solr.xml

@@ -29,7 +29,7 @@
     If 'null' (or absent), cores will not be manageable via request handler
   -->
   <cores adminPath="/admin/cores" defaultCoreName="collection1" host="127.0.0.1" hostPort="${hostPort:8983}" 
-         hostContext="solr" zkClientTimeout="8000" numShards="${numShards:3}" shareSchema="${shareSchema:false}">
+         hostContext="${hostContext:solr}" zkClientTimeout="8000" numShards="${numShards:3}" shareSchema="${shareSchema:false}">
     <core name="collection1" instanceDir="collection1" shard="${shard:}" collection="${collection:collection1}" config="${solrconfig:solrconfig.xml}" schema="${schema:schema.xml}"/>
   </cores>
 </solr>

solr/core/src/test/org/apache/solr/cloud/OverseerCollectionProcessorTest.java

@@ -35,6 +35,7 @@ import java.util.Set;
 
 import org.apache.solr.SolrTestCaseJ4;
 import org.apache.solr.common.cloud.ClusterState;
+import org.apache.solr.common.cloud.SolrZkClient;
 import org.apache.solr.common.cloud.ZkNodeProps;
 import org.apache.solr.common.cloud.ZkStateReader;
 import org.apache.solr.common.params.CoreAdminParams;
@@ -61,6 +62,7 @@ public class OverseerCollectionProcessorTest extends SolrTestCaseJ4 {
   private ShardHandler shardHandlerMock;
   private ZkStateReader zkStateReaderMock;
   private ClusterState clusterStateMock;
+  private SolrZkClient solrZkClientMock;
   
   private Thread thread;
   private Queue<byte[]> queue = new BlockingArrayQueue<byte[]>();
@@ -88,6 +90,7 @@ public class OverseerCollectionProcessorTest extends SolrTestCaseJ4 {
     shardHandlerMock = createMock(ShardHandler.class);
     zkStateReaderMock = createMock(ZkStateReader.class);
     clusterStateMock = createMock(ClusterState.class);
+    solrZkClientMock = createMock(SolrZkClient.class);
     underTest = new OverseerCollectionProcessorToBeTested(zkStateReaderMock,
         "1234", shardHandlerMock, ADMIN_PATH, workQueueMock);
   }
@@ -129,6 +132,15 @@ public class OverseerCollectionProcessorTest extends SolrTestCaseJ4 {
       }
     }).anyTimes();
     
+    zkStateReaderMock.getZkClient();
+    expectLastCall().andAnswer(new IAnswer<Object>() {
+      @Override
+      public Object answer() throws Throwable {
+        return solrZkClientMock;
+      }
+    }).anyTimes();
+    
+    
     clusterStateMock.getCollections();
     expectLastCall().andAnswer(new IAnswer<Object>() {
       @Override
@@ -138,7 +150,19 @@ public class OverseerCollectionProcessorTest extends SolrTestCaseJ4 {
     }).anyTimes();
     final Set<String> liveNodes = new HashSet<String>();
     for (int i = 0; i < liveNodesCount; i++) {
-      liveNodes.add("localhost:" + (8963 + i) + "_solr");
+      final String address = "localhost:" + (8963 + i) + "_solr";
+      liveNodes.add(address);
+      
+      solrZkClientMock.getBaseUrlForNodeName(address);
+      expectLastCall().andAnswer(new IAnswer<Object>() {
+        @Override
+        public Object answer() throws Throwable {
+          // This works as long as this test does not use a 
+          // webapp context with an underscore in it
+          return address.replaceAll("_", "/");
+        }
+      }).anyTimes();
+      
     }
     clusterStateMock.getLiveNodes();
     expectLastCall().andAnswer(new IAnswer<Object>() {
@@ -336,6 +360,7 @@ public class OverseerCollectionProcessorTest extends SolrTestCaseJ4 {
     }
     
     replay(workQueueMock);
+    replay(solrZkClientMock);
     replay(zkStateReaderMock);
     replay(clusterStateMock);
     replay(shardHandlerMock);

solr/core/src/test/org/apache/solr/cloud/ZkControllerTest.java

@@ -49,6 +49,62 @@ public class ZkControllerTest extends SolrTestCaseJ4 {
     initCore();
   }
 
+  public void testNodeNameUrlConversion() throws Exception {
+
+    // nodeName from parts
+    assertEquals("localhost:8888_solr",
+                 ZkController.generateNodeName("localhost", "8888", "solr"));
+    assertEquals("localhost:8888_", // root context
+                 ZkController.generateNodeName("localhost", "8888", ""));
+    assertEquals("foo-bar:77_solr%2Fsub_dir",
+                 ZkController.generateNodeName("foo-bar", "77", "solr/sub_dir"));
+
+    // setup a SolrZkClient to do some getBaseUrlForNodeName testing
+    String zkDir = dataDir.getAbsolutePath() + File.separator
+        + "zookeeper/server1/data";
+
+    ZkTestServer server = new ZkTestServer(zkDir);
+    try {
+      server.run();
+
+      AbstractZkTestCase.tryCleanSolrZkNode(server.getZkHost());
+      AbstractZkTestCase.makeSolrZkNode(server.getZkHost());
+
+      SolrZkClient zkClient = new SolrZkClient(server.getZkAddress(), TIMEOUT);
+      try {
+
+        // getBaseUrlForNodeName
+        assertEquals("http://zzz.xxx:1234/solr",
+                     zkClient.getBaseUrlForNodeName("zzz.xxx:1234_solr"));
+        assertEquals("http://xxx:99/",
+                     zkClient.getBaseUrlForNodeName("xxx:99_"));
+        assertEquals("http://foo-bar.baz.org:9999/some_dir",
+                     zkClient.getBaseUrlForNodeName("foo-bar.baz.org:9999_some_dir"));
+        assertEquals("http://foo-bar.baz.org:9999/solr/sub_dir",
+                     zkClient.getBaseUrlForNodeName("foo-bar.baz.org:9999_solr%2Fsub_dir"));
+        
+        
+        // generateNodeName + getBaseUrlForNodeName
+        assertEquals("http://foo:9876/solr",
+                     zkClient.getBaseUrlForNodeName
+                     (ZkController.generateNodeName("foo","9876","solr")));
+        assertEquals("http://foo.bar.com:9876/solr/sub_dir",
+                     zkClient.getBaseUrlForNodeName
+                     (ZkController.generateNodeName("foo.bar.com","9876","solr/sub_dir")));
+        assertEquals("http://foo-bar:9876/",
+                     zkClient.getBaseUrlForNodeName
+                     (ZkController.generateNodeName("foo-bar","9876","")));
+        assertEquals("http://foo-bar.com:80/some_dir",
+                     zkClient.getBaseUrlForNodeName
+                     (ZkController.generateNodeName("foo-bar.com","80","some_dir")));
+      } finally {
+        zkClient.close();
+      }
+    } finally {
+      server.shutdown();
+    }
+  }
+
   @Test
   public void testReadConfigName() throws Exception {
     String zkDir = dataDir.getAbsolutePath() + File.separator

solr/example/contexts/solr.xml

@@ -1,7 +1,7 @@
 <?xml version="1.0"?>
 <!DOCTYPE Configure PUBLIC "-//Jetty//Configure//EN" "http://www.eclipse.org/jetty/configure.dtd">
 <Configure class="org.eclipse.jetty.webapp.WebAppContext">
-  <Set name="contextPath">/solr</Set>
+  <Set name="contextPath"><SystemProperty name="hostContext" default="/solr"/></Set>
   <Set name="war"><SystemProperty name="jetty.home"/>/webapps/solr.war</Set>
   <Set name="defaultsDescriptor"><SystemProperty name="jetty.home"/>/etc/webdefault.xml</Set>
   <Set name="tempDirectory"><Property name="jetty.home" default="."/>/solr-webapp</Set>

solr/solrj/src/java/org/apache/solr/common/cloud/SolrZkClient.java

@@ -22,6 +22,7 @@ import java.io.IOException;
 import java.io.StringReader;
 import java.io.StringWriter;
 import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
 import java.util.List;
 import java.util.concurrent.TimeoutException;
 import java.util.concurrent.atomic.AtomicLong;
@@ -460,6 +461,28 @@ public class SolrZkClient {
     setData(path, data.getBytes("UTF-8"), retryOnConnLoss);
   }
 
+  /**
+   * Returns the baseURL corrisponding to a given node's nodeName -- 
+   * NOTE: does not (currently) imply that the nodeName (or resulting 
+   * baseURL) exists in the cluster.
+   * @lucene.experimental
+   */
+  public String getBaseUrlForNodeName(final String nodeName) {
+    final int _offset = nodeName.indexOf("_");
+    if (_offset < 0) {
+      throw new IllegalArgumentException("nodeName does not contain expected '_' seperator: " + nodeName);
+    }
+    final String hostAndPort = nodeName.substring(0,_offset);
+    try {
+      final String path = URLDecoder.decode(nodeName.substring(1+_offset),
+                                            "UTF-8");
+      return "http://" + hostAndPort + "/" + path;
+    } catch (UnsupportedEncodingException e) {
+      throw new IllegalStateException("JVM Does not seem to support UTF-8", e);
+    }
+  }
+
+
   /**
    * Fills string with printout of current ZooKeeper layout.
    */

solr/solrj/src/test-files/solrj/solr/shared/solr.xml

@@ -30,7 +30,7 @@
   adminPath: RequestHandler path to manage cores.  
     If 'null' (or absent), cores will not be manageable via REST
   -->
-  <cores adminPath="/admin/cores" defaultCoreName="core0" host="127.0.0.1" hostPort="${hostPort:8983}" hostContext="solr" zkClientTimeout="8000">
+  <cores adminPath="/admin/cores" defaultCoreName="core0" host="127.0.0.1" hostPort="${hostPort:8983}" hostContext="${hostContext:}" zkClientTimeout="8000">
     <core name="collection1" instanceDir="." />
     <core name="core0" instanceDir="${theInstanceDir:./}" collection="${collection:acollection}">
       <property name="version" value="3.5"/>

solr/solrj/src/test-files/solrj/solr/solr.xml

@@ -28,7 +28,7 @@
   adminPath: RequestHandler path to manage cores.  
     If 'null' (or absent), cores will not be manageable via request handler
   -->
-  <cores adminPath="/admin/cores" defaultCoreName="collection1" host="127.0.0.1" hostPort="${hostPort:8983}" hostContext="solr" zkClientTimeout="8000" numShards="${numShards:3}">
+  <cores adminPath="/admin/cores" defaultCoreName="collection1" host="127.0.0.1" hostPort="${hostPort:8983}" hostContext="${hostContext:}" zkClientTimeout="8000" numShards="${numShards:3}">
     <core name="collection1" instanceDir="collection1" shard="${shard:}" collection="${collection:collection1}" config="${solrconfig:solrconfig.xml}" schema="${schema:schema.xml}"/>
   </cores>
 </solr>

solr/test-framework/src/java/org/apache/solr/BaseDistributedSearchTestCase.java

@@ -33,6 +33,7 @@ import java.util.Set;
 import junit.framework.Assert;
 
 import org.apache.lucene.search.FieldCache;
+import org.apache.lucene.util. _TestUtil;
 import org.apache.lucene.util.Constants;
 import org.apache.solr.client.solrj.SolrServer;
 import org.apache.solr.client.solrj.SolrServerException;
@@ -50,6 +51,7 @@ import org.apache.solr.common.util.NamedList;
 import org.apache.solr.schema.TrieDateField;
 import org.apache.solr.util.AbstractSolrTestCase;
 import org.junit.BeforeClass;
+import org.junit.AfterClass;
 import org.junit.Test;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -69,6 +71,100 @@ public abstract class BaseDistributedSearchTestCase extends SolrTestCaseJ4 {
     r = new Random(random().nextLong());
   }
   
+  /**
+   * Set's the value of the "hostContext" system property to a random path 
+   * like string (which may or may not contain sub-paths).  This is used 
+   * in the default constructor for this test to help ensure no code paths have
+   * hardcoded assumptions about the servlet context used to run solr.
+   * <p>
+   * Test configs may use the <code>${hostContext}</code> variable to access 
+   * this system property.
+   * </p>
+   * @see #BaseDistributedSearchTestCase()
+   * @see #clearHostContext
+   */
+  @BeforeClass
+  public static void initHostContext() {
+    // Can't use randomRealisticUnicodeString because unescaped unicode is 
+    // not allowed in URL paths
+    // Can't use URLEncoder.encode(randomRealisticUnicodeString) because
+    // Jetty freaks out and returns 404's when the context uses escapes
+
+    StringBuilder hostContext = new StringBuilder("/");
+    if (random().nextBoolean()) {
+      // half the time we use the root context, the other half...
+
+      // Remember: randomSimpleString might be the empty string
+      hostContext.append(_TestUtil.randomSimpleString(random(), 2));
+      if (random().nextBoolean()) {
+        hostContext.append("_");
+      }
+      hostContext.append(_TestUtil.randomSimpleString(random(), 3));
+      if ( ! "/".equals(hostContext)) {
+        // if our random string is empty, this might add a trailing slash, 
+        // but our code should be ok with that
+        hostContext.append("/").append(_TestUtil.randomSimpleString(random(), 2));
+      } else {
+        // we got 'lucky' and still just have the root context,
+        // NOOP: don't try to add a subdir to nothing (ie "//" is bad)
+      }
+    }
+
+    log.info("Setting hostContext system property: " + hostContext.toString());
+    System.setProperty("hostContext", hostContext.toString());
+  }
+
+  /**
+   * Clears the "hostContext" system property
+   * @see #initHostContext
+   */
+  @AfterClass
+  public static void clearHostContext() throws Exception {
+    System.clearProperty("hostContext");
+  }
+
+  private static String getHostContextSuitableForServletContext() {
+    String ctx = System.getProperty("hostContext","/solr");
+    if ("".equals(ctx)) ctx = "/solr";
+    if (ctx.endsWith("/")) ctx = ctx.substring(0,ctx.length()-1);;
+    if (!ctx.startsWith("/")) ctx = "/" + ctx;
+    return ctx;
+  }
+
+  /**
+   * Constructs a test in which the jetty+solr instances as well as the 
+   * solr clients all use the value of the "hostContext" system property.
+   * <p>
+   * If the system property is not set, or is set to the empty string 
+   * (neither of which should normally happen unless a subclass explicitly 
+   * modifies the property set by {@link #initHostContext} prior to calling 
+   * this constructor) a servlet context of "/solr" is used. (this is for 
+   * consistency with the default behavior of solr.xml parsing when using 
+   * <code>hostContext="${hostContext:}"</code>
+   * </p>
+   * <p>
+   * If the system property is set to a value which does not begin with a 
+   * "/" (which should normally happen unless a subclass explicitly 
+   * modifies the property set by {@link #initHostContext} prior to calling 
+   * this constructor) a leading "/" will be prepended.
+   * </p>
+   *
+   * @see #initHostContext
+   */
+  protected BaseDistributedSearchTestCase() {
+    this(getHostContextSuitableForServletContext());
+  }
+
+  /**
+   * @param context explicit servlet context path to use (eg: "/solr")
+   */
+  protected BaseDistributedSearchTestCase(final String context) {
+    this.context = context;
+    this.deadServers = new String[] {"[ff01::114]:33332" + context, 
+                                     "[ff01::083]:33332" + context, 
+                                     "[ff01::213]:33332" + context};
+  }
+
   protected int shardCount = 4;      // the actual number of solr cores that will be created in the cluster
 
   /**
@@ -84,12 +180,10 @@ public abstract class BaseDistributedSearchTestCase extends SolrTestCaseJ4 {
   protected List<SolrServer> clients = new ArrayList<SolrServer>();
   protected List<JettySolrRunner> jettys = new ArrayList<JettySolrRunner>();
   
-  protected String context = "/solr";
+  protected String context;
+  protected String[] deadServers;
   protected String shards;
   protected String[] shardsArr;
-  // Some ISPs redirect to their own web site for domains that don't exist, causing this to fail
-  // protected String[] deadServers = {"does_not_exist_54321.com:33331/solr","127.0.0.1:33332/solr"};
-  protected String[] deadServers = {"[ff01::114]:33332/solr", "[ff01::083]:33332/solr", "[ff01::213]:33332/solr"};
   protected File testDir;
   protected SolrServer controlClient;
 
@@ -258,7 +352,7 @@ public abstract class BaseDistributedSearchTestCase extends SolrTestCaseJ4 {
   
   public JettySolrRunner createJetty(File solrHome, String dataDir, String shardList, String solrConfigOverride, String schemaOverride) throws Exception {
 
-    JettySolrRunner jetty = new JettySolrRunner(solrHome.getAbsolutePath(), "/solr", 0, solrConfigOverride, schemaOverride);
+    JettySolrRunner jetty = new JettySolrRunner(solrHome.getAbsolutePath(), context, 0, solrConfigOverride, schemaOverride);
     jetty.setShards(shardList);
     jetty.setDataDir(dataDir);
     jetty.start();

solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java

@@ -356,7 +356,7 @@ public abstract class AbstractFullDistribZkTestBase extends AbstractDistribZkTes
   public JettySolrRunner createJetty(String dataDir, String shardList,
       String solrConfigOverride) throws Exception {
     
-    JettySolrRunner jetty = new JettySolrRunner(getSolrHome(), "/solr", 0,
+    JettySolrRunner jetty = new JettySolrRunner(getSolrHome(), context, 0,
         solrConfigOverride, null, false);
     jetty.setShards(shardList);
     jetty.setDataDir(dataDir);