Merge pull request #1778 from gianm/redirect-fixes

Redirect fixes

indexing-service/src/main/java/io/druid/indexing/overlord/TaskMaster.java

@@ -40,6 +40,7 @@ import io.druid.server.initialization.IndexerZkConfig;
 import org.apache.curator.framework.CuratorFramework;
 import org.apache.curator.framework.recipes.leader.LeaderSelector;
 import org.apache.curator.framework.recipes.leader.LeaderSelectorListener;
+import org.apache.curator.framework.recipes.leader.Participant;
 import org.apache.curator.framework.state.ConnectionState;
 
 import java.util.concurrent.atomic.AtomicReference;
@@ -243,7 +244,12 @@ public class TaskMaster
   public String getLeader()
   {
     try {
-      return leaderSelector.getLeader().getId();
+      final Participant leader = leaderSelector.getLeader();
+      if (leader != null && leader.isLeader()) {
+        return leader.getId();
+      } else {
+        return null;
+      }
     }
     catch (Exception e) {
       throw Throwables.propagate(e);

indexing-service/src/main/java/io/druid/indexing/overlord/http/OverlordRedirectInfo.java

@@ -19,13 +19,15 @@ package io.druid.indexing.overlord.http;
 
 import com.google.common.base.Throwables;
 import com.google.inject.Inject;
+import com.metamx.common.IAE;
 import io.druid.indexing.overlord.TaskMaster;
 import io.druid.server.http.RedirectInfo;
 
+import java.net.URI;
 import java.net.URL;
 
 /**
-*/
+ */
 public class OverlordRedirectInfo implements RedirectInfo
 {
   private final TaskMaster taskMaster;
@@ -46,7 +48,12 @@ public class OverlordRedirectInfo implements RedirectInfo
   public URL getRedirectURL(String queryString, String requestURI)
   {
     try {
-      return new URL(String.format("http://%s%s", taskMaster.getLeader(), requestURI));
+      final String leader = taskMaster.getLeader();
+      if (leader == null || leader.isEmpty()) {
+        return null;
+      } else {
+        return new URI("http", leader, requestURI, queryString, null).toURL();
+      }
     }
     catch (Exception e) {
       throw Throwables.propagate(e);

indexing-service/src/test/java/io/druid/indexing/overlord/http/OverlordRedirectInfoTest.java

@@ -0,0 +1,83 @@
+/*
+ * Licensed to Metamarkets Group Inc. (Metamarkets) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. Metamarkets licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package io.druid.indexing.overlord.http;
+
+import io.druid.indexing.overlord.TaskMaster;
+import org.easymock.EasyMock;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.net.URL;
+
+public class OverlordRedirectInfoTest
+{
+  private TaskMaster taskMaster;
+  private OverlordRedirectInfo redirectInfo;
+
+  @Before
+  public void setUp()
+  {
+    taskMaster = EasyMock.createMock(TaskMaster.class);
+    redirectInfo = new OverlordRedirectInfo(taskMaster);
+  }
+
+  @Test
+  public void testDoLocal()
+  {
+    EasyMock.expect(taskMaster.isLeading()).andReturn(true).anyTimes();
+    EasyMock.replay(taskMaster);
+    Assert.assertTrue(redirectInfo.doLocal());
+    EasyMock.verify(taskMaster);
+  }
+
+  @Test
+  public void testGetRedirectURLNull()
+  {
+    EasyMock.expect(taskMaster.getLeader()).andReturn(null).anyTimes();
+    EasyMock.replay(taskMaster);
+    URL url = redirectInfo.getRedirectURL("query", "/request");
+    Assert.assertNull(url);
+    EasyMock.verify(taskMaster);
+  }
+
+  @Test
+  public void testGetRedirectURLEmpty()
+  {
+    EasyMock.expect(taskMaster.getLeader()).andReturn("").anyTimes();
+    EasyMock.replay(taskMaster);
+    URL url = redirectInfo.getRedirectURL("query", "/request");
+    Assert.assertNull(url);
+    EasyMock.verify(taskMaster);
+  }
+
+  @Test
+  public void testGetRedirectURL()
+  {
+    String host = "localhost";
+    String query = "foo=bar&x=y";
+    String request = "/request";
+    EasyMock.expect(taskMaster.getLeader()).andReturn(host).anyTimes();
+    EasyMock.replay(taskMaster);
+    URL url = redirectInfo.getRedirectURL(query, request);
+    Assert.assertEquals("http://localhost/request?foo=bar&x=y", url.toString());
+    EasyMock.verify(taskMaster);
+  }
+}

server/src/main/java/io/druid/server/http/RedirectFilter.java

@@ -72,12 +72,12 @@ public class RedirectFilter implements Filter
       log.debug("Forwarding request to [%s]", url);
 
       if (url == null) {
-        // We apparently have no coordinator, so let's do a Service Unavailable
+        // We apparently have nothing to redirect to, so let's do a Service Unavailable
         response.sendError(HttpServletResponse.SC_SERVICE_UNAVAILABLE);
         return;
       }
 
-      response.setStatus(HttpServletResponse.SC_MOVED_TEMPORARILY);
+      response.setStatus(HttpServletResponse.SC_TEMPORARY_REDIRECT);
       response.setHeader("Location", url.toString());
     }
   }

server/src/test/java/io/druid/server/http/CoordinatorRedirectInfoTest.java

@@ -53,18 +53,21 @@ public class CoordinatorRedirectInfoTest
   {
     EasyMock.expect(druidCoordinator.getCurrentLeader()).andReturn(null).anyTimes();
     EasyMock.replay(druidCoordinator);
-    URL url = coordinatorRedirectInfo.getRedirectURL("query", "request");
+    URL url = coordinatorRedirectInfo.getRedirectURL("query", "/request");
     Assert.assertNull(url);
+    EasyMock.verify(druidCoordinator);
   }
+
   @Test
   public void testGetRedirectURL()
   {
     String host = "localhost";
-    String query = "query";
+    String query = "foo=bar&x=y";
-    String request = "request";
+    String request = "/request";
     EasyMock.expect(druidCoordinator.getCurrentLeader()).andReturn(host).anyTimes();
     EasyMock.replay(druidCoordinator);
-    URL url = coordinatorRedirectInfo.getRedirectURL(query,request);
+    URL url = coordinatorRedirectInfo.getRedirectURL(query, request);
-    Assert.assertTrue(url.toString().contains(host+request+"?"+query));
+    Assert.assertEquals("http://localhost/request?foo=bar&x=y", url.toString());
+    EasyMock.verify(druidCoordinator);
   }
 }