From d48ae53d0373fcf03bb94d745f5316c4959a08a9 Mon Sep 17 00:00:00 2001 From: Leiqing Cai Date: Mon, 4 Nov 2019 15:23:43 -0800 Subject: [PATCH] Output verifier events for queries being skipped by pre-filters Currently, queries pre-filtered are simply discarded with no output events being generated. This changes allows the total output event count to be more predictable. Filters other than blacklist and whitelist will produce a skipped VerifierQueryEvent. --- .../verifier/event/VerifierQueryEvent.java | 31 +++++++- .../framework/AbstractVerification.java | 8 +- .../verifier/framework/SkippedReason.java | 5 ++ .../framework/VerificationManager.java | 44 +++++++++-- .../framework/TestVerificationManager.java | 79 ++++++++++++++++++- 5 files changed, 150 insertions(+), 17 deletions(-) diff --git a/presto-verifier/src/main/java/com/facebook/presto/verifier/event/VerifierQueryEvent.java b/presto-verifier/src/main/java/com/facebook/presto/verifier/event/VerifierQueryEvent.java index f85cee005cd35..4358ac27dea53 100644 --- a/presto-verifier/src/main/java/com/facebook/presto/verifier/event/VerifierQueryEvent.java +++ b/presto-verifier/src/main/java/com/facebook/presto/verifier/event/VerifierQueryEvent.java @@ -24,6 +24,7 @@ import java.util.List; import java.util.Optional; +import static com.facebook.presto.verifier.event.VerifierQueryEvent.EventStatus.SKIPPED; import static java.util.Objects.requireNonNull; @Immutable @@ -66,8 +67,8 @@ public VerifierQueryEvent( Optional skippedReason, Optional determinismAnalysis, Optional resolveMessage, - QueryInfo controlQueryInfo, - QueryInfo testQueryInfo, + Optional controlQueryInfo, + Optional testQueryInfo, Optional errorCode, Optional errorMessage, Optional finalQueryFailure, @@ -81,14 +82,36 @@ public VerifierQueryEvent( this.deterministic = determinismAnalysis.filter(d -> !d.isUnknown()).map(DeterminismAnalysis::isDeterministic).orElse(null); this.determinismAnalysis = determinismAnalysis.map(DeterminismAnalysis::name).orElse(null); this.resolveMessage = resolveMessage.orElse(null); - this.controlQueryInfo = requireNonNull(controlQueryInfo, "controlQueryInfo is null"); - this.testQueryInfo = requireNonNull(testQueryInfo, "testQueryInfo is null"); + this.controlQueryInfo = controlQueryInfo.orElse(null); + this.testQueryInfo = testQueryInfo.orElse(null); this.errorCode = errorCode.orElse(null); this.errorMessage = errorMessage.orElse(null); this.finalQueryFailure = finalQueryFailure.orElse(null); this.queryFailures = ImmutableList.copyOf(queryFailures); } + public static VerifierQueryEvent skipped( + String suite, + String testId, + String name, + SkippedReason skippedReason) + { + return new VerifierQueryEvent( + suite, + testId, + name, + SKIPPED, + Optional.of(skippedReason), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.empty(), + Optional.empty(), + ImmutableList.of()); + } + @EventField public String getSuite() { diff --git a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/AbstractVerification.java b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/AbstractVerification.java index 02be3d3d268f1..efb789ff8aaf5 100644 --- a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/AbstractVerification.java +++ b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/AbstractVerification.java @@ -270,20 +270,20 @@ else if (skippedReason.isPresent()) { skippedReason, determinismAnalysis, resolveMessage, - buildQueryInfo( + Optional.of(buildQueryInfo( sourceQuery.getControlConfiguration(), sourceQuery.getControlQuery(), verificationResult.map(VerificationResult::getControlChecksumQueryId), verificationResult.map(VerificationResult::getControlChecksumQuery), control, - controlStats), - buildQueryInfo( + controlStats)), + Optional.of(buildQueryInfo( sourceQuery.getTestConfiguration(), sourceQuery.getTestQuery(), verificationResult.map(VerificationResult::getTestChecksumQueryId), verificationResult.map(VerificationResult::getTestChecksumQuery), test, - testStats), + testStats)), errorCode, Optional.ofNullable(errorMessage), queryException.map(QueryException::toQueryFailure), diff --git a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/SkippedReason.java b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/SkippedReason.java index f3f7d9b148ac5..4274db5dad924 100644 --- a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/SkippedReason.java +++ b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/SkippedReason.java @@ -15,6 +15,11 @@ public enum SkippedReason { + SYNTAX_ERROR, + UNSUPPORTED_QUERY_TYPE, + MISMATCHED_QUERY_TYPE, + CUSTOM_FILTER, + CONTROL_QUERY_FAILED, CONTROL_SETUP_QUERY_FAILED, CONTROL_QUERY_TIMED_OUT, diff --git a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/VerificationManager.java b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/VerificationManager.java index 77ea43005b905..2bee415aa4025 100644 --- a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/VerificationManager.java +++ b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/VerificationManager.java @@ -32,7 +32,9 @@ import java.io.Closeable; import java.io.IOException; +import java.util.ArrayList; import java.util.EnumMap; +import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Optional; @@ -50,6 +52,10 @@ import static com.facebook.presto.verifier.event.VerifierQueryEvent.EventStatus.SKIPPED; import static com.facebook.presto.verifier.event.VerifierQueryEvent.EventStatus.SUCCEEDED; import static com.facebook.presto.verifier.framework.QueryType.Category.DATA_PRODUCING; +import static com.facebook.presto.verifier.framework.SkippedReason.CUSTOM_FILTER; +import static com.facebook.presto.verifier.framework.SkippedReason.MISMATCHED_QUERY_TYPE; +import static com.facebook.presto.verifier.framework.SkippedReason.SYNTAX_ERROR; +import static com.facebook.presto.verifier.framework.SkippedReason.UNSUPPORTED_QUERY_TYPE; import static com.facebook.presto.verifier.framework.VerifierUtil.PARSING_OPTIONS; import static com.facebook.presto.verifier.prestoaction.PrestoExceptionClassifier.shouldResubmit; import static com.google.common.collect.ImmutableList.toImmutableList; @@ -71,6 +77,8 @@ public class VerificationManager private final QueryConfigurationOverrides controlOverrides; private final QueryConfigurationOverrides testOverrides; + private final String testId; + private final Optional> whitelist; private final Optional> blacklist; private final int maxConcurrency; @@ -104,6 +112,8 @@ public VerificationManager( this.controlOverrides = requireNonNull(controlOverrides, "controlOverrides is null"); this.testOverrides = requireNonNull(testOverrides, "testOverride is null"); + this.testId = requireNonNull(config.getTestId(), "testId is null"); + this.whitelist = requireNonNull(config.getWhitelist(), "whitelist is null"); this.blacklist = requireNonNull(config.getBlacklist(), "blacklist is null"); this.maxConcurrency = config.getMaxConcurrency(); @@ -219,14 +229,23 @@ private List filterQueryType(List sourceQueries) try { QueryType controlQueryType = QueryType.of(sqlParser.createStatement(sourceQuery.getControlQuery(), PARSING_OPTIONS)); QueryType testQueryType = QueryType.of(sqlParser.createStatement(sourceQuery.getTestQuery(), PARSING_OPTIONS)); - if (controlQueryType == testQueryType && controlQueryType.getCategory() == DATA_PRODUCING) { + + if (controlQueryType != testQueryType) { + postEvent(VerifierQueryEvent.skipped(sourceQuery.getSuite(), testId, sourceQuery.getName(), MISMATCHED_QUERY_TYPE)); + } + else if (controlQueryType.getCategory() != DATA_PRODUCING) { + postEvent(VerifierQueryEvent.skipped(sourceQuery.getSuite(), testId, sourceQuery.getName(), UNSUPPORTED_QUERY_TYPE)); + } + else { selected.add(sourceQuery); } } catch (ParsingException e) { log.warn("Failed to parse query: %s", sourceQuery.getName()); + postEvent(VerifierQueryEvent.skipped(sourceQuery.getSuite(), testId, sourceQuery.getName(), SYNTAX_ERROR)); } catch (UnsupportedQueryTypeException ignored) { + postEvent(VerifierQueryEvent.skipped(sourceQuery.getSuite(), testId, sourceQuery.getName(), UNSUPPORTED_QUERY_TYPE)); } } List selectQueries = selected.build(); @@ -241,10 +260,16 @@ private List applyCustomFilters(List sourceQueries) } log.info("Applying custom query filters"); + sourceQueries = new ArrayList<>(sourceQueries); for (Predicate filter : customQueryFilters) { - sourceQueries = sourceQueries.stream() - .filter(filter::test) - .collect(toImmutableList()); + Iterator iterator = sourceQueries.iterator(); + while (iterator.hasNext()) { + SourceQuery sourceQuery = iterator.next(); + if (!filter.test(sourceQuery)) { + iterator.remove(); + postEvent(VerifierQueryEvent.skipped(sourceQuery.getSuite(), testId, sourceQuery.getName(), CUSTOM_FILTER)); + } + } log.info("Applying custom filter %s... Remaining queries: %s", filter.getClass().getSimpleName(), sourceQueries.size()); } return sourceQueries; @@ -280,9 +305,7 @@ private void reportProgressUntilFinished() } else { statusCount.compute(EventStatus.valueOf(event.get().getStatus()), (status, count) -> count == null ? 1 : count + 1); - for (EventClient eventClient : eventClients) { - eventClient.post(event.get()); - } + postEvent(event.get()); } double progress = ((double) completed) / queriesSubmitted.get() * 100; @@ -306,4 +329,11 @@ private void reportProgressUntilFinished() } } } + + private void postEvent(VerifierQueryEvent event) + { + for (EventClient eventClient : eventClients) { + eventClient.post(event); + } + } } diff --git a/presto-verifier/src/test/java/com/facebook/presto/verifier/framework/TestVerificationManager.java b/presto-verifier/src/test/java/com/facebook/presto/verifier/framework/TestVerificationManager.java index 277cdffc41dcf..71c3866035e81 100644 --- a/presto-verifier/src/test/java/com/facebook/presto/verifier/framework/TestVerificationManager.java +++ b/presto-verifier/src/test/java/com/facebook/presto/verifier/framework/TestVerificationManager.java @@ -13,6 +13,7 @@ */ package com.facebook.presto.verifier.framework; +import com.facebook.airlift.event.client.AbstractEventClient; import com.facebook.airlift.json.JsonCodec; import com.facebook.presto.jdbc.QueryStats; import com.facebook.presto.spi.ErrorCodeSupplier; @@ -25,6 +26,7 @@ import com.facebook.presto.verifier.checksum.FloatingPointColumnValidator; import com.facebook.presto.verifier.checksum.OrderableArrayColumnValidator; import com.facebook.presto.verifier.checksum.SimpleColumnValidator; +import com.facebook.presto.verifier.event.VerifierQueryEvent; import com.facebook.presto.verifier.prestoaction.PrestoAction; import com.facebook.presto.verifier.prestoaction.PrestoResourceClient; import com.facebook.presto.verifier.resolver.FailureResolverConfig; @@ -33,8 +35,10 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; +import java.util.ArrayList; import java.util.List; import java.util.Optional; @@ -42,8 +46,13 @@ import static com.facebook.presto.spi.StandardErrorCode.GENERIC_INTERNAL_ERROR; import static com.facebook.presto.sql.parser.IdentifierSymbol.AT_SIGN; import static com.facebook.presto.sql.parser.IdentifierSymbol.COLON; +import static com.facebook.presto.verifier.event.VerifierQueryEvent.EventStatus.SKIPPED; import static com.facebook.presto.verifier.framework.ClusterType.CONTROL; import static com.facebook.presto.verifier.framework.ClusterType.TEST; +import static com.facebook.presto.verifier.framework.SkippedReason.MISMATCHED_QUERY_TYPE; +import static com.facebook.presto.verifier.framework.SkippedReason.SYNTAX_ERROR; +import static com.facebook.presto.verifier.framework.SkippedReason.UNSUPPORTED_QUERY_TYPE; +import static com.google.common.base.Preconditions.checkArgument; import static java.util.Objects.requireNonNull; import static org.testng.Assert.assertEquals; @@ -85,6 +94,24 @@ public V getJsonResponse(String path, JsonCodec responseCodec) } } + private static class MockEventClient + extends AbstractEventClient + { + private final List events = new ArrayList<>(); + + @Override + protected void postEvent(T event) + { + checkArgument(event instanceof VerifierQueryEvent); + this.events.add((VerifierQueryEvent) event); + } + + public List getEvents() + { + return events; + } + } + private static final String SUITE = "test-suite"; private static final String NAME = "test-query"; private static final QualifiedName TABLE_PREFIX = QualifiedName.of("tmp_verifier"); @@ -99,6 +126,14 @@ public V getJsonResponse(String path, JsonCodec responseCodec) QUERY_CONFIGURATION); private static final VerifierConfig VERIFIER_CONFIG = new VerifierConfig().setTestId("test"); + private MockEventClient eventClient; + + @BeforeMethod + public void setup() + { + this.eventClient = new MockEventClient(); + } + @Test public void testFailureRequeued() { @@ -126,7 +161,47 @@ public void testFailureRequeueDisabled() assertEquals(manager.getQueriesSubmitted().get(), 1); } - private static VerificationManager getVerificationManager(List sourceQueries, PrestoAction prestoAction, VerifierConfig verifierConfig) + @Test + public void testFilters() + { + List queries = ImmutableList.of( + createSourceQuery("q1", "CREATE TABLE t1 (x int)", "CREATE TABLE t1 (x int)"), + createSourceQuery("q2", "CREATE TABLE t1 (x int)", "CREATE TABLE t1 (x int)"), + createSourceQuery("q3", "CREATE TABLE t1 (x int)", "CREATE TABLE t1 (x int)"), + createSourceQuery("q4", "SHOW FUNCTIONS", "SHOW FUNCTIONS"), + createSourceQuery("q5", "SELECT * FROM t1", "INSERT INTO t2 SELECT * FROM t1"), + createSourceQuery("q6", "SELECT * FROM t1", "SELECT FROM t1")); + VerificationManager manager = getVerificationManager( + queries, + new MockPrestoAction(GENERIC_INTERNAL_ERROR), + new VerifierConfig() + .setTestId("test") + .setWhitelist("q2,q3,q4,q5,q6") + .setBlacklist("q2")); + manager.start(); + assertEquals(manager.getQueriesSubmitted().get(), 0); + + List events = eventClient.getEvents(); + assertEquals(events.size(), 4); + assertSkippedEvent(events.get(0), "q3", UNSUPPORTED_QUERY_TYPE); + assertSkippedEvent(events.get(1), "q4", UNSUPPORTED_QUERY_TYPE); + assertSkippedEvent(events.get(2), "q5", MISMATCHED_QUERY_TYPE); + assertSkippedEvent(events.get(3), "q6", SYNTAX_ERROR); + } + + private static SourceQuery createSourceQuery(String name, String controlQuery, String testQuery) + { + return new SourceQuery(SUITE, name, controlQuery, testQuery, QUERY_CONFIGURATION, QUERY_CONFIGURATION); + } + + private static void assertSkippedEvent(VerifierQueryEvent event, String name, SkippedReason skippedReason) + { + assertEquals(event.getName(), name); + assertEquals(event.getStatus(), SKIPPED.name()); + assertEquals(event.getSkippedReason(), skippedReason.name()); + } + + private VerificationManager getVerificationManager(List sourceQueries, PrestoAction prestoAction, VerifierConfig verifierConfig) { return new VerificationManager( () -> sourceQueries, @@ -141,7 +216,7 @@ private static VerificationManager getVerificationManager(List sour new TypeRegistry(), new FailureResolverConfig().setEnabled(false)), SQL_PARSER, - ImmutableSet.of(), + ImmutableSet.of(eventClient), ImmutableList.of(), new QueryConfigurationOverridesConfig(), new QueryConfigurationOverridesConfig(),