From dc0b2db44970d3ed7bc227bcee8a2378f6e9a112 Mon Sep 17 00:00:00 2001 From: Abhishek Balaji Radhakrishnan Date: Mon, 17 Jun 2024 07:32:12 -0700 Subject: [PATCH 1/2] Fix flakey BrokerClientTest. The testError() method reliably fails in the IDE. This is because the the test runner has 3 is set to 3, so maven retries this "flaky test" multiple times and the test code returns a successful response in the third attempt. The exception handling in BrokerClientTest was broken: - All non-2xx errors were being turned as 5xx errors. Remove that block of code. If we need to handle retries of more specific 5xx error codes, that should be hanlded explicitly. Or if there's a source of 4xx class error that needs to be 5xx, fix that in the source of error. --- .../apache/druid/discovery/BrokerClient.java | 8 ++--- .../druid/discovery/BrokerClientTest.java | 31 ++++++++++++++++++- 2 files changed, 33 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/apache/druid/discovery/BrokerClient.java b/server/src/main/java/org/apache/druid/discovery/BrokerClient.java index a64c8d670e4a..bdee9b8dfe4a 100644 --- a/server/src/main/java/org/apache/druid/discovery/BrokerClient.java +++ b/server/src/main/java/org/apache/druid/discovery/BrokerClient.java @@ -33,7 +33,6 @@ import org.jboss.netty.handler.codec.http.HttpMethod; import org.jboss.netty.handler.codec.http.HttpResponseStatus; -import javax.servlet.http.HttpServletResponse; import java.io.IOException; import java.net.MalformedURLException; import java.net.URL; @@ -88,10 +87,6 @@ public String sendQuery(final Request request) throws Exception throw DruidException.forPersona(DruidException.Persona.OPERATOR) .ofCategory(DruidException.Category.RUNTIME_FAILURE) .build("Request to broker failed due to failed response status: [%s]", responseStatus); - } else if (responseStatus.getCode() != HttpServletResponse.SC_OK) { - throw DruidException.forPersona(DruidException.Persona.OPERATOR) - .ofCategory(DruidException.Category.RUNTIME_FAILURE) - .build("Request to broker failed due to failed response code: [%s]", responseStatus.getCode()); } return fullResponseHolder.getContent(); }, @@ -99,6 +94,9 @@ public String sendQuery(final Request request) throws Exception if (throwable instanceof ExecutionException) { return throwable.getCause() instanceof IOException || throwable.getCause() instanceof ChannelException; } + if (throwable instanceof DruidException) { + return ((DruidException) throwable).getCategory() == DruidException.Category.RUNTIME_FAILURE; + } return throwable instanceof IOE; }, MAX_RETRIES diff --git a/server/src/test/java/org/apache/druid/discovery/BrokerClientTest.java b/server/src/test/java/org/apache/druid/discovery/BrokerClientTest.java index 333882a43051..e46f423f8f26 100644 --- a/server/src/test/java/org/apache/druid/discovery/BrokerClientTest.java +++ b/server/src/test/java/org/apache/druid/discovery/BrokerClientTest.java @@ -101,7 +101,7 @@ public void testSimple() throws Exception } @Test - public void testError() throws Exception + public void testRetryableError() throws Exception { DruidNodeDiscovery druidNodeDiscovery = EasyMock.createMock(DruidNodeDiscovery.class); EasyMock.expect(druidNodeDiscovery.getAllNodes()).andReturn(ImmutableList.of(discoveryDruidNode)).anyTimes(); @@ -121,6 +121,27 @@ public void testError() throws Exception Assert.assertEquals("hello", brokerClient.sendQuery(request)); } + @Test + public void testNonRetryableError() throws Exception + { + DruidNodeDiscovery druidNodeDiscovery = EasyMock.createMock(DruidNodeDiscovery.class); + EasyMock.expect(druidNodeDiscovery.getAllNodes()).andReturn(ImmutableList.of(discoveryDruidNode)).anyTimes(); + + DruidNodeDiscoveryProvider druidNodeDiscoveryProvider = EasyMock.createMock(DruidNodeDiscoveryProvider.class); + EasyMock.expect(druidNodeDiscoveryProvider.getForNodeRole(NodeRole.BROKER)).andReturn(druidNodeDiscovery); + + EasyMock.replay(druidNodeDiscovery, druidNodeDiscoveryProvider); + + BrokerClient brokerClient = new BrokerClient( + httpClient, + druidNodeDiscoveryProvider + ); + + Request request = brokerClient.makeRequest(HttpMethod.POST, "/simple/error"); + request.setContent("hello".getBytes(StandardCharsets.UTF_8)); + Assert.assertEquals("", brokerClient.sendQuery(request)); + } + @Path("/simple") public static class SimpleResource { @@ -150,5 +171,13 @@ public Response redirecting() return Response.status(504).build(); } } + + @POST + @Path("/error") + @Produces(MediaType.APPLICATION_JSON) + public Response error(String input) + { + return Response.status(404).build(); + } } } From 2b2eb34baef051b02fd75af22d4ebfe974fe5fed Mon Sep 17 00:00:00 2001 From: Abhishek Balaji Radhakrishnan Date: Mon, 17 Jun 2024 09:37:42 -0700 Subject: [PATCH 2/2] Fix CodeQL warning for unused parameter. --- .../test/java/org/apache/druid/discovery/BrokerClientTest.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/server/src/test/java/org/apache/druid/discovery/BrokerClientTest.java b/server/src/test/java/org/apache/druid/discovery/BrokerClientTest.java index e46f423f8f26..de03877a9b0c 100644 --- a/server/src/test/java/org/apache/druid/discovery/BrokerClientTest.java +++ b/server/src/test/java/org/apache/druid/discovery/BrokerClientTest.java @@ -138,7 +138,6 @@ public void testNonRetryableError() throws Exception ); Request request = brokerClient.makeRequest(HttpMethod.POST, "/simple/error"); - request.setContent("hello".getBytes(StandardCharsets.UTF_8)); Assert.assertEquals("", brokerClient.sendQuery(request)); } @@ -175,7 +174,7 @@ public Response redirecting() @POST @Path("/error") @Produces(MediaType.APPLICATION_JSON) - public Response error(String input) + public Response error() { return Response.status(404).build(); }