From 719362525afa91187849e469b3c10c62622e8939 Mon Sep 17 00:00:00 2001 From: Ariel Weisberg Date: Thu, 7 Apr 2022 16:58:17 -0400 Subject: [PATCH] Add concurrent control and test to verifier --- .../framework/AbstractVerification.java | 41 ++++++++++++++----- .../framework/CreateTableVerification.java | 6 ++- .../framework/CreateViewVerification.java | 6 ++- .../verifier/framework/DataVerification.java | 6 ++- .../verifier/framework/DdlVerification.java | 6 ++- .../framework/ExplainVerification.java | 6 ++- .../framework/VerificationFactory.java | 17 ++++++-- .../verifier/framework/VerifierConfig.java | 14 +++++++ .../framework/AbstractVerificationTest.java | 27 +++++++++--- .../framework/TestDataVerification.java | 41 +++++++++++++++++++ .../framework/TestVerifierConfig.java | 7 +++- 11 files changed, 146 insertions(+), 31 deletions(-) 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 b06b49aac0322..d370b1eff01ee 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 @@ -27,10 +27,13 @@ import com.facebook.presto.verifier.prestoaction.QueryActions; import com.facebook.presto.verifier.prestoaction.SqlExceptionClassifier; import com.google.common.collect.ImmutableList; +import com.google.common.util.concurrent.ListenableFuture; +import com.google.common.util.concurrent.ListeningExecutorService; import java.util.List; import java.util.Optional; +import static com.facebook.airlift.concurrent.MoreFutures.getFutureValue; import static com.facebook.presto.spi.StandardErrorCode.EXCEEDED_TIME_LIMIT; import static com.facebook.presto.verifier.event.VerifierQueryEvent.EventStatus.FAILED; import static com.facebook.presto.verifier.event.VerifierQueryEvent.EventStatus.FAILED_RESOLVED; @@ -59,6 +62,7 @@ import static com.google.common.base.Strings.nullToEmpty; import static com.google.common.base.Throwables.getStackTraceAsString; import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.util.concurrent.Futures.immediateFuture; import static java.lang.String.format; import static java.util.Objects.requireNonNull; @@ -72,6 +76,7 @@ public abstract class AbstractVerification> mainQueryResultSetConverter; + private final ListeningExecutorService executor; private final String testId; private final boolean smartTeardown; @@ -81,6 +86,7 @@ public abstract class AbstractVerification> mainQueryResultSetConverter, - VerifierConfig verifierConfig) + VerifierConfig verifierConfig, + ListeningExecutorService executor) { this.queryActions = requireNonNull(queryActions, "queryActions is null"); this.sourceQuery = requireNonNull(sourceQuery, "sourceQuery is null"); this.exceptionClassifier = requireNonNull(exceptionClassifier, "exceptionClassifier is null"); this.verificationContext = requireNonNull(verificationContext, "verificationContext is null"); this.mainQueryResultSetConverter = requireNonNull(mainQueryResultSetConverter, "mainQueryResultSetConverter is null"); + this.executor = requireNonNull(executor, "executor is null"); this.testId = requireNonNull(verifierConfig.getTestId(), "testId is null"); this.smartTeardown = verifierConfig.isSmartTeardown(); @@ -103,6 +111,7 @@ public AbstractVerification( this.teardownOnMainClusters = verifierConfig.isTeardownOnMainClusters(); this.skipControl = verifierConfig.isSkipControl(); this.skipChecksum = verifierConfig.isSkipChecksum(); + this.concurrentControlAndTest = verifierConfig.isConcurrentControlAndTest(); } protected abstract B getQueryRewrite(ClusterType clusterType); @@ -174,7 +183,7 @@ public VerificationResult run() } test = Optional.of(getQueryRewrite(TEST)); - // Run control queries + // First run setup queries if (!skipControl) { QueryBundle controlQueryBundle = control.get(); QueryAction controlSetupAction = setupOnMainClusters ? queryActions.getControlAction() : queryActions.getHelperAction(); @@ -182,21 +191,33 @@ public VerificationResult run() () -> controlSetupAction.execute(query, CONTROL_SETUP), controlQueryContext::addSetupQuery, controlQueryContext::setException)); - controlQueryResult = runMainQuery(controlQueryBundle.getQuery(), CONTROL, controlQueryContext); - controlQueryContext.setState(QueryState.SUCCEEDED); } - else { - controlQueryContext.setState(NOT_RUN); - } - - // Run test queries QueryBundle testQueryBundle = test.get(); QueryAction testSetupAction = setupOnMainClusters ? queryActions.getTestAction() : queryActions.getHelperAction(); testQueryBundle.getSetupQueries().forEach(query -> runAndConsume( () -> testSetupAction.execute(query, TEST_SETUP), testQueryContext::addSetupQuery, testQueryContext::setException)); - testQueryResult = runMainQuery(testQueryBundle.getQuery(), TEST, testQueryContext); + + ListenableFuture>> controlQueryFuture = immediateFuture(Optional.empty()); + // Start control query + if (!skipControl) { + QueryBundle controlQueryBundle = control.get(); + controlQueryFuture = executor.submit(() -> runMainQuery(controlQueryBundle.getQuery(), CONTROL, controlQueryContext)); + } + else { + controlQueryContext.setState(NOT_RUN); + } + + if (!concurrentControlAndTest) { + getFutureValue(controlQueryFuture); + } + + // Run test queries + ListenableFuture>> testQueryFuture = executor.submit(() -> runMainQuery(testQueryBundle.getQuery(), TEST, testQueryContext)); + controlQueryResult = getFutureValue(controlQueryFuture); + controlQueryContext.setState(QueryState.SUCCEEDED); + testQueryResult = getFutureValue(testQueryFuture); testQueryContext.setState(QueryState.SUCCEEDED); // Verify results diff --git a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/CreateTableVerification.java b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/CreateTableVerification.java index e9dc94b1044d3..7bab810e875b7 100644 --- a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/CreateTableVerification.java +++ b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/CreateTableVerification.java @@ -22,6 +22,7 @@ import com.facebook.presto.verifier.prestoaction.QueryActions; import com.facebook.presto.verifier.prestoaction.SqlExceptionClassifier; import com.facebook.presto.verifier.rewrite.QueryRewriter; +import com.google.common.util.concurrent.ListeningExecutorService; import java.sql.SQLException; import java.util.Objects; @@ -52,9 +53,10 @@ public CreateTableVerification( QueryRewriter queryRewriter, SqlExceptionClassifier exceptionClassifier, VerificationContext verificationContext, - VerifierConfig verifierConfig) + VerifierConfig verifierConfig, + ListeningExecutorService executor) { - super(sqlParser, queryActions, sourceQuery, exceptionClassifier, verificationContext, verifierConfig, SHOW_CREATE_TABLE_CONVERTER); + super(sqlParser, queryActions, sourceQuery, exceptionClassifier, verificationContext, verifierConfig, SHOW_CREATE_TABLE_CONVERTER, executor); this.queryRewriter = requireNonNull(queryRewriter, "queryRewriter is null"); } diff --git a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/CreateViewVerification.java b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/CreateViewVerification.java index 8cecba2a6b601..2f82e62db74bb 100644 --- a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/CreateViewVerification.java +++ b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/CreateViewVerification.java @@ -22,6 +22,7 @@ import com.facebook.presto.verifier.prestoaction.QueryActions; import com.facebook.presto.verifier.prestoaction.SqlExceptionClassifier; import com.facebook.presto.verifier.rewrite.QueryRewriter; +import com.google.common.util.concurrent.ListeningExecutorService; import java.sql.SQLException; import java.util.Objects; @@ -52,9 +53,10 @@ public CreateViewVerification( QueryRewriter queryRewriter, SqlExceptionClassifier exceptionClassifier, VerificationContext verificationContext, - VerifierConfig verifierConfig) + VerifierConfig verifierConfig, + ListeningExecutorService executor) { - super(sqlParser, queryActions, sourceQuery, exceptionClassifier, verificationContext, verifierConfig, SHOW_CREATE_VIEW_CONVERTER); + super(sqlParser, queryActions, sourceQuery, exceptionClassifier, verificationContext, verifierConfig, SHOW_CREATE_VIEW_CONVERTER, executor); this.queryRewriter = requireNonNull(queryRewriter, "queryRewriter is null"); } diff --git a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/DataVerification.java b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/DataVerification.java index c52cd807b2a46..24a7c3f35cb2c 100644 --- a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/DataVerification.java +++ b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/DataVerification.java @@ -25,6 +25,7 @@ import com.facebook.presto.verifier.prestoaction.SqlExceptionClassifier; import com.facebook.presto.verifier.resolver.FailureResolverManager; import com.facebook.presto.verifier.rewrite.QueryRewriter; +import com.google.common.util.concurrent.ListeningExecutorService; import java.util.List; import java.util.Optional; @@ -57,9 +58,10 @@ public DataVerification( VerificationContext verificationContext, VerifierConfig verifierConfig, TypeManager typeManager, - ChecksumValidator checksumValidator) + ChecksumValidator checksumValidator, + ListeningExecutorService executor) { - super(queryActions, sourceQuery, exceptionClassifier, verificationContext, Optional.empty(), verifierConfig); + super(queryActions, sourceQuery, exceptionClassifier, verificationContext, Optional.empty(), verifierConfig, executor); this.queryRewriter = requireNonNull(queryRewriter, "queryRewriter is null"); this.determinismAnalyzer = requireNonNull(determinismAnalyzer, "determinismAnalyzer is null"); this.failureResolverManager = requireNonNull(failureResolverManager, "failureResolverManager is null"); diff --git a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/DdlVerification.java b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/DdlVerification.java index aacc7accb11f0..686a8bb42d646 100644 --- a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/DdlVerification.java +++ b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/DdlVerification.java @@ -21,6 +21,7 @@ import com.facebook.presto.verifier.prestoaction.PrestoAction.ResultSetConverter; import com.facebook.presto.verifier.prestoaction.QueryActions; import com.facebook.presto.verifier.prestoaction.SqlExceptionClassifier; +import com.google.common.util.concurrent.ListeningExecutorService; import java.util.Optional; @@ -48,9 +49,10 @@ public DdlVerification( SqlExceptionClassifier exceptionClassifier, VerificationContext verificationContext, VerifierConfig verifierConfig, - ResultSetConverter checksumConverter) + ResultSetConverter checksumConverter, + ListeningExecutorService executor) { - super(queryActions, sourceQuery, exceptionClassifier, verificationContext, Optional.empty(), verifierConfig); + super(queryActions, sourceQuery, exceptionClassifier, verificationContext, Optional.empty(), verifierConfig, executor); this.sqlParser = requireNonNull(sqlParser, "sqlParser"); this.checksumConverter = requireNonNull(checksumConverter, "checksumConverter is null"); } diff --git a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/ExplainVerification.java b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/ExplainVerification.java index 0e35375daf57a..6d266437d17dd 100644 --- a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/ExplainVerification.java +++ b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/ExplainVerification.java @@ -25,6 +25,7 @@ import com.facebook.presto.verifier.prestoaction.QueryActions; import com.facebook.presto.verifier.prestoaction.SqlExceptionClassifier; import com.google.common.collect.ImmutableList; +import com.google.common.util.concurrent.ListeningExecutorService; import java.util.Objects; import java.util.Optional; @@ -53,9 +54,10 @@ public ExplainVerification( SqlExceptionClassifier exceptionClassifier, VerificationContext verificationContext, VerifierConfig verifierConfig, - SqlParser sqlParser) + SqlParser sqlParser, + ListeningExecutorService executor) { - super(queryActions, sourceQuery, exceptionClassifier, verificationContext, Optional.of(QUERY_PLAN_RESULT_SET_CONVERTER), verifierConfig); + super(queryActions, sourceQuery, exceptionClassifier, verificationContext, Optional.of(QUERY_PLAN_RESULT_SET_CONVERTER), verifierConfig, executor); this.sqlParser = requireNonNull(sqlParser, "sqlParser is null"); } diff --git a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/VerificationFactory.java b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/VerificationFactory.java index 70faa1984cb72..921190f8b4082 100644 --- a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/VerificationFactory.java +++ b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/VerificationFactory.java @@ -24,6 +24,8 @@ import com.facebook.presto.verifier.resolver.FailureResolverManagerFactory; import com.facebook.presto.verifier.rewrite.QueryRewriter; import com.facebook.presto.verifier.rewrite.QueryRewriterFactory; +import com.google.common.util.concurrent.ListeningExecutorService; +import com.google.common.util.concurrent.ThreadFactoryBuilder; import javax.inject.Inject; @@ -31,8 +33,10 @@ import static com.facebook.presto.verifier.framework.ClusterType.CONTROL; import static com.facebook.presto.verifier.framework.VerifierUtil.PARSING_OPTIONS; +import static com.google.common.util.concurrent.MoreExecutors.listeningDecorator; import static java.lang.String.format; import static java.util.Objects.requireNonNull; +import static java.util.concurrent.Executors.newCachedThreadPool; public class VerificationFactory { @@ -45,6 +49,7 @@ public class VerificationFactory private final VerifierConfig verifierConfig; private final TypeManager typeManager; private final DeterminismAnalyzerConfig determinismAnalyzerConfig; + private final ListeningExecutorService executor = listeningDecorator(newCachedThreadPool(new ThreadFactoryBuilder().setDaemon(true).setNameFormat("Control/Test query thread - %d").build())); @Inject public VerificationFactory( @@ -83,7 +88,8 @@ public Verification get(SourceQuery sourceQuery, Optional e exceptionClassifier, verificationContext, verifierConfig, - sqlParser); + sqlParser, + executor); } QueryRewriter queryRewriter = queryRewriterFactory.create(queryActions.getHelperAction()); @@ -109,7 +115,8 @@ public Verification get(SourceQuery sourceQuery, Optional e verificationContext, verifierConfig, typeManager, - checksumValidator); + checksumValidator, + executor); case CREATE_VIEW: return new CreateViewVerification( sqlParser, @@ -118,7 +125,8 @@ public Verification get(SourceQuery sourceQuery, Optional e queryRewriter, exceptionClassifier, verificationContext, - verifierConfig); + verifierConfig, + executor); case CREATE_TABLE: return new CreateTableVerification( sqlParser, @@ -127,7 +135,8 @@ public Verification get(SourceQuery sourceQuery, Optional e queryRewriter, exceptionClassifier, verificationContext, - verifierConfig); + verifierConfig, + executor); default: throw new IllegalStateException(format("Unsupported query type: %s", queryType)); } diff --git a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/VerifierConfig.java b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/VerifierConfig.java index 52c9d0e190d5d..847f12f5f76a3 100644 --- a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/VerifierConfig.java +++ b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/VerifierConfig.java @@ -52,6 +52,7 @@ public class VerifierConfig private boolean teardownOnMainClusters = true; private boolean skipControl; private boolean skipChecksum; + private boolean concurrentControlAndTest; private boolean explain; @@ -331,4 +332,17 @@ public VerifierConfig setExplain(boolean explain) this.explain = explain; return this; } + + public boolean isConcurrentControlAndTest() + { + return concurrentControlAndTest; + } + + @ConfigDescription("Run control and test query concurrently") + @Config("concurrent-control-and-test") + public VerifierConfig setConcurrentControlAndTest(boolean concurrentControlAndTest) + { + this.concurrentControlAndTest = concurrentControlAndTest; + return this; + } } diff --git a/presto-verifier/src/test/java/com/facebook/presto/verifier/framework/AbstractVerificationTest.java b/presto-verifier/src/test/java/com/facebook/presto/verifier/framework/AbstractVerificationTest.java index f03fb064cd87c..2675206fa0144 100644 --- a/presto-verifier/src/test/java/com/facebook/presto/verifier/framework/AbstractVerificationTest.java +++ b/presto-verifier/src/test/java/com/facebook/presto/verifier/framework/AbstractVerificationTest.java @@ -105,22 +105,27 @@ protected SourceQuery getSourceQuery(String controlQuery, String testQuery) protected Optional runExplain(String controlQuery, String testQuery) { - return verify(getSourceQuery(controlQuery, testQuery), true, Optional.empty()); + return verify(getSourceQuery(controlQuery, testQuery), true, Optional.empty(), Optional.empty()); } protected Optional runVerification(String controlQuery, String testQuery) { - return verify(getSourceQuery(controlQuery, testQuery), false, Optional.empty()); + return verify(getSourceQuery(controlQuery, testQuery), false, Optional.empty(), Optional.empty()); + } + + protected Optional runVerification(String controlQuery, String testQuery, VerificationSettings settings) + { + return verify(getSourceQuery(controlQuery, testQuery), false, Optional.empty(), Optional.of(settings)); } protected Optional verify(SourceQuery sourceQuery, boolean explain) { - return verify(sourceQuery, explain, Optional.empty()); + return verify(sourceQuery, explain, Optional.empty(), Optional.empty()); } protected Optional verify(SourceQuery sourceQuery, boolean explain, PrestoAction mockPrestoAction) { - return verify(sourceQuery, explain, Optional.of(mockPrestoAction)); + return verify(sourceQuery, explain, Optional.of(mockPrestoAction), Optional.empty()); } protected PrestoAction getPrestoAction(Optional queryConfiguration) @@ -145,9 +150,16 @@ protected PrestoAction getPrestoAction(Optional queryConfigu verifierConfig); } - private Optional verify(SourceQuery sourceQuery, boolean explain, Optional mockPrestoAction) + private Optional verify( + SourceQuery sourceQuery, + boolean explain, + Optional mockPrestoAction, + Optional verificationSettings) { VerifierConfig verifierConfig = new VerifierConfig().setTestId(TEST_ID).setExplain(explain); + verificationSettings.ifPresent(settings -> { + settings.concurrentControlAndTest.ifPresent(verifierConfig::setConcurrentControlAndTest); + }); TypeManager typeManager = createTypeManager(); PrestoAction prestoAction = mockPrestoAction.orElseGet(() -> getPrestoAction(Optional.of(sourceQuery.getControlConfiguration()))); QueryRewriterFactory queryRewriterFactory = new VerificationQueryRewriterFactory( @@ -168,4 +180,9 @@ private Optional verify(SourceQuery sourceQuery, boolean exp determinismAnalyzerConfig); return verificationFactory.get(sourceQuery, Optional.empty()).run().getEvent(); } + + public static class VerificationSettings + { + Optional concurrentControlAndTest; + } } diff --git a/presto-verifier/src/test/java/com/facebook/presto/verifier/framework/TestDataVerification.java b/presto-verifier/src/test/java/com/facebook/presto/verifier/framework/TestDataVerification.java index 7830b4c0aa4fa..e887205123fff 100644 --- a/presto-verifier/src/test/java/com/facebook/presto/verifier/framework/TestDataVerification.java +++ b/presto-verifier/src/test/java/com/facebook/presto/verifier/framework/TestDataVerification.java @@ -19,6 +19,7 @@ import com.facebook.presto.verifier.event.VerifierQueryEvent.EventStatus; import com.google.common.base.Joiner; import com.google.common.collect.ImmutableMap; +import org.testng.annotations.BeforeClass; import org.testng.annotations.Test; import java.util.List; @@ -51,11 +52,20 @@ public class TestDataVerification extends AbstractVerificationTest { + private static VerificationSettings concurrentControlAndTestSettings; + public TestDataVerification() throws Exception { } + @BeforeClass + public static void beforeClass() + { + concurrentControlAndTestSettings = new VerificationSettings(); + concurrentControlAndTestSettings.concurrentControlAndTest = Optional.of(true); + } + @Test public void testSuccess() { @@ -69,6 +79,19 @@ public void testSuccess() assertEvent(event.get(), SUCCEEDED, Optional.empty(), Optional.empty(), Optional.empty()); } + @Test + public void testSuccessConcurrentTestAndControl() + { + Optional event = runVerification("SELECT 1.0", "SELECT 1.00001", concurrentControlAndTestSettings); + assertTrue(event.isPresent()); + assertEvent(event.get(), SUCCEEDED, Optional.empty(), Optional.empty(), Optional.empty()); + + getQueryRunner().execute("CREATE TABLE success_concurrent_test_and_control (x double)"); + event = runVerification("INSERT INTO success_concurrent_test_and_control SELECT 1.0", "INSERT INTO success_concurrent_test_and_control SELECT 1.00001", concurrentControlAndTestSettings); + assertTrue(event.isPresent()); + assertEvent(event.get(), SUCCEEDED, Optional.empty(), Optional.empty(), Optional.empty()); + } + @Test public void testSchemaMismatch() { @@ -100,6 +123,24 @@ public void testRowCountMismatch() "Control 1 rows, Test 2 rows\n")); } + @Test + public void testRowCountMismatchConcurrentControlAndTest() + { + Optional event = runVerification( + "SELECT 1 x", + "SELECT 1 x UNION ALL SELECT 1 x", + concurrentControlAndTestSettings); + assertTrue(event.isPresent()); + assertEvent( + event.get(), + FAILED, + Optional.of(DETERMINISTIC), + Optional.of("ROW_COUNT_MISMATCH"), + Optional.of("Test state SUCCEEDED, Control state SUCCEEDED.\n\n" + + "ROW COUNT MISMATCH\n" + + "Control 1 rows, Test 2 rows\n")); + } + @Test public void testColumnMismatch() { diff --git a/presto-verifier/src/test/java/com/facebook/presto/verifier/framework/TestVerifierConfig.java b/presto-verifier/src/test/java/com/facebook/presto/verifier/framework/TestVerifierConfig.java index e60249de4e214..a75c313f43562 100644 --- a/presto-verifier/src/test/java/com/facebook/presto/verifier/framework/TestVerifierConfig.java +++ b/presto-verifier/src/test/java/com/facebook/presto/verifier/framework/TestVerifierConfig.java @@ -47,7 +47,8 @@ public void testDefault() .setTeardownOnMainClusters(true) .setSkipControl(false) .setSkipChecksum(false) - .setExplain(false)); + .setExplain(false) + .setConcurrentControlAndTest(false)); } @Test @@ -74,6 +75,7 @@ public void testExplicitPropertyMappings() .put("skip-control", "true") .put("skip-checksum", "true") .put("explain", "true") + .put("concurrent-control-and-test", "true") .build(); VerifierConfig expected = new VerifierConfig() .setWhitelist("a,b,c") @@ -95,7 +97,8 @@ public void testExplicitPropertyMappings() .setTeardownOnMainClusters(false) .setSkipControl(true) .setSkipChecksum(true) - .setExplain(true); + .setExplain(true) + .setConcurrentControlAndTest(true); assertFullMapping(properties, expected); }