From a3a9c5f409f7a0985973e062ecd5756f653bb486 Mon Sep 17 00:00:00 2001 From: Karan Kumar Date: Wed, 17 Aug 2022 18:27:39 +0530 Subject: [PATCH] Fixing overlord issued too many redirects (#12908) * Fixing race in overlord redirects where the node was redirecting to itself * Fixing test cases --- .../druid/indexing/overlord/TaskMaster.java | 13 +++++++++++++ .../overlord/http/OverlordRedirectInfo.java | 8 ++++---- .../http/OverlordRedirectInfoTest.java | 19 +++++-------------- .../indexing/overlord/http/OverlordTest.java | 2 +- .../apache/druid/rpc/ServiceClientImpl.java | 14 ++++++++++++-- .../druid/rpc/ServiceClientImplTest.java | 9 +++++---- 6 files changed, 40 insertions(+), 25 deletions(-) diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskMaster.java b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskMaster.java index 5825a2fb03f..b52a3c5c03e 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskMaster.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskMaster.java @@ -232,6 +232,19 @@ public class TaskMaster implements TaskCountStatsProvider, TaskSlotCountStatsPro return overlordLeaderSelector.getCurrentLeader(); } + public Optional getRedirectLocation() + { + String leader = overlordLeaderSelector.getCurrentLeader(); + // do not redirect when + // leader is not elected + // leader is the current node + if (leader == null || leader.isEmpty() || overlordLeaderSelector.isLeader()) { + return Optional.absent(); + } else { + return Optional.of(leader); + } + } + public Optional getTaskRunner() { if (isLeader()) { diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordRedirectInfo.java b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordRedirectInfo.java index 0faa6c8aadd..4e332f599df 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordRedirectInfo.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordRedirectInfo.java @@ -19,6 +19,7 @@ package org.apache.druid.indexing.overlord.http; +import com.google.common.base.Optional; import com.google.common.collect.ImmutableSet; import com.google.inject.Inject; import org.apache.druid.indexing.overlord.TaskMaster; @@ -55,13 +56,12 @@ public class OverlordRedirectInfo implements RedirectInfo public URL getRedirectURL(String queryString, String requestURI) { try { - final String leader = taskMaster.getCurrentLeader(); - if (leader == null || leader.isEmpty()) { + final Optional redirectLocation = taskMaster.getRedirectLocation(); + if (!redirectLocation.isPresent()) { return null; } - String location = StringUtils.format("%s%s", leader, requestURI); - + String location = StringUtils.format("%s%s", redirectLocation.get(), requestURI); if (queryString != null) { location = StringUtils.format("%s?%s", location, queryString); } diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/OverlordRedirectInfoTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/OverlordRedirectInfoTest.java index 33ea5e75c7f..46e03206f49 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/OverlordRedirectInfoTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/OverlordRedirectInfoTest.java @@ -19,6 +19,7 @@ package org.apache.druid.indexing.overlord.http; +import com.google.common.base.Optional; import org.apache.druid.indexing.overlord.TaskMaster; import org.easymock.EasyMock; import org.junit.Assert; @@ -66,19 +67,9 @@ public class OverlordRedirectInfoTest } @Test - public void testGetRedirectURLNull() + public void testGetRedirectURLWithEmptyLocation() { - EasyMock.expect(taskMaster.getCurrentLeader()).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.getCurrentLeader()).andReturn("").anyTimes(); + EasyMock.expect(taskMaster.getRedirectLocation()).andReturn(Optional.absent()).anyTimes(); EasyMock.replay(taskMaster); URL url = redirectInfo.getRedirectURL("query", "/request"); Assert.assertNull(url); @@ -91,7 +82,7 @@ public class OverlordRedirectInfoTest String host = "http://localhost"; String query = "foo=bar&x=y"; String request = "/request"; - EasyMock.expect(taskMaster.getCurrentLeader()).andReturn(host).anyTimes(); + EasyMock.expect(taskMaster.getRedirectLocation()).andReturn(Optional.of(host)).anyTimes(); EasyMock.replay(taskMaster); URL url = redirectInfo.getRedirectURL(query, request); Assert.assertEquals("http://localhost/request?foo=bar&x=y", url.toString()); @@ -107,7 +98,7 @@ public class OverlordRedirectInfoTest "UTF-8" ) + "/status"; - EasyMock.expect(taskMaster.getCurrentLeader()).andReturn(host).anyTimes(); + EasyMock.expect(taskMaster.getRedirectLocation()).andReturn(Optional.of(host)).anyTimes(); EasyMock.replay(taskMaster); URL url = redirectInfo.getRedirectURL(null, request); Assert.assertEquals( diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/OverlordTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/OverlordTest.java index 40ed6e02122..02a71f85604 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/OverlordTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/OverlordTest.java @@ -80,7 +80,6 @@ import org.junit.Test; import javax.annotation.Nullable; import javax.servlet.http.HttpServletRequest; import javax.ws.rs.core.Response; - import java.util.ArrayList; import java.util.Collection; import java.util.List; @@ -217,6 +216,7 @@ public class OverlordTest Thread.sleep(10); } Assert.assertEquals(taskMaster.getCurrentLeader(), druidNode.getHostAndPort()); + Assert.assertEquals(Optional.absent(), taskMaster.getRedirectLocation()); final TaskStorageQueryAdapter taskStorageQueryAdapter = new TaskStorageQueryAdapter(taskStorage, taskLockbox); final WorkerTaskRunnerQueryAdapter workerTaskRunnerQueryAdapter = new WorkerTaskRunnerQueryAdapter(taskMaster, null); diff --git a/server/src/main/java/org/apache/druid/rpc/ServiceClientImpl.java b/server/src/main/java/org/apache/druid/rpc/ServiceClientImpl.java index 9a2ce234c5e..e7fbf72b1d3 100644 --- a/server/src/main/java/org/apache/druid/rpc/ServiceClientImpl.java +++ b/server/src/main/java/org/apache/druid/rpc/ServiceClientImpl.java @@ -200,7 +200,12 @@ public class ServiceClientImpl implements ServiceClient final String newUri = result.error().getResponse().headers().get("Location"); if (redirectCount >= MAX_REDIRECTS) { - retVal.setException(new RpcException("Service [%s] issued too many redirects", serviceName)); + retVal.setException(new RpcException( + "Service [%s] redirected too many times [%d] to invalid url %s", + serviceName, + redirectCount, + newUri + )); } else { // Update preferredLocationNoPath if we got a redirect. final ServiceLocation redirectLocationNoPath = serviceLocationNoPathFromUri(newUri); @@ -212,7 +217,12 @@ public class ServiceClientImpl implements ServiceClient ); } else { retVal.setException( - new RpcException("Service [%s] redirected to invalid URL [%s]", serviceName, newUri) + new RpcException( + "Service [%s] redirected [%d] times to invalid URL [%s]", + serviceName, + redirectCount, + newUri + ) ); } } diff --git a/server/src/test/java/org/apache/druid/rpc/ServiceClientImplTest.java b/server/src/test/java/org/apache/druid/rpc/ServiceClientImplTest.java index 01540c5d7e1..f998bf82d7b 100644 --- a/server/src/test/java/org/apache/druid/rpc/ServiceClientImplTest.java +++ b/server/src/test/java/org/apache/druid/rpc/ServiceClientImplTest.java @@ -289,7 +289,7 @@ public class ServiceClientImplTest MatcherAssert.assertThat(e.getCause(), CoreMatchers.instanceOf(RpcException.class)); MatcherAssert.assertThat( e.getCause(), - ThrowableMessageMatcher.hasMessage(CoreMatchers.containsString("too many redirects")) + ThrowableMessageMatcher.hasMessage(CoreMatchers.containsString("redirected too many times")) ); } @@ -313,7 +313,8 @@ public class ServiceClientImplTest MatcherAssert.assertThat(e.getCause(), CoreMatchers.instanceOf(RpcException.class)); MatcherAssert.assertThat( e.getCause(), - ThrowableMessageMatcher.hasMessage(CoreMatchers.containsString("redirected to invalid URL [invalid-url]")) + ThrowableMessageMatcher.hasMessage( + CoreMatchers.containsString("redirected [0] times to invalid URL [invalid-url]")) ); } @@ -337,7 +338,7 @@ public class ServiceClientImplTest MatcherAssert.assertThat(e.getCause(), CoreMatchers.instanceOf(RpcException.class)); MatcherAssert.assertThat( e.getCause(), - ThrowableMessageMatcher.hasMessage(CoreMatchers.containsString("redirected to invalid URL [null]")) + ThrowableMessageMatcher.hasMessage(CoreMatchers.containsString("redirected [0] times to invalid URL [null]")) ); } @@ -361,7 +362,7 @@ public class ServiceClientImplTest MatcherAssert.assertThat(e.getCause(), CoreMatchers.instanceOf(RpcException.class)); MatcherAssert.assertThat( e.getCause(), - ThrowableMessageMatcher.hasMessage(CoreMatchers.containsString("too many redirects")) + ThrowableMessageMatcher.hasMessage(CoreMatchers.containsString("redirected too many times")) ); }