From 414d5c5da128ff8946936b51bdada632ca2d5231 Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Wed, 13 Sep 2023 14:10:36 +0000 Subject: [PATCH 01/21] stuff --- .../druid/java/util/common/logger/Logger.java | 17 +++ .../query/ChainedExecutionQueryRunner.java | 6 +- .../CalciteMultiValueStringQueryTest.java | 101 ++++++++++++++++++ 3 files changed, 123 insertions(+), 1 deletion(-) diff --git a/processing/src/main/java/org/apache/druid/java/util/common/logger/Logger.java b/processing/src/main/java/org/apache/druid/java/util/common/logger/Logger.java index 1639bf5378f8..982948e37995 100644 --- a/processing/src/main/java/org/apache/druid/java/util/common/logger/Logger.java +++ b/processing/src/main/java/org/apache/druid/java/util/common/logger/Logger.java @@ -111,6 +111,23 @@ public Logger noStackTrace() return noStackTraceLogger; } + /** + * Helps to retain stacktraces in logs in case debug is enabled. + */ + public Logger withStackTraceIf(boolean debug) + { + return debug ? this : noStackTraceLogger; + } + + /** + * Returns a copy of this Logger that does not log exception stack traces, unless the log level is DEBUG or lower. + * Useful for writing code like: {@code log.noStackTrace().warn(e, "Something happened.");} + */ + public Logger noStackTraceUnless() + { + return noStackTraceLogger; + } + public void trace(String message, Object... formatArgs) { if (log.isTraceEnabled()) { diff --git a/processing/src/main/java/org/apache/druid/query/ChainedExecutionQueryRunner.java b/processing/src/main/java/org/apache/druid/query/ChainedExecutionQueryRunner.java index 5ff044f65641..a150854681bf 100644 --- a/processing/src/main/java/org/apache/druid/query/ChainedExecutionQueryRunner.java +++ b/processing/src/main/java/org/apache/druid/query/ChainedExecutionQueryRunner.java @@ -113,6 +113,9 @@ public Iterable call() if (retVal == null) { throw new ISE("Got a null list of results"); } + if(true) { + throw new RuntimeException("jaja"); + } return retVal; } @@ -123,7 +126,8 @@ public Iterable call() throw e; } catch (Exception e) { - log.noStackTrace().error(e, "Exception with one of the sequences!"); + log.withStackTraceIf(query.context().isDebug()).error(e, "Exception with one of the sequences!"); + Throwables.propagateIfPossible(e); throw new RuntimeException(e); } diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteMultiValueStringQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteMultiValueStringQueryTest.java index d4f25101545c..9376087a86e7 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteMultiValueStringQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteMultiValueStringQueryTest.java @@ -20,6 +20,7 @@ package org.apache.druid.sql.calcite; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import org.apache.druid.common.config.NullHandling; import org.apache.druid.error.DruidException; @@ -27,6 +28,7 @@ import org.apache.druid.java.util.common.granularity.Granularities; import org.apache.druid.math.expr.ExpressionProcessing; import org.apache.druid.query.Druids; +import org.apache.druid.query.QueryContexts; import org.apache.druid.query.aggregation.CountAggregatorFactory; import org.apache.druid.query.aggregation.ExpressionLambdaAggregatorFactory; import org.apache.druid.query.aggregation.FilteredAggregatorFactory; @@ -2189,4 +2191,103 @@ public void testMultiValueStringOverlapFilterInconsistentUsage() ); } + + @Test + public void testXA1() + { + // Cannot vectorize due to usage of expressions. + cannotVectorize(); + + testBuilder() + .queryContext(ImmutableMap.of(QueryContexts.ENABLE_DEBUG, true)) + .sql("SELECT\n" + + " MV_FILTER_ONLY(\n" + + " CAST(\n" + + " LOOKUP(coalesce(t.\"dim2\",'a'),'lookyloo')\n" + + " AS VARCHAR),\n" + + " ARRAY['xa','abc','nosuchkey','6','asd1']\n" + + " ) AS \"COALESC-b7e\",\n" + + "count(1)\n" + + "FROM druid.foo AS t\n" + + " where dim3='x' \n" + + "GROUP BY 1\n" + + "LIMIT 1000") + .expectedQuery( + newScanQueryBuilder() + .dataSource(CalciteTests.DATASOURCE3) + .intervals(querySegmentSpec(Filtration.eternity())) + .virtualColumns( + expressionVirtualColumn( + "v0", + "string_to_array(concat(array_to_string(\"dim3\",','),',d'),',')", + ColumnType.STRING + ) + ) + .filters(expressionFilter( + "array_contains(string_to_array(concat(array_to_string(\"dim3\",','),',d'),','),'d')")) + .columns("v0") + .context(QUERY_CONTEXT_DEFAULT) + .build() + ) + .expectedResults( + NullHandling.sqlCompatible() ? + ImmutableList.of( + new Object[]{"[\"a\",\"b\",\"d\"]"}, + new Object[]{"[\"\",\"d\"]"} + ) : ImmutableList.of( + new Object[]{"[\"a\",\"b\",\"d\"]"}, + new Object[]{"[\"\",\"d\"]"} + ) + ) + .run(); + } + @Test + public void testXA1a() + { + // Cannot vectorize due to usage of expressions. + cannotVectorize(); + + testBuilder() + .sql("SELECT\n" + + " MV_FILTER_ONLY(\n" + + " CAST(\n" + + " LOOKUP(t.\"dim3\",'lookyloo')\n" + + " AS VARCHAR),\n" + + " ARRAY['Transparent Inventory','Ads per Page','Social Media Activity','asd','asd1']\n" + + " ) AS \"COALESC-b7e\",\n" + + "count(1)\n" + + "FROM druid.foo AS t\n" + + "GROUP BY 1\n" + + "LIMIT 1000") +// .expectedQuery( +// newScanQueryBuilder() +// .dataSource(CalciteTests.DATASOURCE3) +// .intervals(querySegmentSpec(Filtration.eternity())) +// .virtualColumns( +// expressionVirtualColumn( +// "v0", +// "string_to_array(concat(array_to_string(\"dim3\",','),',d'),',')", +// ColumnType.STRING +// ) +// ) +// .filters(expressionFilter( +// "array_contains(string_to_array(concat(array_to_string(\"dim3\",','),',d'),','),'d')")) +// .columns("v0") +// .context(QUERY_CONTEXT_DEFAULT) +// .build() +// ) + .expectedResults( + NullHandling.sqlCompatible() ? + ImmutableList.of( + new Object[]{"[\"a\",\"b\",\"d\"]"}, + new Object[]{"[\"\",\"d\"]"} + ) : ImmutableList.of( + new Object[]{"[\"a\",\"b\",\"d\"]"}, + new Object[]{"[\"\",\"d\"]"} + ) + ) + .run(); + } + + } From 8eeda44a3c4b20cac3253f66675f7f049300186d Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Wed, 13 Sep 2023 15:06:49 +0000 Subject: [PATCH 02/21] stu --- .../org/apache/druid/java/util/common/logger/Logger.java | 9 --------- 1 file changed, 9 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/java/util/common/logger/Logger.java b/processing/src/main/java/org/apache/druid/java/util/common/logger/Logger.java index 982948e37995..9d81046c026a 100644 --- a/processing/src/main/java/org/apache/druid/java/util/common/logger/Logger.java +++ b/processing/src/main/java/org/apache/druid/java/util/common/logger/Logger.java @@ -119,15 +119,6 @@ public Logger withStackTraceIf(boolean debug) return debug ? this : noStackTraceLogger; } - /** - * Returns a copy of this Logger that does not log exception stack traces, unless the log level is DEBUG or lower. - * Useful for writing code like: {@code log.noStackTrace().warn(e, "Something happened.");} - */ - public Logger noStackTraceUnless() - { - return noStackTraceLogger; - } - public void trace(String message, Object... formatArgs) { if (log.isTraceEnabled()) { From a44940ab33be41a57e301525f30911fd89980f8e Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Wed, 13 Sep 2023 16:14:06 +0000 Subject: [PATCH 03/21] fix0 --- .../druid/server/QueryResultPusher.java | 8 +- .../druid/server/QueryResultPusherTest.java | 89 +++++++++++++++++++ 2 files changed, 93 insertions(+), 4 deletions(-) create mode 100644 server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java diff --git a/server/src/main/java/org/apache/druid/server/QueryResultPusher.java b/server/src/main/java/org/apache/druid/server/QueryResultPusher.java index 074beb545b43..29f812c3323a 100644 --- a/server/src/main/java/org/apache/druid/server/QueryResultPusher.java +++ b/server/src/main/java/org/apache/druid/server/QueryResultPusher.java @@ -158,7 +158,7 @@ public Response push() // to show the user this error even if we didn't get to the step where // we did a security check. request.setAttribute(AuthConfig.DRUID_AUTHORIZATION_CHECKED, true); - return handleDruidException(resultsWriter, e); + return handleDruidException(resultsWriter, e, e); } catch (QueryException e) { return handleQueryException(resultsWriter, e); @@ -208,13 +208,13 @@ public Response push() @Nullable private Response handleQueryException(ResultsWriter resultsWriter, QueryException e) { - return handleDruidException(resultsWriter, DruidException.fromFailure(new QueryExceptionCompat(e))); + return handleDruidException(resultsWriter, DruidException.fromFailure(new QueryExceptionCompat(e)),e); } - private Response handleDruidException(ResultsWriter resultsWriter, DruidException e) + private Response handleDruidException(ResultsWriter resultsWriter, DruidException e, Exception e2) { if (resultsWriter != null) { - resultsWriter.recordFailure(e); + resultsWriter.recordFailure(e2); counter.incrementFailed(); if (accumulator != null && accumulator.isInitialized()) { diff --git a/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java b/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java new file mode 100644 index 000000000000..7f2993183e77 --- /dev/null +++ b/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java @@ -0,0 +1,89 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.druid.server; + +import static org.mockito.ArgumentMatchers.argThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.io.IOException; +import java.io.OutputStream; +import java.util.HashMap; +import java.util.Map; + +import javax.servlet.http.HttpServletRequest; +import javax.ws.rs.core.MediaType; + +import org.apache.druid.server.QueryResultPusher.ResultsWriter; +import org.junit.Test; + +import com.fasterxml.jackson.databind.ObjectMapper; +import com.google.common.base.Throwables; + +public class QueryResultPusherTest +{ + @Test + public void saf() + { + HttpServletRequest request = mock(HttpServletRequest.class); + ObjectMapper jsonMapper = new ObjectMapper(); + ResponseContextConfig responseContextConfig = ResponseContextConfig.newConfig(true); + DruidNode selfNode = mock(DruidNode.class); + QueryResource.QueryMetricCounter counter = mock(QueryResource.QueryMetricCounter.class); + String queryId = "x"; + MediaType contentType = null; + Map extraHeaders = new HashMap(); + + ResultsWriter resultWriter = mock(ResultsWriter.class); + + QueryResultPusher q = new QueryResultPusher(request, jsonMapper, responseContextConfig, selfNode, counter, queryId, + contentType, extraHeaders) + { + + @Override + public void writeException(Exception e, OutputStream out) throws IOException + { + throw new RuntimeException("Unimplemented!"); + + } + + @Override + public ResultsWriter start() + { + return resultWriter; + + } + }; + + + String embeddedExceptionMessage = "Embedded Exception Message!"; + RuntimeException embeddedException = new RuntimeException(embeddedExceptionMessage); + RuntimeException topException = new RuntimeException("Where's the party?", embeddedException); + + when(resultWriter.getQueryResponse()).thenThrow(topException); + + // launch + q.push(); + + verify(resultWriter).recordFailure(argThat( e -> Throwables.getStackTraceAsString ( e ).contains(embeddedExceptionMessage) )); + + } + +} From 3c8fa61e851542da2eaea88e8ecaf82c790aa611 Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Wed, 13 Sep 2023 16:44:12 +0000 Subject: [PATCH 04/21] updates --- .../druid/server/QueryResultPusher.java | 31 ++++++++++++------- .../druid/server/QueryResultPusherTest.java | 27 +++++++++------- 2 files changed, 34 insertions(+), 24 deletions(-) diff --git a/server/src/main/java/org/apache/druid/server/QueryResultPusher.java b/server/src/main/java/org/apache/druid/server/QueryResultPusher.java index 29f812c3323a..6c03cfae608d 100644 --- a/server/src/main/java/org/apache/druid/server/QueryResultPusher.java +++ b/server/src/main/java/org/apache/druid/server/QueryResultPusher.java @@ -158,10 +158,10 @@ public Response push() // to show the user this error even if we didn't get to the step where // we did a security check. request.setAttribute(AuthConfig.DRUID_AUTHORIZATION_CHECKED, true); - return handleDruidException(resultsWriter, e, e); + return handleException(resultsWriter, e); } catch (QueryException e) { - return handleQueryException(resultsWriter, e); + return handleException(resultsWriter, e); } catch (RuntimeException re) { if (re instanceof ForbiddenException) { @@ -174,10 +174,10 @@ public Response push() } throw re; } - return handleQueryException(resultsWriter, new QueryInterruptedException(re)); + return handleException(resultsWriter, re); } catch (IOException ioEx) { - return handleQueryException(resultsWriter, new QueryInterruptedException(ioEx)); + return handleException(resultsWriter, ioEx); } finally { if (accumulator != null) { @@ -205,16 +205,12 @@ public Response push() return null; } - @Nullable - private Response handleQueryException(ResultsWriter resultsWriter, QueryException e) + private Response handleException(ResultsWriter resultsWriter, Exception inputException) { - return handleDruidException(resultsWriter, DruidException.fromFailure(new QueryExceptionCompat(e)),e); - } + DruidException e = toDruidException(inputException); - private Response handleDruidException(ResultsWriter resultsWriter, DruidException e, Exception e2) - { if (resultsWriter != null) { - resultsWriter.recordFailure(e2); + resultsWriter.recordFailure(inputException); counter.incrementFailed(); if (accumulator != null && accumulator.isInitialized()) { @@ -265,7 +261,7 @@ private Response handleDruidException(ResultsWriter resultsWriter, DruidExceptio response.setStatus(e.getStatusCode()); response.setHeader("Content-Type", contentType.toString()); try (ServletOutputStream out = response.getOutputStream()) { - writeException(e, out); + writeException(inputException, out); } catch (IOException ioException) { log.warn( @@ -278,6 +274,17 @@ private Response handleDruidException(ResultsWriter resultsWriter, DruidExceptio } } + private DruidException toDruidException(Exception e) + { + if (e instanceof DruidException) { + return (DruidException) e; + } + if (!(e instanceof QueryException)) { + e = new QueryInterruptedException(e); + } + return DruidException.fromFailure(new QueryExceptionCompat((QueryException) e)); + } + public interface ResultsWriter extends Closeable { /** diff --git a/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java b/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java index 7f2993183e77..145e55236436 100644 --- a/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java +++ b/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java @@ -40,50 +40,53 @@ public class QueryResultPusherTest { @Test - public void saf() + public void testResultPusherRetainsNestedExceptionBacktraces() { HttpServletRequest request = mock(HttpServletRequest.class); ObjectMapper jsonMapper = new ObjectMapper(); ResponseContextConfig responseContextConfig = ResponseContextConfig.newConfig(true); DruidNode selfNode = mock(DruidNode.class); QueryResource.QueryMetricCounter counter = mock(QueryResource.QueryMetricCounter.class); - String queryId = "x"; + String queryId = "someQuery"; MediaType contentType = null; Map extraHeaders = new HashMap(); ResultsWriter resultWriter = mock(ResultsWriter.class); - QueryResultPusher q = new QueryResultPusher(request, jsonMapper, responseContextConfig, selfNode, counter, queryId, - contentType, extraHeaders) + QueryResultPusher pusher = new QueryResultPusher( + request, + jsonMapper, + responseContextConfig, + selfNode, + counter, + queryId, + contentType, + extraHeaders) { @Override public void writeException(Exception e, OutputStream out) throws IOException { throw new RuntimeException("Unimplemented!"); - } @Override public ResultsWriter start() { return resultWriter; - } }; - String embeddedExceptionMessage = "Embedded Exception Message!"; RuntimeException embeddedException = new RuntimeException(embeddedExceptionMessage); RuntimeException topException = new RuntimeException("Where's the party?", embeddedException); when(resultWriter.getQueryResponse()).thenThrow(topException); - // launch - q.push(); - - verify(resultWriter).recordFailure(argThat( e -> Throwables.getStackTraceAsString ( e ).contains(embeddedExceptionMessage) )); + // run pusher + pusher.push(); + verify(resultWriter) + .recordFailure(argThat(e -> Throwables.getStackTraceAsString(e).contains(embeddedExceptionMessage))); } - } From 738ad56f340e82ef01a9b457e32b6f17f1585c00 Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Wed, 13 Sep 2023 16:45:41 +0000 Subject: [PATCH 05/21] remove unrelated --- .../druid/java/util/common/logger/Logger.java | 8 -- .../query/ChainedExecutionQueryRunner.java | 6 +- .../CalciteMultiValueStringQueryTest.java | 101 ------------------ 3 files changed, 1 insertion(+), 114 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/java/util/common/logger/Logger.java b/processing/src/main/java/org/apache/druid/java/util/common/logger/Logger.java index 9d81046c026a..1639bf5378f8 100644 --- a/processing/src/main/java/org/apache/druid/java/util/common/logger/Logger.java +++ b/processing/src/main/java/org/apache/druid/java/util/common/logger/Logger.java @@ -111,14 +111,6 @@ public Logger noStackTrace() return noStackTraceLogger; } - /** - * Helps to retain stacktraces in logs in case debug is enabled. - */ - public Logger withStackTraceIf(boolean debug) - { - return debug ? this : noStackTraceLogger; - } - public void trace(String message, Object... formatArgs) { if (log.isTraceEnabled()) { diff --git a/processing/src/main/java/org/apache/druid/query/ChainedExecutionQueryRunner.java b/processing/src/main/java/org/apache/druid/query/ChainedExecutionQueryRunner.java index a150854681bf..5ff044f65641 100644 --- a/processing/src/main/java/org/apache/druid/query/ChainedExecutionQueryRunner.java +++ b/processing/src/main/java/org/apache/druid/query/ChainedExecutionQueryRunner.java @@ -113,9 +113,6 @@ public Iterable call() if (retVal == null) { throw new ISE("Got a null list of results"); } - if(true) { - throw new RuntimeException("jaja"); - } return retVal; } @@ -126,8 +123,7 @@ public Iterable call() throw e; } catch (Exception e) { - log.withStackTraceIf(query.context().isDebug()).error(e, "Exception with one of the sequences!"); - + log.noStackTrace().error(e, "Exception with one of the sequences!"); Throwables.propagateIfPossible(e); throw new RuntimeException(e); } diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteMultiValueStringQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteMultiValueStringQueryTest.java index 9376087a86e7..d4f25101545c 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteMultiValueStringQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteMultiValueStringQueryTest.java @@ -20,7 +20,6 @@ package org.apache.druid.sql.calcite; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import org.apache.druid.common.config.NullHandling; import org.apache.druid.error.DruidException; @@ -28,7 +27,6 @@ import org.apache.druid.java.util.common.granularity.Granularities; import org.apache.druid.math.expr.ExpressionProcessing; import org.apache.druid.query.Druids; -import org.apache.druid.query.QueryContexts; import org.apache.druid.query.aggregation.CountAggregatorFactory; import org.apache.druid.query.aggregation.ExpressionLambdaAggregatorFactory; import org.apache.druid.query.aggregation.FilteredAggregatorFactory; @@ -2191,103 +2189,4 @@ public void testMultiValueStringOverlapFilterInconsistentUsage() ); } - - @Test - public void testXA1() - { - // Cannot vectorize due to usage of expressions. - cannotVectorize(); - - testBuilder() - .queryContext(ImmutableMap.of(QueryContexts.ENABLE_DEBUG, true)) - .sql("SELECT\n" - + " MV_FILTER_ONLY(\n" - + " CAST(\n" - + " LOOKUP(coalesce(t.\"dim2\",'a'),'lookyloo')\n" - + " AS VARCHAR),\n" - + " ARRAY['xa','abc','nosuchkey','6','asd1']\n" - + " ) AS \"COALESC-b7e\",\n" - + "count(1)\n" - + "FROM druid.foo AS t\n" - + " where dim3='x' \n" - + "GROUP BY 1\n" - + "LIMIT 1000") - .expectedQuery( - newScanQueryBuilder() - .dataSource(CalciteTests.DATASOURCE3) - .intervals(querySegmentSpec(Filtration.eternity())) - .virtualColumns( - expressionVirtualColumn( - "v0", - "string_to_array(concat(array_to_string(\"dim3\",','),',d'),',')", - ColumnType.STRING - ) - ) - .filters(expressionFilter( - "array_contains(string_to_array(concat(array_to_string(\"dim3\",','),',d'),','),'d')")) - .columns("v0") - .context(QUERY_CONTEXT_DEFAULT) - .build() - ) - .expectedResults( - NullHandling.sqlCompatible() ? - ImmutableList.of( - new Object[]{"[\"a\",\"b\",\"d\"]"}, - new Object[]{"[\"\",\"d\"]"} - ) : ImmutableList.of( - new Object[]{"[\"a\",\"b\",\"d\"]"}, - new Object[]{"[\"\",\"d\"]"} - ) - ) - .run(); - } - @Test - public void testXA1a() - { - // Cannot vectorize due to usage of expressions. - cannotVectorize(); - - testBuilder() - .sql("SELECT\n" - + " MV_FILTER_ONLY(\n" - + " CAST(\n" - + " LOOKUP(t.\"dim3\",'lookyloo')\n" - + " AS VARCHAR),\n" - + " ARRAY['Transparent Inventory','Ads per Page','Social Media Activity','asd','asd1']\n" - + " ) AS \"COALESC-b7e\",\n" - + "count(1)\n" - + "FROM druid.foo AS t\n" - + "GROUP BY 1\n" - + "LIMIT 1000") -// .expectedQuery( -// newScanQueryBuilder() -// .dataSource(CalciteTests.DATASOURCE3) -// .intervals(querySegmentSpec(Filtration.eternity())) -// .virtualColumns( -// expressionVirtualColumn( -// "v0", -// "string_to_array(concat(array_to_string(\"dim3\",','),',d'),',')", -// ColumnType.STRING -// ) -// ) -// .filters(expressionFilter( -// "array_contains(string_to_array(concat(array_to_string(\"dim3\",','),',d'),','),'d')")) -// .columns("v0") -// .context(QUERY_CONTEXT_DEFAULT) -// .build() -// ) - .expectedResults( - NullHandling.sqlCompatible() ? - ImmutableList.of( - new Object[]{"[\"a\",\"b\",\"d\"]"}, - new Object[]{"[\"\",\"d\"]"} - ) : ImmutableList.of( - new Object[]{"[\"a\",\"b\",\"d\"]"}, - new Object[]{"[\"\",\"d\"]"} - ) - ) - .run(); - } - - } From 6d19e14d6a89d5e1633e9cc0cab83844154ce7dc Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Wed, 13 Sep 2023 16:50:46 +0000 Subject: [PATCH 06/21] fix checkstyle --- .../druid/server/QueryResultPusherTest.java | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java b/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java index 145e55236436..11754337a676 100644 --- a/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java +++ b/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java @@ -7,35 +7,35 @@ * "License"); you may not use this file except in compliance * with the License. You may obtain a copy of the License at * - * http://www.apache.org/licenses/LICENSE-2.0 + * http://www.apache.org/licenses/LICENSE-2.0 * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. */ package org.apache.druid.server; -import static org.mockito.ArgumentMatchers.argThat; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.google.common.base.Throwables; +import org.apache.druid.server.QueryResultPusher.ResultsWriter; +import org.junit.Test; + +import javax.servlet.http.HttpServletRequest; +import javax.ws.rs.core.MediaType; import java.io.IOException; import java.io.OutputStream; import java.util.HashMap; import java.util.Map; -import javax.servlet.http.HttpServletRequest; -import javax.ws.rs.core.MediaType; - -import org.apache.druid.server.QueryResultPusher.ResultsWriter; -import org.junit.Test; - -import com.fasterxml.jackson.databind.ObjectMapper; -import com.google.common.base.Throwables; +import static org.mockito.ArgumentMatchers.argThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; public class QueryResultPusherTest { From c863779c838a492d88890abfa4e0194a43f514cf Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Thu, 14 Sep 2023 07:40:48 +0000 Subject: [PATCH 07/21] bad idea --- .../druid/server/QueryResultPusherTest.java | 38 ++++++++++++++++++- 1 file changed, 36 insertions(+), 2 deletions(-) diff --git a/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java b/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java index 11754337a676..9ecf3e4a8ba8 100644 --- a/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java +++ b/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java @@ -23,13 +23,18 @@ import com.google.common.base.Throwables; import org.apache.druid.server.QueryResultPusher.ResultsWriter; import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameters; import javax.servlet.http.HttpServletRequest; import javax.ws.rs.core.MediaType; import java.io.IOException; import java.io.OutputStream; +import java.util.ArrayList; import java.util.HashMap; +import java.util.List; import java.util.Map; import static org.mockito.ArgumentMatchers.argThat; @@ -37,8 +42,29 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +@RunWith(Parameterized.class) public class QueryResultPusherTest { + + private boolean resultWriterPath; + + @Parameters + public static List parameters() + { + + List params = new ArrayList<>(); + + params.add(new Object[] {true}); + params.add(new Object[] {false}); + + return params; + } + + public QueryResultPusherTest(boolean resultWriterPath) + { + this.resultWriterPath = resultWriterPath; + } + @Test public void testResultPusherRetainsNestedExceptionBacktraces() { @@ -67,13 +93,21 @@ public void testResultPusherRetainsNestedExceptionBacktraces() @Override public void writeException(Exception e, OutputStream out) throws IOException { - throw new RuntimeException("Unimplemented!"); + if (resultWriterPath) { + throw new RuntimeException("Unexpected"); + } else { + resultWriter.recordFailure(e); + } } @Override public ResultsWriter start() { - return resultWriter; + if (resultWriterPath) { + return resultWriter; + } else { + return null; + } } }; From 97ab0c60b692fd5bd5bd4fd68bc92a0c6fdc5399 Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Thu, 14 Sep 2023 07:40:50 +0000 Subject: [PATCH 08/21] Revert "bad idea" This reverts commit c863779c838a492d88890abfa4e0194a43f514cf. --- .../druid/server/QueryResultPusherTest.java | 38 +------------------ 1 file changed, 2 insertions(+), 36 deletions(-) diff --git a/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java b/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java index 9ecf3e4a8ba8..11754337a676 100644 --- a/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java +++ b/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java @@ -23,18 +23,13 @@ import com.google.common.base.Throwables; import org.apache.druid.server.QueryResultPusher.ResultsWriter; import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.Parameterized; -import org.junit.runners.Parameterized.Parameters; import javax.servlet.http.HttpServletRequest; import javax.ws.rs.core.MediaType; import java.io.IOException; import java.io.OutputStream; -import java.util.ArrayList; import java.util.HashMap; -import java.util.List; import java.util.Map; import static org.mockito.ArgumentMatchers.argThat; @@ -42,29 +37,8 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -@RunWith(Parameterized.class) public class QueryResultPusherTest { - - private boolean resultWriterPath; - - @Parameters - public static List parameters() - { - - List params = new ArrayList<>(); - - params.add(new Object[] {true}); - params.add(new Object[] {false}); - - return params; - } - - public QueryResultPusherTest(boolean resultWriterPath) - { - this.resultWriterPath = resultWriterPath; - } - @Test public void testResultPusherRetainsNestedExceptionBacktraces() { @@ -93,21 +67,13 @@ public void testResultPusherRetainsNestedExceptionBacktraces() @Override public void writeException(Exception e, OutputStream out) throws IOException { - if (resultWriterPath) { - throw new RuntimeException("Unexpected"); - } else { - resultWriter.recordFailure(e); - } + throw new RuntimeException("Unimplemented!"); } @Override public ResultsWriter start() { - if (resultWriterPath) { - return resultWriter; - } else { - return null; - } + return resultWriter; } }; From 5bbd85277e7a51e49f56f89c955095ddaa503c9b Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Thu, 14 Sep 2023 08:02:07 +0000 Subject: [PATCH 09/21] update test to check #writeException --- .../druid/server/QueryResultPusherTest.java | 25 ++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java b/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java index 11754337a676..c61fbdab46c2 100644 --- a/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java +++ b/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java @@ -21,17 +21,23 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.base.Throwables; +import org.apache.druid.java.util.common.guava.Sequence; import org.apache.druid.server.QueryResultPusher.ResultsWriter; import org.junit.Test; +import javax.servlet.AsyncContext; +import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; import javax.ws.rs.core.MediaType; import java.io.IOException; import java.io.OutputStream; import java.util.HashMap; +import java.util.List; import java.util.Map; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; @@ -48,10 +54,11 @@ public void testResultPusherRetainsNestedExceptionBacktraces() DruidNode selfNode = mock(DruidNode.class); QueryResource.QueryMetricCounter counter = mock(QueryResource.QueryMetricCounter.class); String queryId = "someQuery"; - MediaType contentType = null; + MediaType contentType = mock(MediaType.class); Map extraHeaders = new HashMap(); ResultsWriter resultWriter = mock(ResultsWriter.class); + List loggedExceptions = mock(List.class); QueryResultPusher pusher = new QueryResultPusher( request, @@ -67,7 +74,7 @@ public void testResultPusherRetainsNestedExceptionBacktraces() @Override public void writeException(Exception e, OutputStream out) throws IOException { - throw new RuntimeException("Unimplemented!"); + loggedExceptions.add(e); } @Override @@ -81,12 +88,24 @@ public ResultsWriter start() RuntimeException embeddedException = new RuntimeException(embeddedExceptionMessage); RuntimeException topException = new RuntimeException("Where's the party?", embeddedException); - when(resultWriter.getQueryResponse()).thenThrow(topException); + QueryResponse queryResponse = mock(QueryResponse.class); + Sequence results = mock(Sequence.class); + AsyncContext asyncContext = mock(AsyncContext.class); + ServletResponse response = mock(HttpServletResponse.class); + + when(resultWriter.getQueryResponse()).thenReturn(queryResponse); + when(queryResponse.getResults()).thenReturn(results); + when(results.accumulate(any(), any())).thenThrow(topException); + when(request.startAsync()).thenReturn(asyncContext); + when(asyncContext.getResponse()).thenReturn(response); // run pusher pusher.push(); verify(resultWriter) .recordFailure(argThat(e -> Throwables.getStackTraceAsString(e).contains(embeddedExceptionMessage))); + verify(loggedExceptions) + .add(argThat(e -> Throwables.getStackTraceAsString(e).contains(embeddedExceptionMessage))); + } } From 065225a644be558ed1fc840c49d6a7984bf110b1 Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Thu, 14 Sep 2023 08:07:39 +0000 Subject: [PATCH 10/21] cleanup --- .../java/org/apache/druid/server/QueryResultPusherTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java b/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java index c61fbdab46c2..eb5886b663d4 100644 --- a/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java +++ b/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java @@ -95,9 +95,9 @@ public ResultsWriter start() when(resultWriter.getQueryResponse()).thenReturn(queryResponse); when(queryResponse.getResults()).thenReturn(results); - when(results.accumulate(any(), any())).thenThrow(topException); when(request.startAsync()).thenReturn(asyncContext); when(asyncContext.getResponse()).thenReturn(response); + when(results.accumulate(any(), any())).thenThrow(topException); // run pusher pusher.push(); From d9c0fac06cf4351cd2c368cc442959242bd34981 Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Thu, 14 Sep 2023 08:12:09 +0000 Subject: [PATCH 11/21] warnings --- .../java/org/apache/druid/server/QueryResultPusherTest.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java b/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java index eb5886b663d4..adaf597a7eee 100644 --- a/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java +++ b/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java @@ -58,6 +58,7 @@ public void testResultPusherRetainsNestedExceptionBacktraces() Map extraHeaders = new HashMap(); ResultsWriter resultWriter = mock(ResultsWriter.class); + @SuppressWarnings("unchecked") List loggedExceptions = mock(List.class); QueryResultPusher pusher = new QueryResultPusher( @@ -88,7 +89,9 @@ public ResultsWriter start() RuntimeException embeddedException = new RuntimeException(embeddedExceptionMessage); RuntimeException topException = new RuntimeException("Where's the party?", embeddedException); + @SuppressWarnings("unchecked") QueryResponse queryResponse = mock(QueryResponse.class); + @SuppressWarnings("unchecked") Sequence results = mock(Sequence.class); AsyncContext asyncContext = mock(AsyncContext.class); ServletResponse response = mock(HttpServletResponse.class); From 5571b083e69124b422ccb63528ba3cef1330bb12 Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Thu, 14 Sep 2023 08:22:13 +0000 Subject: [PATCH 12/21] alternate fix --- .../druid/error/QueryExceptionCompat.java | 2 +- .../druid/server/QueryResultPusher.java | 31 +++++++------------ 2 files changed, 13 insertions(+), 20 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/error/QueryExceptionCompat.java b/processing/src/main/java/org/apache/druid/error/QueryExceptionCompat.java index 9894208e9f9c..163c7b04cd0b 100644 --- a/processing/src/main/java/org/apache/druid/error/QueryExceptionCompat.java +++ b/processing/src/main/java/org/apache/druid/error/QueryExceptionCompat.java @@ -47,7 +47,7 @@ protected DruidException makeException(DruidException.DruidExceptionBuilder bob) { return bob.forPersona(DruidException.Persona.OPERATOR) .ofCategory(convertFailType(exception.getFailType())) - .build(exception.getMessage()) + .build(exception, exception.getMessage()) .withContext("host", exception.getHost()) .withContext("errorClass", exception.getErrorClass()) .withContext("legacyErrorCode", exception.getErrorCode()); diff --git a/server/src/main/java/org/apache/druid/server/QueryResultPusher.java b/server/src/main/java/org/apache/druid/server/QueryResultPusher.java index 6c03cfae608d..074beb545b43 100644 --- a/server/src/main/java/org/apache/druid/server/QueryResultPusher.java +++ b/server/src/main/java/org/apache/druid/server/QueryResultPusher.java @@ -158,10 +158,10 @@ public Response push() // to show the user this error even if we didn't get to the step where // we did a security check. request.setAttribute(AuthConfig.DRUID_AUTHORIZATION_CHECKED, true); - return handleException(resultsWriter, e); + return handleDruidException(resultsWriter, e); } catch (QueryException e) { - return handleException(resultsWriter, e); + return handleQueryException(resultsWriter, e); } catch (RuntimeException re) { if (re instanceof ForbiddenException) { @@ -174,10 +174,10 @@ public Response push() } throw re; } - return handleException(resultsWriter, re); + return handleQueryException(resultsWriter, new QueryInterruptedException(re)); } catch (IOException ioEx) { - return handleException(resultsWriter, ioEx); + return handleQueryException(resultsWriter, new QueryInterruptedException(ioEx)); } finally { if (accumulator != null) { @@ -205,12 +205,16 @@ public Response push() return null; } - private Response handleException(ResultsWriter resultsWriter, Exception inputException) + @Nullable + private Response handleQueryException(ResultsWriter resultsWriter, QueryException e) { - DruidException e = toDruidException(inputException); + return handleDruidException(resultsWriter, DruidException.fromFailure(new QueryExceptionCompat(e))); + } + private Response handleDruidException(ResultsWriter resultsWriter, DruidException e) + { if (resultsWriter != null) { - resultsWriter.recordFailure(inputException); + resultsWriter.recordFailure(e); counter.incrementFailed(); if (accumulator != null && accumulator.isInitialized()) { @@ -261,7 +265,7 @@ private Response handleException(ResultsWriter resultsWriter, Exception inputExc response.setStatus(e.getStatusCode()); response.setHeader("Content-Type", contentType.toString()); try (ServletOutputStream out = response.getOutputStream()) { - writeException(inputException, out); + writeException(e, out); } catch (IOException ioException) { log.warn( @@ -274,17 +278,6 @@ private Response handleException(ResultsWriter resultsWriter, Exception inputExc } } - private DruidException toDruidException(Exception e) - { - if (e instanceof DruidException) { - return (DruidException) e; - } - if (!(e instanceof QueryException)) { - e = new QueryInterruptedException(e); - } - return DruidException.fromFailure(new QueryExceptionCompat((QueryException) e)); - } - public interface ResultsWriter extends Closeable { /** From 915e37019df34c29b7a7cd13b1fd458586881209 Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Thu, 14 Sep 2023 13:09:40 +0000 Subject: [PATCH 13/21] more test --- .../druid/server/QueryResourceTest.java | 102 ++++++++++++++++++ 1 file changed, 102 insertions(+) diff --git a/server/src/test/java/org/apache/druid/server/QueryResourceTest.java b/server/src/test/java/org/apache/druid/server/QueryResourceTest.java index c7c96ecd3b12..fad7a57c85fc 100644 --- a/server/src/test/java/org/apache/druid/server/QueryResourceTest.java +++ b/server/src/test/java/org/apache/druid/server/QueryResourceTest.java @@ -23,6 +23,7 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.jaxrs.smile.SmileMediaTypes; import com.google.common.base.Suppliers; +import com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; @@ -106,6 +107,13 @@ import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; +import static org.junit.Assert.assertEquals; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.ArgumentMatchers.argThat; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.verify; + public class QueryResourceTest { private static final QueryToolChestWarehouse WAREHOUSE = new MapQueryToolChestWarehouse(ImmutableMap.of()); @@ -375,6 +383,100 @@ public QueryRunner getQueryRunnerForSegments( ); } + + @Test + public void testQueryThrowsRuntimeExceptionFromLifecycleExecute() throws IOException + { + String embeddedExceptionMessage = "Embedded Exception Message!"; + String overrideConfigKey = "priority"; + String overrideConfigValue = "678"; + + final List lifeCycleSpys=new ArrayList(); + DefaultQueryConfig overrideConfig = new DefaultQueryConfig(ImmutableMap.of(overrideConfigKey, overrideConfigValue)); + queryResource = new QueryResource( + new QueryLifecycleFactory( + WAREHOUSE, + new QuerySegmentWalker() + { + @Override + public QueryRunner getQueryRunnerForIntervals( + Query query, + Iterable intervals + ) + { + throw new RuntimeException("something", new RuntimeException(embeddedExceptionMessage)); + } + + @Override + public QueryRunner getQueryRunnerForSegments( + Query query, + Iterable specs + ) + { + throw new UnsupportedOperationException(); + } + }, + new DefaultGenericQueryMetricsFactory(), + new NoopServiceEmitter(), + testRequestLogger, + new AuthConfig(), + AuthTestUtils.TEST_AUTHORIZER_MAPPER, + Suppliers.ofInstance(overrideConfig) + ) { + + @Override + public QueryLifecycle factorize() + { + QueryLifecycle ret = spy(super.factorize()); + lifeCycleSpys.add(ret); + return ret; + } + }, + jsonMapper, + smileMapper, + queryScheduler, + new AuthConfig(), + null, + ResponseContextConfig.newConfig(true), + DRUID_NODE + ); + + expectPermissiveHappyPathAuth(); + + final Response response = expectSynchronousRequestFlow(SIMPLE_TIMESERIES_QUERY); + Assert.assertEquals(Status.INTERNAL_SERVER_ERROR.getStatusCode(), response.getStatus()); + + final ErrorResponse entity = (ErrorResponse) response.getEntity(); + MatcherAssert.assertThat( + entity.getUnderlyingException(), + new DruidExceptionMatcher( + DruidException.Persona.OPERATOR, + DruidException.Category.RUNTIME_FAILURE, "legacyQueryException") + .expectMessageIs("something") + ); + + + assertEquals(1, lifeCycleSpys.size()); + verify(lifeCycleSpys.get(0)) + .emitLogsAndMetrics( + argThat(e -> Throwables.getStackTraceAsString(e).contains(embeddedExceptionMessage)), + any(), + anyLong()); + + Assert.assertEquals(1, testRequestLogger.getNativeQuerylogs().size()); + Assert.assertNotNull(testRequestLogger.getNativeQuerylogs().get(0).getQuery()); + Assert.assertNotNull(testRequestLogger.getNativeQuerylogs().get(0).getQuery().getContext()); + Assert.assertTrue(testRequestLogger.getNativeQuerylogs() + .get(0) + .getQuery() + .getContext() + .containsKey(overrideConfigKey)); + Assert.assertEquals( + overrideConfigValue, + testRequestLogger.getNativeQuerylogs().get(0).getQuery().getContext().get(overrideConfigKey) + ); + } + @Test public void testGoodQueryWithQueryConfigDoesNotOverrideQueryContext() throws IOException { From 6069b104a1270866f608ae18c3002e31bb25efb8 Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Thu, 14 Sep 2023 13:14:02 +0000 Subject: [PATCH 14/21] fix style --- .../test/java/org/apache/druid/server/QueryResourceTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/org/apache/druid/server/QueryResourceTest.java b/server/src/test/java/org/apache/druid/server/QueryResourceTest.java index fad7a57c85fc..e221107faaf6 100644 --- a/server/src/test/java/org/apache/druid/server/QueryResourceTest.java +++ b/server/src/test/java/org/apache/druid/server/QueryResourceTest.java @@ -391,7 +391,7 @@ public void testQueryThrowsRuntimeExceptionFromLifecycleExecute() throws IOExcep String overrideConfigKey = "priority"; String overrideConfigValue = "678"; - final List lifeCycleSpys=new ArrayList(); + final List lifeCycleSpys = new ArrayList(); DefaultQueryConfig overrideConfig = new DefaultQueryConfig(ImmutableMap.of(overrideConfigKey, overrideConfigValue)); queryResource = new QueryResource( new QueryLifecycleFactory( From b8ede465e781627f2661e8afcf313461c6fa7567 Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Thu, 14 Sep 2023 13:52:31 +0000 Subject: [PATCH 15/21] different approach --- .../druid/server/QueryResourceTest.java | 88 +++++++++---------- 1 file changed, 42 insertions(+), 46 deletions(-) diff --git a/server/src/test/java/org/apache/druid/server/QueryResourceTest.java b/server/src/test/java/org/apache/druid/server/QueryResourceTest.java index e221107faaf6..94b9d970b544 100644 --- a/server/src/test/java/org/apache/druid/server/QueryResourceTest.java +++ b/server/src/test/java/org/apache/druid/server/QueryResourceTest.java @@ -88,6 +88,7 @@ import org.junit.Test; import javax.annotation.Nonnull; +import javax.annotation.Nullable; import javax.servlet.http.HttpServletResponse; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; @@ -107,12 +108,7 @@ import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; -import static org.junit.Assert.assertEquals; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyLong; -import static org.mockito.ArgumentMatchers.argThat; -import static org.mockito.Mockito.spy; -import static org.mockito.Mockito.verify; +import static org.assertj.core.api.Assertions.assertThat; public class QueryResourceTest { @@ -391,45 +387,52 @@ public void testQueryThrowsRuntimeExceptionFromLifecycleExecute() throws IOExcep String overrideConfigKey = "priority"; String overrideConfigValue = "678"; - final List lifeCycleSpys = new ArrayList(); DefaultQueryConfig overrideConfig = new DefaultQueryConfig(ImmutableMap.of(overrideConfigKey, overrideConfigValue)); - queryResource = new QueryResource( - new QueryLifecycleFactory( - WAREHOUSE, - new QuerySegmentWalker() - { - @Override - public QueryRunner getQueryRunnerForIntervals( - Query query, - Iterable intervals - ) - { - throw new RuntimeException("something", new RuntimeException(embeddedExceptionMessage)); - } + QuerySegmentWalker querySegmentWalker = new QuerySegmentWalker() + { + @Override + public QueryRunner getQueryRunnerForIntervals( + Query query, + Iterable intervals + ) + { + throw new RuntimeException("something", new RuntimeException(embeddedExceptionMessage)); + } - @Override - public QueryRunner getQueryRunnerForSegments( - Query query, - Iterable specs - ) - { - throw new UnsupportedOperationException(); - } - }, - new DefaultGenericQueryMetricsFactory(), - new NoopServiceEmitter(), - testRequestLogger, - new AuthConfig(), - AuthTestUtils.TEST_AUTHORIZER_MAPPER, - Suppliers.ofInstance(overrideConfig) - ) { + @Override + public QueryRunner getQueryRunnerForSegments( + Query query, + Iterable specs + ) + { + throw new UnsupportedOperationException(); + } + }; + + queryResource = new QueryResource( + new QueryLifecycleFactory(null, null, null, null, null, null, null, Suppliers.ofInstance(overrideConfig)){ @Override public QueryLifecycle factorize() { - QueryLifecycle ret = spy(super.factorize()); - lifeCycleSpys.add(ret); - return ret; + return new QueryLifecycle( + WAREHOUSE, + querySegmentWalker, + new DefaultGenericQueryMetricsFactory(), + new NoopServiceEmitter(), + testRequestLogger, + AuthTestUtils.TEST_AUTHORIZER_MAPPER, + overrideConfig, + new AuthConfig(), + System.currentTimeMillis(), + System.nanoTime()) + { + @Override + public void emitLogsAndMetrics(@Nullable Throwable e, @Nullable String remoteAddress, long bytesWritten) + { + assertThat(Throwables.getStackTraceAsString(e).contains(embeddedExceptionMessage)); + } + }; } }, jsonMapper, @@ -456,13 +459,6 @@ public QueryLifecycle factorize() ); - assertEquals(1, lifeCycleSpys.size()); - verify(lifeCycleSpys.get(0)) - .emitLogsAndMetrics( - argThat(e -> Throwables.getStackTraceAsString(e).contains(embeddedExceptionMessage)), - any(), - anyLong()); - Assert.assertEquals(1, testRequestLogger.getNativeQuerylogs().size()); Assert.assertNotNull(testRequestLogger.getNativeQuerylogs().get(0).getQuery()); Assert.assertNotNull(testRequestLogger.getNativeQuerylogs().get(0).getQuery().getContext()); From 2831662dd23735212bb385499044fdc02cc95984 Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Thu, 14 Sep 2023 14:48:27 +0000 Subject: [PATCH 16/21] update tests --- .../druid/server/QueryResourceTest.java | 21 +---- .../druid/server/QueryResultPusherTest.java | 79 ++++++++++++------- 2 files changed, 54 insertions(+), 46 deletions(-) diff --git a/server/src/test/java/org/apache/druid/server/QueryResourceTest.java b/server/src/test/java/org/apache/druid/server/QueryResourceTest.java index 94b9d970b544..bf8ecc1fadeb 100644 --- a/server/src/test/java/org/apache/druid/server/QueryResourceTest.java +++ b/server/src/test/java/org/apache/druid/server/QueryResourceTest.java @@ -108,7 +108,7 @@ import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; -import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.Assert.assertTrue; public class QueryResourceTest { @@ -411,7 +411,8 @@ public QueryRunner getQueryRunnerForSegments( queryResource = new QueryResource( - new QueryLifecycleFactory(null, null, null, null, null, null, null, Suppliers.ofInstance(overrideConfig)){ + new QueryLifecycleFactory(null, null, null, null, null, null, null, Suppliers.ofInstance(overrideConfig)) + { @Override public QueryLifecycle factorize() { @@ -430,7 +431,7 @@ public QueryLifecycle factorize() @Override public void emitLogsAndMetrics(@Nullable Throwable e, @Nullable String remoteAddress, long bytesWritten) { - assertThat(Throwables.getStackTraceAsString(e).contains(embeddedExceptionMessage)); + assertTrue(Throwables.getStackTraceAsString(e).contains(embeddedExceptionMessage)); } }; } @@ -457,20 +458,6 @@ public void emitLogsAndMetrics(@Nullable Throwable e, @Nullable String remoteAdd DruidException.Category.RUNTIME_FAILURE, "legacyQueryException") .expectMessageIs("something") ); - - - Assert.assertEquals(1, testRequestLogger.getNativeQuerylogs().size()); - Assert.assertNotNull(testRequestLogger.getNativeQuerylogs().get(0).getQuery()); - Assert.assertNotNull(testRequestLogger.getNativeQuerylogs().get(0).getQuery().getContext()); - Assert.assertTrue(testRequestLogger.getNativeQuerylogs() - .get(0) - .getQuery() - .getContext() - .containsKey(overrideConfigKey)); - Assert.assertEquals( - overrideConfigValue, - testRequestLogger.getNativeQuerylogs().get(0).getQuery().getContext().get(overrideConfigKey) - ); } @Test diff --git a/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java b/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java index adaf597a7eee..0e0a2306cafc 100644 --- a/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java +++ b/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java @@ -21,14 +21,13 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.base.Throwables; -import org.apache.druid.java.util.common.guava.Sequence; +import org.apache.druid.jackson.DefaultObjectMapper; import org.apache.druid.server.QueryResultPusher.ResultsWriter; +import org.apache.druid.server.mocks.MockHttpServletRequest; +import org.easymock.IArgumentMatcher; import org.junit.Test; -import javax.servlet.AsyncContext; -import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; import javax.ws.rs.core.MediaType; import java.io.IOException; @@ -37,24 +36,35 @@ import java.util.List; import java.util.Map; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.argThat; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; +import static org.easymock.EasyMock.expect; +import static org.easymock.EasyMock.expectLastCall; +import static org.easymock.EasyMock.mock; +import static org.easymock.EasyMock.replay; +import static org.easymock.EasyMock.reportMatcher; +import static org.easymock.EasyMock.verify; public class QueryResultPusherTest { + private static final DruidNode DRUID_NODE = new DruidNode( + "broker", + "localhost", + true, + 8082, + null, + true, + false); + @Test - public void testResultPusherRetainsNestedExceptionBacktraces() + public void testResultPusherRetainsNestedExceptionBacktraces() throws Exception { - HttpServletRequest request = mock(HttpServletRequest.class); - ObjectMapper jsonMapper = new ObjectMapper(); + + HttpServletRequest request = new MockHttpServletRequest(); + ObjectMapper jsonMapper = new DefaultObjectMapper(); ResponseContextConfig responseContextConfig = ResponseContextConfig.newConfig(true); - DruidNode selfNode = mock(DruidNode.class); + DruidNode selfNode = DRUID_NODE; QueryResource.QueryMetricCounter counter = mock(QueryResource.QueryMetricCounter.class); String queryId = "someQuery"; - MediaType contentType = mock(MediaType.class); + MediaType contentType = MediaType.APPLICATION_JSON_TYPE; Map extraHeaders = new HashMap(); ResultsWriter resultWriter = mock(ResultsWriter.class); @@ -89,26 +99,37 @@ public ResultsWriter start() RuntimeException embeddedException = new RuntimeException(embeddedExceptionMessage); RuntimeException topException = new RuntimeException("Where's the party?", embeddedException); - @SuppressWarnings("unchecked") - QueryResponse queryResponse = mock(QueryResponse.class); - @SuppressWarnings("unchecked") - Sequence results = mock(Sequence.class); - AsyncContext asyncContext = mock(AsyncContext.class); - ServletResponse response = mock(HttpServletResponse.class); + expect(resultWriter.start()).andThrow(topException); - when(resultWriter.getQueryResponse()).thenReturn(queryResponse); - when(queryResponse.getResults()).thenReturn(results); - when(request.startAsync()).thenReturn(asyncContext); - when(asyncContext.getResponse()).thenReturn(response); - when(results.accumulate(any(), any())).thenThrow(topException); + resultWriter.recordFailure(exceptionBacktraceMessageContains(embeddedExceptionMessage)); + expectLastCall(); + resultWriter.close(); + expectLastCall(); + + replay(resultWriter); // run pusher pusher.push(); - verify(resultWriter) - .recordFailure(argThat(e -> Throwables.getStackTraceAsString(e).contains(embeddedExceptionMessage))); - verify(loggedExceptions) - .add(argThat(e -> Throwables.getStackTraceAsString(e).contains(embeddedExceptionMessage))); + verify(resultWriter); + } + private Exception exceptionBacktraceMessageContains(final String message) + { + reportMatcher(new IArgumentMatcher() + { + @Override + public boolean matches(Object argument) + { + return Throwables.getStackTraceAsString((Throwable) argument).contains(message); + } + + @Override + public void appendTo(StringBuffer buffer) + { + buffer.append("exceptionBacktraceMessageContains(\"" + message + "\")"); + } + }); + return null; } } From f5d94dfc7c9e298d646427e5020c14349a670257 Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Thu, 14 Sep 2023 14:49:15 +0000 Subject: [PATCH 17/21] remove static import --- .../test/java/org/apache/druid/server/QueryResourceTest.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/server/src/test/java/org/apache/druid/server/QueryResourceTest.java b/server/src/test/java/org/apache/druid/server/QueryResourceTest.java index bf8ecc1fadeb..6b0a2720d9d1 100644 --- a/server/src/test/java/org/apache/druid/server/QueryResourceTest.java +++ b/server/src/test/java/org/apache/druid/server/QueryResourceTest.java @@ -108,8 +108,6 @@ import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; -import static org.junit.Assert.assertTrue; - public class QueryResourceTest { private static final QueryToolChestWarehouse WAREHOUSE = new MapQueryToolChestWarehouse(ImmutableMap.of()); @@ -431,7 +429,7 @@ public QueryLifecycle factorize() @Override public void emitLogsAndMetrics(@Nullable Throwable e, @Nullable String remoteAddress, long bytesWritten) { - assertTrue(Throwables.getStackTraceAsString(e).contains(embeddedExceptionMessage)); + Assert.assertTrue(Throwables.getStackTraceAsString(e).contains(embeddedExceptionMessage)); } }; } From c86fe5cb5a2de69437ac69a6995658e7d1d4410b Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Thu, 14 Sep 2023 15:23:52 +0000 Subject: [PATCH 18/21] update test --- .../druid/server/QueryResultPusherTest.java | 92 ++++++++++--------- 1 file changed, 47 insertions(+), 45 deletions(-) diff --git a/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java b/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java index 0e0a2306cafc..7bcc1455c37f 100644 --- a/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java +++ b/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java @@ -23,25 +23,22 @@ import com.google.common.base.Throwables; import org.apache.druid.jackson.DefaultObjectMapper; import org.apache.druid.server.QueryResultPusher.ResultsWriter; +import org.apache.druid.server.QueryResultPusher.Writer; import org.apache.druid.server.mocks.MockHttpServletRequest; -import org.easymock.IArgumentMatcher; import org.junit.Test; import javax.servlet.http.HttpServletRequest; import javax.ws.rs.core.MediaType; +import javax.ws.rs.core.Response.ResponseBuilder; import java.io.IOException; import java.io.OutputStream; import java.util.HashMap; -import java.util.List; import java.util.Map; +import java.util.concurrent.atomic.AtomicBoolean; -import static org.easymock.EasyMock.expect; -import static org.easymock.EasyMock.expectLastCall; import static org.easymock.EasyMock.mock; -import static org.easymock.EasyMock.replay; -import static org.easymock.EasyMock.reportMatcher; -import static org.easymock.EasyMock.verify; +import static org.junit.Assert.assertTrue; public class QueryResultPusherTest { @@ -66,11 +63,50 @@ public void testResultPusherRetainsNestedExceptionBacktraces() throws Exception String queryId = "someQuery"; MediaType contentType = MediaType.APPLICATION_JSON_TYPE; Map extraHeaders = new HashMap(); + AtomicBoolean recordFailureInvoked = new AtomicBoolean(); - ResultsWriter resultWriter = mock(ResultsWriter.class); - @SuppressWarnings("unchecked") - List loggedExceptions = mock(List.class); + String embeddedExceptionMessage = "Embedded Exception Message!"; + RuntimeException embeddedException = new RuntimeException(embeddedExceptionMessage); + RuntimeException topException = new RuntimeException("Where's the party?", embeddedException); + + ResultsWriter resultWriter = new ResultsWriter() + { + + @Override + public void close() throws IOException + { + } + + @Override + public ResponseBuilder start() + { + throw topException; + } + + @Override + public void recordSuccess(long numBytes) + { + } + + @Override + public void recordFailure(Exception e) + { + assertTrue(Throwables.getStackTraceAsString(e).contains(embeddedExceptionMessage)); + recordFailureInvoked.set(true); + } + @Override + public Writer makeWriter(OutputStream out) throws IOException + { + return null; + } + + @Override + public QueryResponse getQueryResponse() + { + return null; + } + }; QueryResultPusher pusher = new QueryResultPusher( request, jsonMapper, @@ -85,7 +121,6 @@ public void testResultPusherRetainsNestedExceptionBacktraces() throws Exception @Override public void writeException(Exception e, OutputStream out) throws IOException { - loggedExceptions.add(e); } @Override @@ -95,41 +130,8 @@ public ResultsWriter start() } }; - String embeddedExceptionMessage = "Embedded Exception Message!"; - RuntimeException embeddedException = new RuntimeException(embeddedExceptionMessage); - RuntimeException topException = new RuntimeException("Where's the party?", embeddedException); - - expect(resultWriter.start()).andThrow(topException); - - resultWriter.recordFailure(exceptionBacktraceMessageContains(embeddedExceptionMessage)); - expectLastCall(); - resultWriter.close(); - expectLastCall(); - - replay(resultWriter); - - // run pusher pusher.push(); - verify(resultWriter); - } - - private Exception exceptionBacktraceMessageContains(final String message) - { - reportMatcher(new IArgumentMatcher() - { - @Override - public boolean matches(Object argument) - { - return Throwables.getStackTraceAsString((Throwable) argument).contains(message); - } - - @Override - public void appendTo(StringBuffer buffer) - { - buffer.append("exceptionBacktraceMessageContains(\"" + message + "\")"); - } - }); - return null; + assertTrue("recordFailure(e) should have been invoked!", recordFailureInvoked.get()); } } From 1f74c8d009848bb894901739d52011c0885a7855 Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Thu, 14 Sep 2023 16:19:15 +0000 Subject: [PATCH 19/21] cleanup --- .../org/apache/druid/server/QueryResultPusherTest.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java b/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java index 7bcc1455c37f..d392f9d298e6 100644 --- a/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java +++ b/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java @@ -31,7 +31,6 @@ import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response.ResponseBuilder; -import java.io.IOException; import java.io.OutputStream; import java.util.HashMap; import java.util.Map; @@ -52,7 +51,7 @@ public class QueryResultPusherTest false); @Test - public void testResultPusherRetainsNestedExceptionBacktraces() throws Exception + public void testResultPusherRetainsNestedExceptionBacktraces() { HttpServletRequest request = new MockHttpServletRequest(); @@ -73,7 +72,7 @@ public void testResultPusherRetainsNestedExceptionBacktraces() throws Exception { @Override - public void close() throws IOException + public void close() { } @@ -96,7 +95,7 @@ public void recordFailure(Exception e) } @Override - public Writer makeWriter(OutputStream out) throws IOException + public Writer makeWriter(OutputStream out) { return null; } @@ -119,7 +118,7 @@ public QueryResponse getQueryResponse() { @Override - public void writeException(Exception e, OutputStream out) throws IOException + public void writeException(Exception e, OutputStream out) { } From ee1e1fa2a4788f57a10ee7eeb49ad5c2f081fcaa Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Fri, 15 Sep 2023 13:59:15 +0000 Subject: [PATCH 20/21] add NoopQueryMetricCounter --- .../druid/server/QueryResultPusherTest.java | 29 +++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java b/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java index d392f9d298e6..5b123b7cc2ac 100644 --- a/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java +++ b/server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java @@ -22,6 +22,7 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.base.Throwables; import org.apache.druid.jackson.DefaultObjectMapper; +import org.apache.druid.server.QueryResource.QueryMetricCounter; import org.apache.druid.server.QueryResultPusher.ResultsWriter; import org.apache.druid.server.QueryResultPusher.Writer; import org.apache.druid.server.mocks.MockHttpServletRequest; @@ -36,7 +37,6 @@ import java.util.Map; import java.util.concurrent.atomic.AtomicBoolean; -import static org.easymock.EasyMock.mock; import static org.junit.Assert.assertTrue; public class QueryResultPusherTest @@ -58,7 +58,7 @@ public void testResultPusherRetainsNestedExceptionBacktraces() ObjectMapper jsonMapper = new DefaultObjectMapper(); ResponseContextConfig responseContextConfig = ResponseContextConfig.newConfig(true); DruidNode selfNode = DRUID_NODE; - QueryResource.QueryMetricCounter counter = mock(QueryResource.QueryMetricCounter.class); + QueryResource.QueryMetricCounter counter = new NoopQueryMetricCounter(); String queryId = "someQuery"; MediaType contentType = MediaType.APPLICATION_JSON_TYPE; Map extraHeaders = new HashMap(); @@ -133,4 +133,29 @@ public ResultsWriter start() assertTrue("recordFailure(e) should have been invoked!", recordFailureInvoked.get()); } + + static class NoopQueryMetricCounter implements QueryMetricCounter + { + + @Override + public void incrementSuccess() + { + } + + @Override + public void incrementFailed() + { + } + + @Override + public void incrementInterrupted() + { + } + + @Override + public void incrementTimedOut() + { + } + + } } From 9791b9d5f54b5bb67992989c0b814360998f2f2d Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Mon, 18 Sep 2023 14:09:21 +0000 Subject: [PATCH 21/21] empty-commit+merge to retrigger ci