From 73769ff14dc8f9d787b8af577b6d844053ffbee1 Mon Sep 17 00:00:00 2001 From: Abhishek Singh Chouhan Date: Fri, 14 Apr 2023 12:14:16 -0700 Subject: [PATCH 1/8] Refresh DruidLeaderClient cache for non-200 responses --- ...torPollingBasicAuthorizerCacheManager.java | 5 +- .../druid/catalog/sync/CatalogClient.java | 4 +- .../indexing/worker/WorkerTaskManager.java | 6 +- .../client/coordinator/CoordinatorClient.java | 5 +- .../indexing/HttpIndexingServiceClient.java | 5 +- .../druid/discovery/DruidLeaderClient.java | 145 ++++++++++++++---- .../query/lookup/LookupReferencesManager.java | 4 +- .../HttpIndexingServiceClientTest.java | 7 +- .../discovery/DruidLeaderClientTest.java | 86 +++++++++++ .../lookup/LookupReferencesManagerTest.java | 132 +++++++++------- .../sql/calcite/schema/SystemSchemaTest.java | 12 +- 11 files changed, 309 insertions(+), 102 deletions(-) diff --git a/extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/authorization/db/cache/CoordinatorPollingBasicAuthorizerCacheManager.java b/extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/authorization/db/cache/CoordinatorPollingBasicAuthorizerCacheManager.java index 3d192e89d0db..0eedd5eda590 100644 --- a/extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/authorization/db/cache/CoordinatorPollingBasicAuthorizerCacheManager.java +++ b/extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/authorization/db/cache/CoordinatorPollingBasicAuthorizerCacheManager.java @@ -21,6 +21,7 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.base.Preconditions; +import com.google.common.collect.Sets; import com.google.inject.Inject; import com.google.inject.Injector; import org.apache.druid.client.coordinator.Coordinator; @@ -59,6 +60,7 @@ import java.io.IOException; import java.util.HashSet; import java.util.Map; +import java.util.Optional; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ScheduledExecutorService; @@ -417,7 +419,8 @@ private GroupMappingAndRoleMap tryFetchGroupMappingMapsFromCoordinator( ); BytesFullResponseHolder responseHolder = druidLeaderClient.go( req, - new BytesFullResponseHandler() + new BytesFullResponseHandler(), + Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)) ); final HttpResponseStatus status = responseHolder.getStatus(); diff --git a/extensions-core/druid-catalog/src/main/java/org/apache/druid/catalog/sync/CatalogClient.java b/extensions-core/druid-catalog/src/main/java/org/apache/druid/catalog/sync/CatalogClient.java index a06addfce710..5a1980a10617 100644 --- a/extensions-core/druid-catalog/src/main/java/org/apache/druid/catalog/sync/CatalogClient.java +++ b/extensions-core/druid-catalog/src/main/java/org/apache/druid/catalog/sync/CatalogClient.java @@ -21,6 +21,7 @@ import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.ObjectMapper; +import com.google.common.collect.Sets; import org.apache.druid.catalog.http.CatalogResource; import org.apache.druid.catalog.model.ResolvedTable; import org.apache.druid.catalog.model.TableDefnRegistry; @@ -45,6 +46,7 @@ import java.io.IOException; import java.util.Collections; import java.util.List; +import java.util.Optional; /** * Guice-injected client for the catalog update sync process. Requests @@ -127,7 +129,7 @@ private T send(String url, TypeReference typeRef) } final StringFullResponseHolder responseHolder; try { - responseHolder = coordClient.go(request); + responseHolder = coordClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND))); } catch (IOException e) { throw new ISE(e, "Failed to send catalog sync"); diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/worker/WorkerTaskManager.java b/indexing-service/src/main/java/org/apache/druid/indexing/worker/WorkerTaskManager.java index 0267ccad33b0..1a1c6a354012 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/worker/WorkerTaskManager.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/worker/WorkerTaskManager.java @@ -25,6 +25,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Lists; +import com.google.common.collect.Sets; import com.google.common.util.concurrent.FutureCallback; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; @@ -52,6 +53,7 @@ import org.apache.druid.server.coordination.ChangeRequestsSnapshot; import org.jboss.netty.handler.codec.http.HttpHeaders; import org.jboss.netty.handler.codec.http.HttpMethod; +import org.jboss.netty.handler.codec.http.HttpResponseStatus; import javax.ws.rs.core.MediaType; import java.io.File; @@ -61,6 +63,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Optional; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; import java.util.concurrent.ExecutorService; @@ -509,7 +512,8 @@ private void scheduleCompletedTasksCleanup() overlordClient.makeRequest(HttpMethod.POST, "/druid/indexer/v1/taskStatus") .setContent(jsonMapper.writeValueAsBytes(taskIds)) .addHeader(HttpHeaders.Names.ACCEPT, MediaType.APPLICATION_JSON) - .addHeader(HttpHeaders.Names.CONTENT_TYPE, MediaType.APPLICATION_JSON) + .addHeader(HttpHeaders.Names.CONTENT_TYPE, MediaType.APPLICATION_JSON), + Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)) ); if (fullResponseHolder.getStatus().getCode() == 200) { diff --git a/server/src/main/java/org/apache/druid/client/coordinator/CoordinatorClient.java b/server/src/main/java/org/apache/druid/client/coordinator/CoordinatorClient.java index 029663ae8215..b29c3898024f 100644 --- a/server/src/main/java/org/apache/druid/client/coordinator/CoordinatorClient.java +++ b/server/src/main/java/org/apache/druid/client/coordinator/CoordinatorClient.java @@ -21,6 +21,7 @@ import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.ObjectMapper; +import com.google.common.collect.Sets; import com.google.inject.Inject; import org.apache.druid.client.ImmutableSegmentLoadInfo; import org.apache.druid.discovery.DruidLeaderClient; @@ -37,6 +38,7 @@ import javax.ws.rs.core.MediaType; import java.util.Collection; import java.util.List; +import java.util.Optional; public class CoordinatorClient { @@ -71,7 +73,8 @@ public Boolean isHandOffComplete(String dataSource, SegmentDescriptor descriptor descriptor.getPartitionNumber(), descriptor.getVersion() ) - ) + ), + Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)) ); if (response.getStatus().equals(HttpResponseStatus.NOT_FOUND)) { diff --git a/server/src/main/java/org/apache/druid/client/indexing/HttpIndexingServiceClient.java b/server/src/main/java/org/apache/druid/client/indexing/HttpIndexingServiceClient.java index 821e4343901f..9ec016ac574c 100644 --- a/server/src/main/java/org/apache/druid/client/indexing/HttpIndexingServiceClient.java +++ b/server/src/main/java/org/apache/druid/client/indexing/HttpIndexingServiceClient.java @@ -25,6 +25,7 @@ import com.google.common.base.Strings; import com.google.common.collect.Iterables; import com.google.inject.Inject; +import org.apache.commons.compress.utils.Sets; import org.apache.druid.common.utils.IdUtils; import org.apache.druid.discovery.DruidLeaderClient; import org.apache.druid.indexer.TaskStatusPlus; @@ -52,6 +53,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Set; public class HttpIndexingServiceClient implements IndexingServiceClient @@ -362,7 +364,8 @@ public Map getTaskReport(String taskId) druidLeaderClient.makeRequest( HttpMethod.GET, StringUtils.format("/druid/indexer/v1/task/%s/reports", StringUtils.urlEncode(taskId)) - ) + ), + Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)) ); if (responseHolder.getContent().length() == 0 || !HttpResponseStatus.OK.equals(responseHolder.getStatus())) { diff --git a/server/src/main/java/org/apache/druid/discovery/DruidLeaderClient.java b/server/src/main/java/org/apache/druid/discovery/DruidLeaderClient.java index 0679ac39e4ac..74cc21fb5c55 100644 --- a/server/src/main/java/org/apache/druid/discovery/DruidLeaderClient.java +++ b/server/src/main/java/org/apache/druid/discovery/DruidLeaderClient.java @@ -45,6 +45,8 @@ import java.net.URL; import java.nio.charset.StandardCharsets; import java.util.Iterator; +import java.util.Optional; +import java.util.Set; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; @@ -122,19 +124,75 @@ public Request makeRequest(HttpMethod httpMethod, String urlPath) throws IOExcep return new Request(httpMethod, new URL(StringUtils.format("%s%s", getCurrentKnownLeader(true), urlPath))); } + /** + * Executes a Request object aimed at the leader. Throws IOException if the leader cannot be located. + * Internal retries are attempted if non 200 response is received. If the caller expects and wants to handle a response + * other than 200, they should use {@link #go(Request, Optional)} , which will avoid internal retries for the desired + * response codes + * + * @param request + * @throws IOException + * @throws InterruptedException + */ public StringFullResponseHolder go(Request request) throws IOException, InterruptedException { - return go(request, new StringFullResponseHandler(StandardCharsets.UTF_8)); + return go(request, new StringFullResponseHandler(StandardCharsets.UTF_8), Optional.empty()); + } + + /** + * * + * + * @param request + * @param acceptableResponses A set of HttpResponseStatuses, in addition to 200(OK), which the caller will handle. + * If a request returns any of these codes, the result is passed to the client. If a request + * returns a response other than the ones in acceptableResponses, attempts are made to relocate + * the leader and get an acceptable response. Reattempts are best-effort. If these reattempts still + * result in an unacceptable response, the response is returned to the client + * If empty, only 200 is considered as acceptable + * @throws IOException + * @throws InterruptedException + */ + public StringFullResponseHolder go(Request request, Optional> acceptableResponses) + throws IOException, InterruptedException + { + return go(request, new StringFullResponseHandler(StandardCharsets.UTF_8), acceptableResponses); } /** * Executes a Request object aimed at the leader. Throws IOException if the leader cannot be located. + * Internal retries are attempted if non 200 response is received. If the caller expects and wants to handle a response + * other than 200, they should use {@link #go(Request, HttpResponseHandler, Optional)} , which will avoid internal retries for the desired + * response codes */ public > H go(Request request, HttpResponseHandler responseHandler) throws IOException, InterruptedException + { + return go(request, responseHandler, Optional.empty()); + } + + /** + * Executes a Request object aimed at the leader. Throws IOException if the leader cannot be located. + * + * @param request + * @param responseHandler + * @param acceptableResponses A set of HttpResponseStatuses, in addition to 200(OK), which the caller will handle. + * If a request returns any of these codes, the result is passed to the client. If a request + * returns a response other than the ones in acceptableResponses, attempts are made to relocate + * the leader and get an acceptable response. Reattempts are best-effort. If these reattempts still + * result in an unacceptable response, the response is returned to the client + * If empty, only 200 is considered as acceptable + * @throws IOException + * @throws InterruptedException + */ + public > H go( + Request request, + HttpResponseHandler responseHandler, + Optional> acceptableResponses + ) + throws IOException, InterruptedException { Preconditions.checkState(lifecycleLock.awaitStarted(1, TimeUnit.MILLISECONDS)); - for (int counter = 0; counter < MAX_RETRIES; counter++) { + for (int counter = 1; counter <= MAX_RETRIES; counter++) { final H fullResponseHolder; @@ -152,35 +210,8 @@ public > H go(Request request, HttpResponseHa catch (IOException | ChannelException ex) { // can happen if the node is stopped. log.warn(ex, "Request[%s] failed.", request.getUrl()); - - try { - if (request.getUrl().getQuery() == null) { - request = withUrl( - request, - new URL(StringUtils.format("%s%s", getCurrentKnownLeader(false), request.getUrl().getPath())) - ); - } else { - request = withUrl( - request, - new URL(StringUtils.format( - "%s%s?%s", - getCurrentKnownLeader(false), - request.getUrl().getPath(), - request.getUrl().getQuery() - )) - ); - } - continue; - } - catch (MalformedURLException e) { - // Not an IOException; this is our own fault. - throw new ISE( - e, - "failed to build url with path[%] and query string [%s].", - request.getUrl().getPath(), - request.getUrl().getQuery() - ); - } + request = getNewRequestUrlInvalidatingCache(request); + continue; } if (HttpResponseStatus.TEMPORARY_REDIRECT.equals(fullResponseHolder.getResponse().getStatus())) { @@ -213,8 +244,13 @@ public > H go(Request request, HttpResponseHa )); request = withUrl(request, redirectUrl); - } else { + } else if (isAcceptableResponse(fullResponseHolder.getResponse().getStatus(), acceptableResponses) + || (counter == MAX_RETRIES)) { return fullResponseHolder; + } else { + // We don't have an acceptable response and still have retries left. Retry by invalidating the cache and relocating + // the leader + request = getNewRequestUrlInvalidatingCache(request); } } @@ -295,4 +331,49 @@ private Request withUrl(Request old, URL url) } return req; } + + private Request getNewRequestUrlInvalidatingCache(Request oldRequest) throws IOException + { + try { + Request newRequest; + if (oldRequest.getUrl().getQuery() == null) { + newRequest = withUrl( + oldRequest, + new URL(StringUtils.format("%s%s", getCurrentKnownLeader(false), oldRequest.getUrl().getPath())) + ); + } else { + newRequest = withUrl( + oldRequest, + new URL(StringUtils.format( + "%s%s?%s", + getCurrentKnownLeader(false), + oldRequest.getUrl().getPath(), + oldRequest.getUrl().getQuery() + )) + ); + } + return newRequest; + } + catch (MalformedURLException e) { + // Not an IOException; this is our own fault. + throw new ISE( + e, + "failed to build url with path[%] and query string [%s].", + oldRequest.getUrl().getPath(), + oldRequest.getUrl().getQuery() + ); + } + } + + private boolean isAcceptableResponse( + HttpResponseStatus requestStatus, + Optional> acceptableResponses + ) + { + if (HttpResponseStatus.OK.equals(requestStatus) || + acceptableResponses.isPresent() && acceptableResponses.get().contains(requestStatus)) { + return true; + } + return false; + } } diff --git a/server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java b/server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java index 792c52f00320..e6984136435d 100644 --- a/server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java +++ b/server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java @@ -27,6 +27,7 @@ import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Sets; import com.google.inject.Inject; import org.apache.commons.lang.mutable.MutableBoolean; import org.apache.druid.client.coordinator.Coordinator; @@ -588,7 +589,8 @@ private StringFullResponseHolder fetchLookupsForTier(String tier) throws Interru druidLeaderClient.makeRequest( HttpMethod.GET, StringUtils.format("/druid/coordinator/v1/lookups/config/%s?detailed=true", tier) - ) + ), + Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)) ); } diff --git a/server/src/test/java/org/apache/druid/client/indexing/HttpIndexingServiceClientTest.java b/server/src/test/java/org/apache/druid/client/indexing/HttpIndexingServiceClientTest.java index f669b4a8d872..c10d45c4288e 100644 --- a/server/src/test/java/org/apache/druid/client/indexing/HttpIndexingServiceClientTest.java +++ b/server/src/test/java/org/apache/druid/client/indexing/HttpIndexingServiceClientTest.java @@ -52,6 +52,7 @@ import java.net.URL; import java.nio.charset.StandardCharsets; import java.util.Map; +import java.util.Optional; public class HttpIndexingServiceClientTest { @@ -212,7 +213,7 @@ public void testGetTaskReport() throws Exception StandardCharsets.UTF_8 ).addChunk(jsonMapper.writeValueAsString(dummyResponse)); - EasyMock.expect(druidLeaderClient.go(EasyMock.anyObject(Request.class))) + EasyMock.expect(druidLeaderClient.go(EasyMock.anyObject(Request.class), EasyMock.anyObject(Optional.class))) .andReturn(responseHolder) .anyTimes(); @@ -249,7 +250,7 @@ public void testGetTaskReportStatusNotFound() throws Exception StandardCharsets.UTF_8 ).addChunk(""); - EasyMock.expect(druidLeaderClient.go(EasyMock.anyObject(Request.class))) + EasyMock.expect(druidLeaderClient.go(EasyMock.anyObject(Request.class), EasyMock.anyObject(Optional.class))) .andReturn(responseHolder) .anyTimes(); @@ -281,7 +282,7 @@ public void testGetTaskReportEmpty() throws Exception StandardCharsets.UTF_8 ).addChunk(""); - EasyMock.expect(druidLeaderClient.go(EasyMock.anyObject(Request.class))) + EasyMock.expect(druidLeaderClient.go(EasyMock.anyObject(Request.class), EasyMock.anyObject(Optional.class))) .andReturn(responseHolder) .anyTimes(); diff --git a/server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java b/server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java index 570720db48ff..cbad9fc50864 100644 --- a/server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java +++ b/server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java @@ -21,6 +21,8 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Sets; +import com.google.common.util.concurrent.ListenableFutureTask; import com.google.inject.Binder; import com.google.inject.Inject; import com.google.inject.Injector; @@ -39,9 +41,11 @@ import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.java.util.http.client.HttpClient; import org.apache.druid.java.util.http.client.Request; +import org.apache.druid.java.util.http.client.response.StringFullResponseHolder; import org.apache.druid.server.DruidNode; import org.apache.druid.server.initialization.BaseJettyTest; import org.apache.druid.server.initialization.jetty.JettyServerInitializer; +import org.asynchttpclient.ListenableFuture; import org.easymock.EasyMock; import org.eclipse.jetty.server.Handler; import org.eclipse.jetty.server.Server; @@ -49,11 +53,15 @@ import org.eclipse.jetty.servlet.DefaultServlet; import org.eclipse.jetty.servlet.ServletContextHandler; import org.eclipse.jetty.servlet.ServletHolder; +import org.jboss.netty.handler.codec.http.DefaultHttpResponse; import org.jboss.netty.handler.codec.http.HttpMethod; +import org.jboss.netty.handler.codec.http.HttpResponseStatus; +import org.jboss.netty.handler.codec.http.HttpVersion; import org.junit.Assert; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; +import org.mockito.Mockito; import javax.ws.rs.GET; import javax.ws.rs.POST; @@ -63,7 +71,10 @@ import javax.ws.rs.core.Response; import java.io.IOException; import java.net.URI; +import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; +import java.util.Optional; +import java.util.Set; /** */ @@ -211,6 +222,81 @@ public void testServerFailureAndRedirect() throws Exception Assert.assertEquals("hello", druidLeaderClient.go(request).getContent()); } + @Test + public void testNon200ResponseFromServerAndCacheRefresh() throws Exception + { + DruidNodeDiscovery druidNodeDiscovery = EasyMock.createMock(DruidNodeDiscovery.class); + // Should be called twice. Second time is when we refresh the cache since we get 503 in the first request + EasyMock.expect(druidNodeDiscovery.getAllNodes()).andReturn(ImmutableList.of(discoveryDruidNode)).times(2); + + DruidNodeDiscoveryProvider druidNodeDiscoveryProvider = EasyMock.createMock(DruidNodeDiscoveryProvider.class); + EasyMock.expect(druidNodeDiscoveryProvider.getForNodeRole(NodeRole.PEON)).andReturn(druidNodeDiscovery).anyTimes(); + + ListenableFutureTask task = EasyMock.createMock(ListenableFutureTask.class); + EasyMock.expect(task.get()).andReturn(new StringFullResponseHolder(new DefaultHttpResponse( + HttpVersion.HTTP_1_1, + HttpResponseStatus.SERVICE_UNAVAILABLE + ), Charset.defaultCharset())); + EasyMock.replay(druidNodeDiscovery, druidNodeDiscoveryProvider, task); + + HttpClient httpClient = Mockito.spy(this.httpClient); + // Override behavior for the first call only + Mockito.doReturn(task).doCallRealMethod().when(httpClient).go(Mockito.any(), Mockito.any()); + + DruidLeaderClient druidLeaderClient = new DruidLeaderClient( + httpClient, + druidNodeDiscoveryProvider, + NodeRole.PEON, + "/simple/leader" + ); + druidLeaderClient.start(); + + Request request = druidLeaderClient.makeRequest(HttpMethod.POST, "/simple/direct"); + request.setContent("hello".getBytes(StandardCharsets.UTF_8)); + Assert.assertEquals("hello", druidLeaderClient.go(request).getContent()); + EasyMock.verify(druidNodeDiscovery); + } + + @Test + public void testNon200ResponseFromServerNoCacheRefresh() throws Exception + { + DruidNodeDiscovery druidNodeDiscovery = EasyMock.createMock(DruidNodeDiscovery.class); + // Should be called only once, since we want to handle 503 ourselves and don't want cache refresh + EasyMock.expect(druidNodeDiscovery.getAllNodes()).andReturn(ImmutableList.of(discoveryDruidNode)).times(1); + + DruidNodeDiscoveryProvider druidNodeDiscoveryProvider = EasyMock.createMock(DruidNodeDiscoveryProvider.class); + EasyMock.expect(druidNodeDiscoveryProvider.getForNodeRole(NodeRole.PEON)).andReturn(druidNodeDiscovery).anyTimes(); + + ListenableFutureTask task = EasyMock.createMock(ListenableFutureTask.class); + EasyMock.expect(task.get()).andReturn(new StringFullResponseHolder(new DefaultHttpResponse( + HttpVersion.HTTP_1_1, + HttpResponseStatus.SERVICE_UNAVAILABLE + ), Charset.defaultCharset())); + EasyMock.replay(druidNodeDiscovery, druidNodeDiscoveryProvider, task); + + HttpClient httpClient = Mockito.spy(this.httpClient); + Mockito.doReturn(task).doCallRealMethod().when(httpClient).go(Mockito.any(), Mockito.any()); + + DruidLeaderClient druidLeaderClient = new DruidLeaderClient( + httpClient, + druidNodeDiscoveryProvider, + NodeRole.PEON, + "/simple/leader" + ); + druidLeaderClient.start(); + + Request request = druidLeaderClient.makeRequest(HttpMethod.POST, "/simple/direct"); + request.setContent("hello".getBytes(StandardCharsets.UTF_8)); + Assert.assertEquals( + HttpResponseStatus.SERVICE_UNAVAILABLE, + druidLeaderClient.go( + request, + Optional.of(Sets.newHashSet(HttpResponseStatus.SERVICE_UNAVAILABLE)) + ).getResponse().getStatus() + ); + EasyMock.verify(druidNodeDiscovery); + } + @Test public void testFindCurrentLeader() { diff --git a/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java b/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java index 8cc6956d117f..4ec513c5d5d1 100644 --- a/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java +++ b/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java @@ -22,6 +22,7 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Sets; import org.apache.druid.discovery.DruidLeaderClient; import org.apache.druid.jackson.DefaultObjectMapper; import org.apache.druid.java.util.emitter.EmittingLogger; @@ -110,15 +111,16 @@ public void testStartStop() throws InterruptedException, IOException EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); EasyMock.replay(config); EasyMock.expect(druidLeaderClient.makeRequest( - HttpMethod.GET, - "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" - )) + HttpMethod.GET, + "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" + )) .andReturn(request); StringFullResponseHolder responseHolder = new StringFullResponseHolder( newEmptyResponse(HttpResponseStatus.OK), StandardCharsets.UTF_8 ).addChunk(strResult); - EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); + EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) + .andReturn(responseHolder); EasyMock.replay(druidLeaderClient); Assert.assertFalse(lookupReferencesManager.lifecycleLock.awaitStarted(1, TimeUnit.MICROSECONDS)); Assert.assertNull(lookupReferencesManager.mainThread); @@ -173,15 +175,16 @@ public void testAddGetRemove() throws Exception EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); EasyMock.replay(config); EasyMock.expect(druidLeaderClient.makeRequest( - HttpMethod.GET, - "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" - )) + HttpMethod.GET, + "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" + )) .andReturn(request); StringFullResponseHolder responseHolder = new StringFullResponseHolder( newEmptyResponse(HttpResponseStatus.OK), StandardCharsets.UTF_8 ).addChunk(strResult); - EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); + EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) + .andReturn(responseHolder); EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); Assert.assertEquals(Optional.empty(), lookupReferencesManager.get("test")); @@ -213,15 +216,16 @@ public void testCloseIsCalledAfterStopping() throws Exception EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); EasyMock.replay(config); EasyMock.expect(druidLeaderClient.makeRequest( - HttpMethod.GET, - "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" - )) + HttpMethod.GET, + "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" + )) .andReturn(request); StringFullResponseHolder responseHolder = new StringFullResponseHolder( newEmptyResponse(HttpResponseStatus.OK), StandardCharsets.UTF_8 ).addChunk(strResult); - EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); + EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) + .andReturn(responseHolder); EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); lookupReferencesManager.add("testMock", new LookupExtractorFactoryContainer("0", lookupExtractorFactory)); @@ -246,15 +250,16 @@ public void testDestroyIsCalledAfterRemove() throws Exception EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); EasyMock.replay(config); EasyMock.expect(druidLeaderClient.makeRequest( - HttpMethod.GET, - "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" - )) + HttpMethod.GET, + "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" + )) .andReturn(request); StringFullResponseHolder responseHolder = new StringFullResponseHolder( newEmptyResponse(HttpResponseStatus.OK), StandardCharsets.UTF_8 ).addChunk(strResult); - EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); + EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) + .andReturn(responseHolder); EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); lookupReferencesManager.add("testMock", new LookupExtractorFactoryContainer("0", lookupExtractorFactory)); @@ -276,15 +281,16 @@ public void testGetNotThere() throws Exception EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); EasyMock.replay(config); EasyMock.expect(druidLeaderClient.makeRequest( - HttpMethod.GET, - "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" - )) + HttpMethod.GET, + "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" + )) .andReturn(request); StringFullResponseHolder responseHolder = new StringFullResponseHolder( newEmptyResponse(HttpResponseStatus.OK), StandardCharsets.UTF_8 ).addChunk(strResult); - EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); + EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) + .andReturn(responseHolder); EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); Assert.assertEquals(Optional.empty(), lookupReferencesManager.get("notThere")); @@ -308,15 +314,16 @@ public void testUpdateWithHigherVersion() throws Exception EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); EasyMock.replay(config); EasyMock.expect(druidLeaderClient.makeRequest( - HttpMethod.GET, - "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" - )) + HttpMethod.GET, + "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" + )) .andReturn(request); StringFullResponseHolder responseHolder = new StringFullResponseHolder( newEmptyResponse(HttpResponseStatus.OK), StandardCharsets.UTF_8 ).addChunk(strResult); - EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); + EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) + .andReturn(responseHolder); EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); lookupReferencesManager.add("testName", new LookupExtractorFactoryContainer("1", lookupExtractorFactory1)); @@ -344,15 +351,16 @@ public void testUpdateWithLowerVersion() throws Exception EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); EasyMock.replay(config); EasyMock.expect(druidLeaderClient.makeRequest( - HttpMethod.GET, - "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" - )) + HttpMethod.GET, + "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" + )) .andReturn(request); StringFullResponseHolder responseHolder = new StringFullResponseHolder( newEmptyResponse(HttpResponseStatus.OK), StandardCharsets.UTF_8 ).addChunk(strResult); - EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); + EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) + .andReturn(responseHolder); EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); lookupReferencesManager.add("testName", new LookupExtractorFactoryContainer("1", lookupExtractorFactory1)); @@ -374,15 +382,16 @@ public void testRemoveNonExisting() throws Exception EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); EasyMock.replay(config); EasyMock.expect(druidLeaderClient.makeRequest( - HttpMethod.GET, - "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" - )) + HttpMethod.GET, + "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" + )) .andReturn(request); StringFullResponseHolder responseHolder = new StringFullResponseHolder( newEmptyResponse(HttpResponseStatus.OK), StandardCharsets.UTF_8 ).addChunk(strResult); - EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); + EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) + .andReturn(responseHolder); EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); lookupReferencesManager.remove("test"); @@ -413,7 +422,8 @@ public void testGetAllLookupNames() throws Exception newEmptyResponse(HttpResponseStatus.OK), StandardCharsets.UTF_8 ).addChunk(strResult); - EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); + EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) + .andReturn(responseHolder); EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); lookupReferencesManager.add("one", container1); @@ -466,15 +476,16 @@ public void testGetAllLookupsState() throws Exception EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); EasyMock.replay(config); EasyMock.expect(druidLeaderClient.makeRequest( - HttpMethod.GET, - "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" - )) + HttpMethod.GET, + "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" + )) .andReturn(request); StringFullResponseHolder responseHolder = new StringFullResponseHolder( newEmptyResponse(HttpResponseStatus.OK), StandardCharsets.UTF_8 ).addChunk(strResult); - EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); + EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) + .andReturn(responseHolder); EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); lookupReferencesManager.add("one", container1); @@ -510,15 +521,16 @@ public void testRealModeWithMainThread() throws Exception EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); EasyMock.replay(config); EasyMock.expect(druidLeaderClient.makeRequest( - HttpMethod.GET, - "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" - )) + HttpMethod.GET, + "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" + )) .andReturn(request); StringFullResponseHolder responseHolder = new StringFullResponseHolder( newEmptyResponse(HttpResponseStatus.OK), StandardCharsets.UTF_8 ).addChunk(strResult); - EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); + EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) + .andReturn(responseHolder); EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); Assert.assertTrue(lookupReferencesManager.mainThread.isAlive()); @@ -598,15 +610,16 @@ public void testCoordinatorLookupSync() throws Exception EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); EasyMock.replay(config); EasyMock.expect(druidLeaderClient.makeRequest( - HttpMethod.GET, - "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" - )) + HttpMethod.GET, + "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" + )) .andReturn(request); StringFullResponseHolder responseHolder = new StringFullResponseHolder( newEmptyResponse(HttpResponseStatus.OK), StandardCharsets.UTF_8 ).addChunk(strResult); - EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); + EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) + .andReturn(responseHolder); EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); @@ -638,12 +651,14 @@ public int getCoordinatorRetryDelay() EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); EasyMock.replay(config); EasyMock.expect(druidLeaderClient.makeRequest( - HttpMethod.GET, - "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" - )) + HttpMethod.GET, + "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" + )) .andReturn(request) .anyTimes(); - EasyMock.expect(druidLeaderClient.go(request)).andThrow(new IllegalStateException()).anyTimes(); + EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) + .andThrow(new IllegalStateException()) + .anyTimes(); EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); @@ -671,12 +686,14 @@ public int getCoordinatorRetryDelay() EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); EasyMock.replay(config); EasyMock.expect(druidLeaderClient.makeRequest( - HttpMethod.GET, - "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" - )) + HttpMethod.GET, + "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" + )) .andReturn(request) .anyTimes(); - EasyMock.expect(druidLeaderClient.go(request)).andThrow(new IllegalStateException()).anyTimes(); + EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) + .andThrow(new IllegalStateException()) + .anyTimes(); EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); Assert.assertEquals( @@ -709,15 +726,16 @@ public boolean getEnableLookupSyncOnStartup() EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); EasyMock.replay(config); EasyMock.expect(druidLeaderClient.makeRequest( - HttpMethod.GET, - "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" - )) + HttpMethod.GET, + "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" + )) .andReturn(request); StringFullResponseHolder responseHolder = new StringFullResponseHolder( newEmptyResponse(HttpResponseStatus.OK), StandardCharsets.UTF_8 ).addChunk(strResult); - EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); + EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) + .andReturn(responseHolder); lookupReferencesManager.start(); Assert.assertEquals(Optional.empty(), lookupReferencesManager.get("testMockForDisableLookupSync")); diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/schema/SystemSchemaTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/schema/SystemSchemaTest.java index 50d0f09a85e5..715a40c4dad9 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/schema/SystemSchemaTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/schema/SystemSchemaTest.java @@ -1152,7 +1152,9 @@ public void testTasksTable() throws Exception HttpResponse httpResp = new DefaultHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.OK); InputStreamFullResponseHolder responseHolder = new InputStreamFullResponseHolder(httpResp); - EasyMock.expect(client.go(EasyMock.eq(request), EasyMock.anyObject(InputStreamFullResponseHandler.class))).andReturn(responseHolder).once(); + EasyMock.expect(client.go(EasyMock.eq(request), EasyMock.anyObject(InputStreamFullResponseHandler.class))) + .andReturn(responseHolder) + .once(); EasyMock.expect(request.getUrl()).andReturn(new URL("http://test-host:1234/druid/indexer/v1/tasks")).anyTimes(); String json = "[{\n" @@ -1277,7 +1279,7 @@ public void testTasksTableAuth() throws Exception HttpResponse httpResp = new DefaultHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.OK); - EasyMock.expect(client.go(EasyMock.eq(request), EasyMock.anyObject())) + EasyMock.expect(client.go(EasyMock.eq(request), EasyMock.anyObject(HttpResponseHandler.class))) .andReturn(createFullResponseHolder(httpResp, json)) .andReturn(createFullResponseHolder(httpResp, json)) .andReturn(createFullResponseHolder(httpResp, json)); @@ -1326,7 +1328,9 @@ public void testSupervisorTable() throws Exception HttpResponse httpResp = new DefaultHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.OK); InputStreamFullResponseHolder responseHolder = new InputStreamFullResponseHolder(httpResp); - EasyMock.expect(client.go(EasyMock.eq(request), EasyMock.anyObject(InputStreamFullResponseHandler.class))).andReturn(responseHolder).once(); + EasyMock.expect(client.go(EasyMock.eq(request), EasyMock.anyObject(InputStreamFullResponseHandler.class))) + .andReturn(responseHolder) + .once(); EasyMock.expect(responseHandler.getStatus()).andReturn(httpResp.getStatus().getCode()).anyTimes(); EasyMock.expect(request.getUrl()) @@ -1393,7 +1397,7 @@ public void testSupervisorTableAuth() throws Exception + "}]"; HttpResponse httpResponse = new DefaultHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.OK); - EasyMock.expect(client.go(EasyMock.eq(request), EasyMock.anyObject())) + EasyMock.expect(client.go(EasyMock.eq(request), EasyMock.anyObject(HttpResponseHandler.class))) .andReturn(createFullResponseHolder(httpResponse, json)) .andReturn(createFullResponseHolder(httpResponse, json)) .andReturn(createFullResponseHolder(httpResponse, json)); From cbbbecfd44ad8da9251c8164dfa271814f6877cf Mon Sep 17 00:00:00 2001 From: Abhishek Singh Chouhan Date: Fri, 14 Apr 2023 16:18:08 -0700 Subject: [PATCH 2/8] Change local variable name to avoid confusion --- .../druid/discovery/DruidLeaderClientTest.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java b/server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java index cbad9fc50864..5d52ed396315 100644 --- a/server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java +++ b/server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java @@ -239,12 +239,12 @@ public void testNon200ResponseFromServerAndCacheRefresh() throws Exception ), Charset.defaultCharset())); EasyMock.replay(druidNodeDiscovery, druidNodeDiscoveryProvider, task); - HttpClient httpClient = Mockito.spy(this.httpClient); + HttpClient spyHttpClient = Mockito.spy(this.httpClient); // Override behavior for the first call only - Mockito.doReturn(task).doCallRealMethod().when(httpClient).go(Mockito.any(), Mockito.any()); + Mockito.doReturn(task).doCallRealMethod().when(spyHttpClient).go(Mockito.any(), Mockito.any()); DruidLeaderClient druidLeaderClient = new DruidLeaderClient( - httpClient, + spyHttpClient, druidNodeDiscoveryProvider, NodeRole.PEON, "/simple/leader" @@ -274,11 +274,11 @@ public void testNon200ResponseFromServerNoCacheRefresh() throws Exception ), Charset.defaultCharset())); EasyMock.replay(druidNodeDiscovery, druidNodeDiscoveryProvider, task); - HttpClient httpClient = Mockito.spy(this.httpClient); - Mockito.doReturn(task).doCallRealMethod().when(httpClient).go(Mockito.any(), Mockito.any()); + HttpClient spyHttpClient = Mockito.spy(this.httpClient); + Mockito.doReturn(task).doCallRealMethod().when(spyHttpClient).go(Mockito.any(), Mockito.any()); DruidLeaderClient druidLeaderClient = new DruidLeaderClient( - httpClient, + spyHttpClient, druidNodeDiscoveryProvider, NodeRole.PEON, "/simple/leader" From ce73e521e841f5e2cf2a6901420cfed235ecb36c Mon Sep 17 00:00:00 2001 From: Abhishek Singh Chouhan Date: Tue, 18 Apr 2023 18:10:16 -0700 Subject: [PATCH 3/8] Implicit retries for 503 and 504 --- ...torPollingBasicAuthorizerCacheManager.java | 5 +- .../druid/catalog/sync/CatalogClient.java | 4 +- .../indexing/worker/WorkerTaskManager.java | 6 +- .../client/coordinator/CoordinatorClient.java | 5 +- .../indexing/HttpIndexingServiceClient.java | 5 +- .../druid/discovery/DruidLeaderClient.java | 72 ++-------- .../query/lookup/LookupReferencesManager.java | 4 +- .../HttpIndexingServiceClientTest.java | 7 +- .../discovery/DruidLeaderClientTest.java | 8 +- .../lookup/LookupReferencesManagerTest.java | 132 ++++++++---------- .../sql/calcite/schema/SystemSchemaTest.java | 12 +- 11 files changed, 90 insertions(+), 170 deletions(-) diff --git a/extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/authorization/db/cache/CoordinatorPollingBasicAuthorizerCacheManager.java b/extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/authorization/db/cache/CoordinatorPollingBasicAuthorizerCacheManager.java index 0eedd5eda590..3d192e89d0db 100644 --- a/extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/authorization/db/cache/CoordinatorPollingBasicAuthorizerCacheManager.java +++ b/extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/authorization/db/cache/CoordinatorPollingBasicAuthorizerCacheManager.java @@ -21,7 +21,6 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.base.Preconditions; -import com.google.common.collect.Sets; import com.google.inject.Inject; import com.google.inject.Injector; import org.apache.druid.client.coordinator.Coordinator; @@ -60,7 +59,6 @@ import java.io.IOException; import java.util.HashSet; import java.util.Map; -import java.util.Optional; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ScheduledExecutorService; @@ -419,8 +417,7 @@ private GroupMappingAndRoleMap tryFetchGroupMappingMapsFromCoordinator( ); BytesFullResponseHolder responseHolder = druidLeaderClient.go( req, - new BytesFullResponseHandler(), - Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)) + new BytesFullResponseHandler() ); final HttpResponseStatus status = responseHolder.getStatus(); diff --git a/extensions-core/druid-catalog/src/main/java/org/apache/druid/catalog/sync/CatalogClient.java b/extensions-core/druid-catalog/src/main/java/org/apache/druid/catalog/sync/CatalogClient.java index 5a1980a10617..a06addfce710 100644 --- a/extensions-core/druid-catalog/src/main/java/org/apache/druid/catalog/sync/CatalogClient.java +++ b/extensions-core/druid-catalog/src/main/java/org/apache/druid/catalog/sync/CatalogClient.java @@ -21,7 +21,6 @@ import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.ObjectMapper; -import com.google.common.collect.Sets; import org.apache.druid.catalog.http.CatalogResource; import org.apache.druid.catalog.model.ResolvedTable; import org.apache.druid.catalog.model.TableDefnRegistry; @@ -46,7 +45,6 @@ import java.io.IOException; import java.util.Collections; import java.util.List; -import java.util.Optional; /** * Guice-injected client for the catalog update sync process. Requests @@ -129,7 +127,7 @@ private T send(String url, TypeReference typeRef) } final StringFullResponseHolder responseHolder; try { - responseHolder = coordClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND))); + responseHolder = coordClient.go(request); } catch (IOException e) { throw new ISE(e, "Failed to send catalog sync"); diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/worker/WorkerTaskManager.java b/indexing-service/src/main/java/org/apache/druid/indexing/worker/WorkerTaskManager.java index 1a1c6a354012..0267ccad33b0 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/worker/WorkerTaskManager.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/worker/WorkerTaskManager.java @@ -25,7 +25,6 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Lists; -import com.google.common.collect.Sets; import com.google.common.util.concurrent.FutureCallback; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; @@ -53,7 +52,6 @@ import org.apache.druid.server.coordination.ChangeRequestsSnapshot; import org.jboss.netty.handler.codec.http.HttpHeaders; import org.jboss.netty.handler.codec.http.HttpMethod; -import org.jboss.netty.handler.codec.http.HttpResponseStatus; import javax.ws.rs.core.MediaType; import java.io.File; @@ -63,7 +61,6 @@ import java.util.List; import java.util.Map; import java.util.Objects; -import java.util.Optional; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; import java.util.concurrent.ExecutorService; @@ -512,8 +509,7 @@ private void scheduleCompletedTasksCleanup() overlordClient.makeRequest(HttpMethod.POST, "/druid/indexer/v1/taskStatus") .setContent(jsonMapper.writeValueAsBytes(taskIds)) .addHeader(HttpHeaders.Names.ACCEPT, MediaType.APPLICATION_JSON) - .addHeader(HttpHeaders.Names.CONTENT_TYPE, MediaType.APPLICATION_JSON), - Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)) + .addHeader(HttpHeaders.Names.CONTENT_TYPE, MediaType.APPLICATION_JSON) ); if (fullResponseHolder.getStatus().getCode() == 200) { diff --git a/server/src/main/java/org/apache/druid/client/coordinator/CoordinatorClient.java b/server/src/main/java/org/apache/druid/client/coordinator/CoordinatorClient.java index b29c3898024f..029663ae8215 100644 --- a/server/src/main/java/org/apache/druid/client/coordinator/CoordinatorClient.java +++ b/server/src/main/java/org/apache/druid/client/coordinator/CoordinatorClient.java @@ -21,7 +21,6 @@ import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.ObjectMapper; -import com.google.common.collect.Sets; import com.google.inject.Inject; import org.apache.druid.client.ImmutableSegmentLoadInfo; import org.apache.druid.discovery.DruidLeaderClient; @@ -38,7 +37,6 @@ import javax.ws.rs.core.MediaType; import java.util.Collection; import java.util.List; -import java.util.Optional; public class CoordinatorClient { @@ -73,8 +71,7 @@ public Boolean isHandOffComplete(String dataSource, SegmentDescriptor descriptor descriptor.getPartitionNumber(), descriptor.getVersion() ) - ), - Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)) + ) ); if (response.getStatus().equals(HttpResponseStatus.NOT_FOUND)) { diff --git a/server/src/main/java/org/apache/druid/client/indexing/HttpIndexingServiceClient.java b/server/src/main/java/org/apache/druid/client/indexing/HttpIndexingServiceClient.java index 9ec016ac574c..821e4343901f 100644 --- a/server/src/main/java/org/apache/druid/client/indexing/HttpIndexingServiceClient.java +++ b/server/src/main/java/org/apache/druid/client/indexing/HttpIndexingServiceClient.java @@ -25,7 +25,6 @@ import com.google.common.base.Strings; import com.google.common.collect.Iterables; import com.google.inject.Inject; -import org.apache.commons.compress.utils.Sets; import org.apache.druid.common.utils.IdUtils; import org.apache.druid.discovery.DruidLeaderClient; import org.apache.druid.indexer.TaskStatusPlus; @@ -53,7 +52,6 @@ import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Optional; import java.util.Set; public class HttpIndexingServiceClient implements IndexingServiceClient @@ -364,8 +362,7 @@ public Map getTaskReport(String taskId) druidLeaderClient.makeRequest( HttpMethod.GET, StringUtils.format("/druid/indexer/v1/task/%s/reports", StringUtils.urlEncode(taskId)) - ), - Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)) + ) ); if (responseHolder.getContent().length() == 0 || !HttpResponseStatus.OK.equals(responseHolder.getStatus())) { diff --git a/server/src/main/java/org/apache/druid/discovery/DruidLeaderClient.java b/server/src/main/java/org/apache/druid/discovery/DruidLeaderClient.java index 74cc21fb5c55..241c1bc2259d 100644 --- a/server/src/main/java/org/apache/druid/discovery/DruidLeaderClient.java +++ b/server/src/main/java/org/apache/druid/discovery/DruidLeaderClient.java @@ -45,8 +45,6 @@ import java.net.URL; import java.nio.charset.StandardCharsets; import java.util.Iterator; -import java.util.Optional; -import java.util.Set; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; @@ -126,9 +124,8 @@ public Request makeRequest(HttpMethod httpMethod, String urlPath) throws IOExcep /** * Executes a Request object aimed at the leader. Throws IOException if the leader cannot be located. - * Internal retries are attempted if non 200 response is received. If the caller expects and wants to handle a response - * other than 200, they should use {@link #go(Request, Optional)} , which will avoid internal retries for the desired - * response codes + * Internal retries are attempted if 503/504 response is received. If the caller wants to override this + * retry behavior, they should use {@link #go(Request, HttpResponseHandler, boolean)} * * @param request * @throws IOException @@ -136,38 +133,18 @@ public Request makeRequest(HttpMethod httpMethod, String urlPath) throws IOExcep */ public StringFullResponseHolder go(Request request) throws IOException, InterruptedException { - return go(request, new StringFullResponseHandler(StandardCharsets.UTF_8), Optional.empty()); - } - - /** - * * - * - * @param request - * @param acceptableResponses A set of HttpResponseStatuses, in addition to 200(OK), which the caller will handle. - * If a request returns any of these codes, the result is passed to the client. If a request - * returns a response other than the ones in acceptableResponses, attempts are made to relocate - * the leader and get an acceptable response. Reattempts are best-effort. If these reattempts still - * result in an unacceptable response, the response is returned to the client - * If empty, only 200 is considered as acceptable - * @throws IOException - * @throws InterruptedException - */ - public StringFullResponseHolder go(Request request, Optional> acceptableResponses) - throws IOException, InterruptedException - { - return go(request, new StringFullResponseHandler(StandardCharsets.UTF_8), acceptableResponses); + return go(request, new StringFullResponseHandler(StandardCharsets.UTF_8), true); } /** * Executes a Request object aimed at the leader. Throws IOException if the leader cannot be located. - * Internal retries are attempted if non 200 response is received. If the caller expects and wants to handle a response - * other than 200, they should use {@link #go(Request, HttpResponseHandler, Optional)} , which will avoid internal retries for the desired - * response codes + * Internal retries are attempted if 503/504 response is received. If the caller wants to override this + * retry behavior, they should use {@link #go(Request, HttpResponseHandler, boolean)} */ public > H go(Request request, HttpResponseHandler responseHandler) throws IOException, InterruptedException { - return go(request, responseHandler, Optional.empty()); + return go(request, responseHandler, true); } /** @@ -175,24 +152,19 @@ public > H go(Request request, HttpResponseHa * * @param request * @param responseHandler - * @param acceptableResponses A set of HttpResponseStatuses, in addition to 200(OK), which the caller will handle. - * If a request returns any of these codes, the result is passed to the client. If a request - * returns a response other than the ones in acceptableResponses, attempts are made to relocate - * the leader and get an acceptable response. Reattempts are best-effort. If these reattempts still - * result in an unacceptable response, the response is returned to the client - * If empty, only 200 is considered as acceptable + * @param shouldRetry If internal retries with cache invalidation should be done for 503 and 504 responses * @throws IOException * @throws InterruptedException */ public > H go( Request request, HttpResponseHandler responseHandler, - Optional> acceptableResponses + boolean shouldRetry ) throws IOException, InterruptedException { Preconditions.checkState(lifecycleLock.awaitStarted(1, TimeUnit.MILLISECONDS)); - for (int counter = 1; counter <= MAX_RETRIES; counter++) { + for (int counter = 0; counter < MAX_RETRIES; counter++) { final H fullResponseHolder; @@ -213,8 +185,8 @@ public > H go( request = getNewRequestUrlInvalidatingCache(request); continue; } - - if (HttpResponseStatus.TEMPORARY_REDIRECT.equals(fullResponseHolder.getResponse().getStatus())) { + HttpResponseStatus responseStatus = fullResponseHolder.getResponse().getStatus(); + if (HttpResponseStatus.TEMPORARY_REDIRECT.equals(responseStatus)) { String redirectUrlStr = fullResponseHolder.getResponse().headers().get("Location"); if (redirectUrlStr == null) { throw new IOE("No redirect location is found in response from url[%s].", request.getUrl()); @@ -244,13 +216,11 @@ public > H go( )); request = withUrl(request, redirectUrl); - } else if (isAcceptableResponse(fullResponseHolder.getResponse().getStatus(), acceptableResponses) - || (counter == MAX_RETRIES)) { - return fullResponseHolder; - } else { - // We don't have an acceptable response and still have retries left. Retry by invalidating the cache and relocating - // the leader + } else if (shouldRetry && (HttpResponseStatus.SERVICE_UNAVAILABLE.equals(responseStatus) + || HttpResponseStatus.GATEWAY_TIMEOUT.equals(responseStatus))) { request = getNewRequestUrlInvalidatingCache(request); + } else { + return fullResponseHolder; } } @@ -364,16 +334,4 @@ private Request getNewRequestUrlInvalidatingCache(Request oldRequest) throws IOE ); } } - - private boolean isAcceptableResponse( - HttpResponseStatus requestStatus, - Optional> acceptableResponses - ) - { - if (HttpResponseStatus.OK.equals(requestStatus) || - acceptableResponses.isPresent() && acceptableResponses.get().contains(requestStatus)) { - return true; - } - return false; - } } diff --git a/server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java b/server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java index e6984136435d..792c52f00320 100644 --- a/server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java +++ b/server/src/main/java/org/apache/druid/query/lookup/LookupReferencesManager.java @@ -27,7 +27,6 @@ import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Sets; import com.google.inject.Inject; import org.apache.commons.lang.mutable.MutableBoolean; import org.apache.druid.client.coordinator.Coordinator; @@ -589,8 +588,7 @@ private StringFullResponseHolder fetchLookupsForTier(String tier) throws Interru druidLeaderClient.makeRequest( HttpMethod.GET, StringUtils.format("/druid/coordinator/v1/lookups/config/%s?detailed=true", tier) - ), - Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)) + ) ); } diff --git a/server/src/test/java/org/apache/druid/client/indexing/HttpIndexingServiceClientTest.java b/server/src/test/java/org/apache/druid/client/indexing/HttpIndexingServiceClientTest.java index c10d45c4288e..f669b4a8d872 100644 --- a/server/src/test/java/org/apache/druid/client/indexing/HttpIndexingServiceClientTest.java +++ b/server/src/test/java/org/apache/druid/client/indexing/HttpIndexingServiceClientTest.java @@ -52,7 +52,6 @@ import java.net.URL; import java.nio.charset.StandardCharsets; import java.util.Map; -import java.util.Optional; public class HttpIndexingServiceClientTest { @@ -213,7 +212,7 @@ public void testGetTaskReport() throws Exception StandardCharsets.UTF_8 ).addChunk(jsonMapper.writeValueAsString(dummyResponse)); - EasyMock.expect(druidLeaderClient.go(EasyMock.anyObject(Request.class), EasyMock.anyObject(Optional.class))) + EasyMock.expect(druidLeaderClient.go(EasyMock.anyObject(Request.class))) .andReturn(responseHolder) .anyTimes(); @@ -250,7 +249,7 @@ public void testGetTaskReportStatusNotFound() throws Exception StandardCharsets.UTF_8 ).addChunk(""); - EasyMock.expect(druidLeaderClient.go(EasyMock.anyObject(Request.class), EasyMock.anyObject(Optional.class))) + EasyMock.expect(druidLeaderClient.go(EasyMock.anyObject(Request.class))) .andReturn(responseHolder) .anyTimes(); @@ -282,7 +281,7 @@ public void testGetTaskReportEmpty() throws Exception StandardCharsets.UTF_8 ).addChunk(""); - EasyMock.expect(druidLeaderClient.go(EasyMock.anyObject(Request.class), EasyMock.anyObject(Optional.class))) + EasyMock.expect(druidLeaderClient.go(EasyMock.anyObject(Request.class))) .andReturn(responseHolder) .anyTimes(); diff --git a/server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java b/server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java index 5d52ed396315..7378c29897d5 100644 --- a/server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java +++ b/server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java @@ -41,6 +41,7 @@ import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.java.util.http.client.HttpClient; import org.apache.druid.java.util.http.client.Request; +import org.apache.druid.java.util.http.client.response.StringFullResponseHandler; import org.apache.druid.java.util.http.client.response.StringFullResponseHolder; import org.apache.druid.server.DruidNode; import org.apache.druid.server.initialization.BaseJettyTest; @@ -223,7 +224,7 @@ public void testServerFailureAndRedirect() throws Exception } @Test - public void testNon200ResponseFromServerAndCacheRefresh() throws Exception + public void test503ResponseFromServerAndCacheRefresh() throws Exception { DruidNodeDiscovery druidNodeDiscovery = EasyMock.createMock(DruidNodeDiscovery.class); // Should be called twice. Second time is when we refresh the cache since we get 503 in the first request @@ -258,7 +259,7 @@ public void testNon200ResponseFromServerAndCacheRefresh() throws Exception } @Test - public void testNon200ResponseFromServerNoCacheRefresh() throws Exception + public void test503ResponseFromServerNoCacheRefresh() throws Exception { DruidNodeDiscovery druidNodeDiscovery = EasyMock.createMock(DruidNodeDiscovery.class); // Should be called only once, since we want to handle 503 ourselves and don't want cache refresh @@ -291,7 +292,8 @@ public void testNon200ResponseFromServerNoCacheRefresh() throws Exception HttpResponseStatus.SERVICE_UNAVAILABLE, druidLeaderClient.go( request, - Optional.of(Sets.newHashSet(HttpResponseStatus.SERVICE_UNAVAILABLE)) + new StringFullResponseHandler(Charset.defaultCharset()), + false ).getResponse().getStatus() ); EasyMock.verify(druidNodeDiscovery); diff --git a/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java b/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java index 4ec513c5d5d1..8cc6956d117f 100644 --- a/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java +++ b/server/src/test/java/org/apache/druid/query/lookup/LookupReferencesManagerTest.java @@ -22,7 +22,6 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Sets; import org.apache.druid.discovery.DruidLeaderClient; import org.apache.druid.jackson.DefaultObjectMapper; import org.apache.druid.java.util.emitter.EmittingLogger; @@ -111,16 +110,15 @@ public void testStartStop() throws InterruptedException, IOException EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); EasyMock.replay(config); EasyMock.expect(druidLeaderClient.makeRequest( - HttpMethod.GET, - "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" - )) + HttpMethod.GET, + "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" + )) .andReturn(request); StringFullResponseHolder responseHolder = new StringFullResponseHolder( newEmptyResponse(HttpResponseStatus.OK), StandardCharsets.UTF_8 ).addChunk(strResult); - EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) - .andReturn(responseHolder); + EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); EasyMock.replay(druidLeaderClient); Assert.assertFalse(lookupReferencesManager.lifecycleLock.awaitStarted(1, TimeUnit.MICROSECONDS)); Assert.assertNull(lookupReferencesManager.mainThread); @@ -175,16 +173,15 @@ public void testAddGetRemove() throws Exception EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); EasyMock.replay(config); EasyMock.expect(druidLeaderClient.makeRequest( - HttpMethod.GET, - "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" - )) + HttpMethod.GET, + "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" + )) .andReturn(request); StringFullResponseHolder responseHolder = new StringFullResponseHolder( newEmptyResponse(HttpResponseStatus.OK), StandardCharsets.UTF_8 ).addChunk(strResult); - EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) - .andReturn(responseHolder); + EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); Assert.assertEquals(Optional.empty(), lookupReferencesManager.get("test")); @@ -216,16 +213,15 @@ public void testCloseIsCalledAfterStopping() throws Exception EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); EasyMock.replay(config); EasyMock.expect(druidLeaderClient.makeRequest( - HttpMethod.GET, - "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" - )) + HttpMethod.GET, + "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" + )) .andReturn(request); StringFullResponseHolder responseHolder = new StringFullResponseHolder( newEmptyResponse(HttpResponseStatus.OK), StandardCharsets.UTF_8 ).addChunk(strResult); - EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) - .andReturn(responseHolder); + EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); lookupReferencesManager.add("testMock", new LookupExtractorFactoryContainer("0", lookupExtractorFactory)); @@ -250,16 +246,15 @@ public void testDestroyIsCalledAfterRemove() throws Exception EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); EasyMock.replay(config); EasyMock.expect(druidLeaderClient.makeRequest( - HttpMethod.GET, - "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" - )) + HttpMethod.GET, + "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" + )) .andReturn(request); StringFullResponseHolder responseHolder = new StringFullResponseHolder( newEmptyResponse(HttpResponseStatus.OK), StandardCharsets.UTF_8 ).addChunk(strResult); - EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) - .andReturn(responseHolder); + EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); lookupReferencesManager.add("testMock", new LookupExtractorFactoryContainer("0", lookupExtractorFactory)); @@ -281,16 +276,15 @@ public void testGetNotThere() throws Exception EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); EasyMock.replay(config); EasyMock.expect(druidLeaderClient.makeRequest( - HttpMethod.GET, - "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" - )) + HttpMethod.GET, + "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" + )) .andReturn(request); StringFullResponseHolder responseHolder = new StringFullResponseHolder( newEmptyResponse(HttpResponseStatus.OK), StandardCharsets.UTF_8 ).addChunk(strResult); - EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) - .andReturn(responseHolder); + EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); Assert.assertEquals(Optional.empty(), lookupReferencesManager.get("notThere")); @@ -314,16 +308,15 @@ public void testUpdateWithHigherVersion() throws Exception EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); EasyMock.replay(config); EasyMock.expect(druidLeaderClient.makeRequest( - HttpMethod.GET, - "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" - )) + HttpMethod.GET, + "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" + )) .andReturn(request); StringFullResponseHolder responseHolder = new StringFullResponseHolder( newEmptyResponse(HttpResponseStatus.OK), StandardCharsets.UTF_8 ).addChunk(strResult); - EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) - .andReturn(responseHolder); + EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); lookupReferencesManager.add("testName", new LookupExtractorFactoryContainer("1", lookupExtractorFactory1)); @@ -351,16 +344,15 @@ public void testUpdateWithLowerVersion() throws Exception EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); EasyMock.replay(config); EasyMock.expect(druidLeaderClient.makeRequest( - HttpMethod.GET, - "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" - )) + HttpMethod.GET, + "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" + )) .andReturn(request); StringFullResponseHolder responseHolder = new StringFullResponseHolder( newEmptyResponse(HttpResponseStatus.OK), StandardCharsets.UTF_8 ).addChunk(strResult); - EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) - .andReturn(responseHolder); + EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); lookupReferencesManager.add("testName", new LookupExtractorFactoryContainer("1", lookupExtractorFactory1)); @@ -382,16 +374,15 @@ public void testRemoveNonExisting() throws Exception EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); EasyMock.replay(config); EasyMock.expect(druidLeaderClient.makeRequest( - HttpMethod.GET, - "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" - )) + HttpMethod.GET, + "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" + )) .andReturn(request); StringFullResponseHolder responseHolder = new StringFullResponseHolder( newEmptyResponse(HttpResponseStatus.OK), StandardCharsets.UTF_8 ).addChunk(strResult); - EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) - .andReturn(responseHolder); + EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); lookupReferencesManager.remove("test"); @@ -422,8 +413,7 @@ public void testGetAllLookupNames() throws Exception newEmptyResponse(HttpResponseStatus.OK), StandardCharsets.UTF_8 ).addChunk(strResult); - EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) - .andReturn(responseHolder); + EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); lookupReferencesManager.add("one", container1); @@ -476,16 +466,15 @@ public void testGetAllLookupsState() throws Exception EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); EasyMock.replay(config); EasyMock.expect(druidLeaderClient.makeRequest( - HttpMethod.GET, - "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" - )) + HttpMethod.GET, + "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" + )) .andReturn(request); StringFullResponseHolder responseHolder = new StringFullResponseHolder( newEmptyResponse(HttpResponseStatus.OK), StandardCharsets.UTF_8 ).addChunk(strResult); - EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) - .andReturn(responseHolder); + EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); lookupReferencesManager.add("one", container1); @@ -521,16 +510,15 @@ public void testRealModeWithMainThread() throws Exception EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); EasyMock.replay(config); EasyMock.expect(druidLeaderClient.makeRequest( - HttpMethod.GET, - "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" - )) + HttpMethod.GET, + "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" + )) .andReturn(request); StringFullResponseHolder responseHolder = new StringFullResponseHolder( newEmptyResponse(HttpResponseStatus.OK), StandardCharsets.UTF_8 ).addChunk(strResult); - EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) - .andReturn(responseHolder); + EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); Assert.assertTrue(lookupReferencesManager.mainThread.isAlive()); @@ -610,16 +598,15 @@ public void testCoordinatorLookupSync() throws Exception EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); EasyMock.replay(config); EasyMock.expect(druidLeaderClient.makeRequest( - HttpMethod.GET, - "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" - )) + HttpMethod.GET, + "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" + )) .andReturn(request); StringFullResponseHolder responseHolder = new StringFullResponseHolder( newEmptyResponse(HttpResponseStatus.OK), StandardCharsets.UTF_8 ).addChunk(strResult); - EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) - .andReturn(responseHolder); + EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); @@ -651,14 +638,12 @@ public int getCoordinatorRetryDelay() EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); EasyMock.replay(config); EasyMock.expect(druidLeaderClient.makeRequest( - HttpMethod.GET, - "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" - )) + HttpMethod.GET, + "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" + )) .andReturn(request) .anyTimes(); - EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) - .andThrow(new IllegalStateException()) - .anyTimes(); + EasyMock.expect(druidLeaderClient.go(request)).andThrow(new IllegalStateException()).anyTimes(); EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); @@ -686,14 +671,12 @@ public int getCoordinatorRetryDelay() EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); EasyMock.replay(config); EasyMock.expect(druidLeaderClient.makeRequest( - HttpMethod.GET, - "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" - )) + HttpMethod.GET, + "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" + )) .andReturn(request) .anyTimes(); - EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) - .andThrow(new IllegalStateException()) - .anyTimes(); + EasyMock.expect(druidLeaderClient.go(request)).andThrow(new IllegalStateException()).anyTimes(); EasyMock.replay(druidLeaderClient); lookupReferencesManager.start(); Assert.assertEquals( @@ -726,16 +709,15 @@ public boolean getEnableLookupSyncOnStartup() EasyMock.expect(config.getLookupTier()).andReturn(LOOKUP_TIER).anyTimes(); EasyMock.replay(config); EasyMock.expect(druidLeaderClient.makeRequest( - HttpMethod.GET, - "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" - )) + HttpMethod.GET, + "/druid/coordinator/v1/lookups/config/lookupTier?detailed=true" + )) .andReturn(request); StringFullResponseHolder responseHolder = new StringFullResponseHolder( newEmptyResponse(HttpResponseStatus.OK), StandardCharsets.UTF_8 ).addChunk(strResult); - EasyMock.expect(druidLeaderClient.go(request, Optional.of(Sets.newHashSet(HttpResponseStatus.NOT_FOUND)))) - .andReturn(responseHolder); + EasyMock.expect(druidLeaderClient.go(request)).andReturn(responseHolder); lookupReferencesManager.start(); Assert.assertEquals(Optional.empty(), lookupReferencesManager.get("testMockForDisableLookupSync")); diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/schema/SystemSchemaTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/schema/SystemSchemaTest.java index 715a40c4dad9..50d0f09a85e5 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/schema/SystemSchemaTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/schema/SystemSchemaTest.java @@ -1152,9 +1152,7 @@ public void testTasksTable() throws Exception HttpResponse httpResp = new DefaultHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.OK); InputStreamFullResponseHolder responseHolder = new InputStreamFullResponseHolder(httpResp); - EasyMock.expect(client.go(EasyMock.eq(request), EasyMock.anyObject(InputStreamFullResponseHandler.class))) - .andReturn(responseHolder) - .once(); + EasyMock.expect(client.go(EasyMock.eq(request), EasyMock.anyObject(InputStreamFullResponseHandler.class))).andReturn(responseHolder).once(); EasyMock.expect(request.getUrl()).andReturn(new URL("http://test-host:1234/druid/indexer/v1/tasks")).anyTimes(); String json = "[{\n" @@ -1279,7 +1277,7 @@ public void testTasksTableAuth() throws Exception HttpResponse httpResp = new DefaultHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.OK); - EasyMock.expect(client.go(EasyMock.eq(request), EasyMock.anyObject(HttpResponseHandler.class))) + EasyMock.expect(client.go(EasyMock.eq(request), EasyMock.anyObject())) .andReturn(createFullResponseHolder(httpResp, json)) .andReturn(createFullResponseHolder(httpResp, json)) .andReturn(createFullResponseHolder(httpResp, json)); @@ -1328,9 +1326,7 @@ public void testSupervisorTable() throws Exception HttpResponse httpResp = new DefaultHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.OK); InputStreamFullResponseHolder responseHolder = new InputStreamFullResponseHolder(httpResp); - EasyMock.expect(client.go(EasyMock.eq(request), EasyMock.anyObject(InputStreamFullResponseHandler.class))) - .andReturn(responseHolder) - .once(); + EasyMock.expect(client.go(EasyMock.eq(request), EasyMock.anyObject(InputStreamFullResponseHandler.class))).andReturn(responseHolder).once(); EasyMock.expect(responseHandler.getStatus()).andReturn(httpResp.getStatus().getCode()).anyTimes(); EasyMock.expect(request.getUrl()) @@ -1397,7 +1393,7 @@ public void testSupervisorTableAuth() throws Exception + "}]"; HttpResponse httpResponse = new DefaultHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.OK); - EasyMock.expect(client.go(EasyMock.eq(request), EasyMock.anyObject(HttpResponseHandler.class))) + EasyMock.expect(client.go(EasyMock.eq(request), EasyMock.anyObject())) .andReturn(createFullResponseHolder(httpResponse, json)) .andReturn(createFullResponseHolder(httpResponse, json)) .andReturn(createFullResponseHolder(httpResponse, json)); From f8e5122d85b31a13c7711cabb3e1c3112bfbc93d Mon Sep 17 00:00:00 2001 From: Abhishek Singh Chouhan Date: Tue, 18 Apr 2023 19:26:08 -0700 Subject: [PATCH 4/8] Remove unused imports --- .../org/apache/druid/discovery/DruidLeaderClientTest.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java b/server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java index 7378c29897d5..11487ba3f153 100644 --- a/server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java +++ b/server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java @@ -21,7 +21,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Sets; import com.google.common.util.concurrent.ListenableFutureTask; import com.google.inject.Binder; import com.google.inject.Inject; @@ -46,7 +45,6 @@ import org.apache.druid.server.DruidNode; import org.apache.druid.server.initialization.BaseJettyTest; import org.apache.druid.server.initialization.jetty.JettyServerInitializer; -import org.asynchttpclient.ListenableFuture; import org.easymock.EasyMock; import org.eclipse.jetty.server.Handler; import org.eclipse.jetty.server.Server; @@ -74,8 +72,6 @@ import java.net.URI; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; -import java.util.Optional; -import java.util.Set; /** */ From a6662d262cb743250576b127d0cc82e14205cfe9 Mon Sep 17 00:00:00 2001 From: Abhishek Singh Chouhan Date: Wed, 19 Apr 2023 00:03:22 -0700 Subject: [PATCH 5/8] Use argumentmatcher instead of Mockito for #any in test --- .../org/apache/druid/discovery/DruidLeaderClientTest.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java b/server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java index 11487ba3f153..d688c48ff7d1 100644 --- a/server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java +++ b/server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java @@ -60,6 +60,7 @@ import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; +import org.mockito.ArgumentMatchers; import org.mockito.Mockito; import javax.ws.rs.GET; @@ -238,7 +239,7 @@ public void test503ResponseFromServerAndCacheRefresh() throws Exception HttpClient spyHttpClient = Mockito.spy(this.httpClient); // Override behavior for the first call only - Mockito.doReturn(task).doCallRealMethod().when(spyHttpClient).go(Mockito.any(), Mockito.any()); + Mockito.doReturn(task).doCallRealMethod().when(spyHttpClient).go(ArgumentMatchers.any(), ArgumentMatchers.any()); DruidLeaderClient druidLeaderClient = new DruidLeaderClient( spyHttpClient, @@ -272,7 +273,7 @@ public void test503ResponseFromServerNoCacheRefresh() throws Exception EasyMock.replay(druidNodeDiscovery, druidNodeDiscoveryProvider, task); HttpClient spyHttpClient = Mockito.spy(this.httpClient); - Mockito.doReturn(task).doCallRealMethod().when(spyHttpClient).go(Mockito.any(), Mockito.any()); + Mockito.doReturn(task).doCallRealMethod().when(spyHttpClient).go(ArgumentMatchers.any(), ArgumentMatchers.any()); DruidLeaderClient druidLeaderClient = new DruidLeaderClient( spyHttpClient, From e51c3527b5fb86cc056dad617371fc026c163210 Mon Sep 17 00:00:00 2001 From: Abhishek Singh Chouhan Date: Wed, 19 Apr 2023 09:30:41 -0700 Subject: [PATCH 6/8] Remove flag to disable retry for 503/504 --- .../druid/discovery/DruidLeaderClient.java | 25 +++-------- .../discovery/DruidLeaderClientTest.java | 41 ------------------- 2 files changed, 6 insertions(+), 60 deletions(-) diff --git a/server/src/main/java/org/apache/druid/discovery/DruidLeaderClient.java b/server/src/main/java/org/apache/druid/discovery/DruidLeaderClient.java index 241c1bc2259d..a020bab29ca3 100644 --- a/server/src/main/java/org/apache/druid/discovery/DruidLeaderClient.java +++ b/server/src/main/java/org/apache/druid/discovery/DruidLeaderClient.java @@ -124,8 +124,7 @@ public Request makeRequest(HttpMethod httpMethod, String urlPath) throws IOExcep /** * Executes a Request object aimed at the leader. Throws IOException if the leader cannot be located. - * Internal retries are attempted if 503/504 response is received. If the caller wants to override this - * retry behavior, they should use {@link #go(Request, HttpResponseHandler, boolean)} + * Internal retries with cache invalidation are attempted if 503/504 response is received. * * @param request * @throws IOException @@ -133,33 +132,21 @@ public Request makeRequest(HttpMethod httpMethod, String urlPath) throws IOExcep */ public StringFullResponseHolder go(Request request) throws IOException, InterruptedException { - return go(request, new StringFullResponseHandler(StandardCharsets.UTF_8), true); - } - - /** - * Executes a Request object aimed at the leader. Throws IOException if the leader cannot be located. - * Internal retries are attempted if 503/504 response is received. If the caller wants to override this - * retry behavior, they should use {@link #go(Request, HttpResponseHandler, boolean)} - */ - public > H go(Request request, HttpResponseHandler responseHandler) - throws IOException, InterruptedException - { - return go(request, responseHandler, true); + return go(request, new StringFullResponseHandler(StandardCharsets.UTF_8)); } /** * Executes a Request object aimed at the leader. Throws IOException if the leader cannot be located. + * Internal retries with cache invalidation are attempted if 503/504 response is received. * * @param request * @param responseHandler - * @param shouldRetry If internal retries with cache invalidation should be done for 503 and 504 responses * @throws IOException * @throws InterruptedException */ public > H go( Request request, - HttpResponseHandler responseHandler, - boolean shouldRetry + HttpResponseHandler responseHandler ) throws IOException, InterruptedException { @@ -216,8 +203,8 @@ public > H go( )); request = withUrl(request, redirectUrl); - } else if (shouldRetry && (HttpResponseStatus.SERVICE_UNAVAILABLE.equals(responseStatus) - || HttpResponseStatus.GATEWAY_TIMEOUT.equals(responseStatus))) { + } else if (HttpResponseStatus.SERVICE_UNAVAILABLE.equals(responseStatus) + || HttpResponseStatus.GATEWAY_TIMEOUT.equals(responseStatus)) { request = getNewRequestUrlInvalidatingCache(request); } else { return fullResponseHolder; diff --git a/server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java b/server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java index d688c48ff7d1..5f5cf706b241 100644 --- a/server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java +++ b/server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java @@ -255,47 +255,6 @@ public void test503ResponseFromServerAndCacheRefresh() throws Exception EasyMock.verify(druidNodeDiscovery); } - @Test - public void test503ResponseFromServerNoCacheRefresh() throws Exception - { - DruidNodeDiscovery druidNodeDiscovery = EasyMock.createMock(DruidNodeDiscovery.class); - // Should be called only once, since we want to handle 503 ourselves and don't want cache refresh - EasyMock.expect(druidNodeDiscovery.getAllNodes()).andReturn(ImmutableList.of(discoveryDruidNode)).times(1); - - DruidNodeDiscoveryProvider druidNodeDiscoveryProvider = EasyMock.createMock(DruidNodeDiscoveryProvider.class); - EasyMock.expect(druidNodeDiscoveryProvider.getForNodeRole(NodeRole.PEON)).andReturn(druidNodeDiscovery).anyTimes(); - - ListenableFutureTask task = EasyMock.createMock(ListenableFutureTask.class); - EasyMock.expect(task.get()).andReturn(new StringFullResponseHolder(new DefaultHttpResponse( - HttpVersion.HTTP_1_1, - HttpResponseStatus.SERVICE_UNAVAILABLE - ), Charset.defaultCharset())); - EasyMock.replay(druidNodeDiscovery, druidNodeDiscoveryProvider, task); - - HttpClient spyHttpClient = Mockito.spy(this.httpClient); - Mockito.doReturn(task).doCallRealMethod().when(spyHttpClient).go(ArgumentMatchers.any(), ArgumentMatchers.any()); - - DruidLeaderClient druidLeaderClient = new DruidLeaderClient( - spyHttpClient, - druidNodeDiscoveryProvider, - NodeRole.PEON, - "/simple/leader" - ); - druidLeaderClient.start(); - - Request request = druidLeaderClient.makeRequest(HttpMethod.POST, "/simple/direct"); - request.setContent("hello".getBytes(StandardCharsets.UTF_8)); - Assert.assertEquals( - HttpResponseStatus.SERVICE_UNAVAILABLE, - druidLeaderClient.go( - request, - new StringFullResponseHandler(Charset.defaultCharset()), - false - ).getResponse().getStatus() - ); - EasyMock.verify(druidNodeDiscovery); - } - @Test public void testFindCurrentLeader() { From e288a90754043db263a2a2215192183bb247ebd7 Mon Sep 17 00:00:00 2001 From: Abhishek Singh Chouhan Date: Wed, 19 Apr 2023 10:19:39 -0700 Subject: [PATCH 7/8] Remove unused import from test --- .../java/org/apache/druid/discovery/DruidLeaderClientTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java b/server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java index 5f5cf706b241..6b8f32ba1906 100644 --- a/server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java +++ b/server/src/test/java/org/apache/druid/discovery/DruidLeaderClientTest.java @@ -40,7 +40,6 @@ import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.java.util.http.client.HttpClient; import org.apache.druid.java.util.http.client.Request; -import org.apache.druid.java.util.http.client.response.StringFullResponseHandler; import org.apache.druid.java.util.http.client.response.StringFullResponseHolder; import org.apache.druid.server.DruidNode; import org.apache.druid.server.initialization.BaseJettyTest; From beb881724f59a9a9ba8b8245292bf5547cd85513 Mon Sep 17 00:00:00 2001 From: Abhishek Singh Chouhan Date: Wed, 19 Apr 2023 21:46:09 -0700 Subject: [PATCH 8/8] Add log line for internal retry --- .../java/org/apache/druid/discovery/DruidLeaderClient.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/server/src/main/java/org/apache/druid/discovery/DruidLeaderClient.java b/server/src/main/java/org/apache/druid/discovery/DruidLeaderClient.java index a020bab29ca3..3bc29da40f46 100644 --- a/server/src/main/java/org/apache/druid/discovery/DruidLeaderClient.java +++ b/server/src/main/java/org/apache/druid/discovery/DruidLeaderClient.java @@ -205,6 +205,13 @@ public > H go( request = withUrl(request, redirectUrl); } else if (HttpResponseStatus.SERVICE_UNAVAILABLE.equals(responseStatus) || HttpResponseStatus.GATEWAY_TIMEOUT.equals(responseStatus)) { + log.warn( + "Request[%s] received a %s response. Attempt %s/%s", + request.getUrl(), + responseStatus, + counter + 1, + MAX_RETRIES + ); request = getNewRequestUrlInvalidatingCache(request); } else { return fullResponseHolder;