From 906e99d631072624cf2edf43f4c6d57c10deb1c9 Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Fri, 25 Sep 2015 08:52:19 -0700 Subject: [PATCH 1/2] RedirectFilter: User agents are more likely to preserve method on 307. --- server/src/main/java/io/druid/server/http/RedirectFilter.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/io/druid/server/http/RedirectFilter.java b/server/src/main/java/io/druid/server/http/RedirectFilter.java index 989b3ba75b0..28582393891 100644 --- a/server/src/main/java/io/druid/server/http/RedirectFilter.java +++ b/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()); } } From 348172203f61939cce44b06a52d0e6cc57060eb4 Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Fri, 25 Sep 2015 08:53:40 -0700 Subject: [PATCH 2/2] OverlordRedirectInfo: Fix ability to detect that there is no leader. --- .../druid/indexing/overlord/TaskMaster.java | 8 +- .../overlord/http/OverlordRedirectInfo.java | 11 ++- .../http/OverlordRedirectInfoTest.java | 83 +++++++++++++++++++ .../http/CoordinatorRedirectInfoTest.java | 13 +-- 4 files changed, 107 insertions(+), 8 deletions(-) create mode 100644 indexing-service/src/test/java/io/druid/indexing/overlord/http/OverlordRedirectInfoTest.java diff --git a/indexing-service/src/main/java/io/druid/indexing/overlord/TaskMaster.java b/indexing-service/src/main/java/io/druid/indexing/overlord/TaskMaster.java index 0f25c2e4470..9362b11c2df 100644 --- a/indexing-service/src/main/java/io/druid/indexing/overlord/TaskMaster.java +++ b/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); diff --git a/indexing-service/src/main/java/io/druid/indexing/overlord/http/OverlordRedirectInfo.java b/indexing-service/src/main/java/io/druid/indexing/overlord/http/OverlordRedirectInfo.java index 4e2cb0d872e..7efc5606dbd 100644 --- a/indexing-service/src/main/java/io/druid/indexing/overlord/http/OverlordRedirectInfo.java +++ b/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); diff --git a/indexing-service/src/test/java/io/druid/indexing/overlord/http/OverlordRedirectInfoTest.java b/indexing-service/src/test/java/io/druid/indexing/overlord/http/OverlordRedirectInfoTest.java new file mode 100644 index 00000000000..d85a9d11fde --- /dev/null +++ b/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); + } +} diff --git a/server/src/test/java/io/druid/server/http/CoordinatorRedirectInfoTest.java b/server/src/test/java/io/druid/server/http/CoordinatorRedirectInfoTest.java index a22cb9b619a..c040bad9ffb 100644 --- a/server/src/test/java/io/druid/server/http/CoordinatorRedirectInfoTest.java +++ b/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 request = "request"; + String query = "foo=bar&x=y"; + String request = "/request"; EasyMock.expect(druidCoordinator.getCurrentLeader()).andReturn(host).anyTimes(); EasyMock.replay(druidCoordinator); - URL url = coordinatorRedirectInfo.getRedirectURL(query,request); - Assert.assertTrue(url.toString().contains(host+request+"?"+query)); + URL url = coordinatorRedirectInfo.getRedirectURL(query, request); + Assert.assertEquals("http://localhost/request?foo=bar&x=y", url.toString()); + EasyMock.verify(druidCoordinator); } }