Skip to content

Support skipping control in Verifier #14958

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
caithagoras merged 1 commit into from
Aug 5, 2020
Merged

Support skipping control in Verifier #14958

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions presto-verifier/src/main/java/com/facebook/presto/verifier/event/VerifierQueryEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ public VerifierQueryEvent(
Optional<DeterminismAnalysis> determinismAnalysis,
Optional<DeterminismAnalysisDetails> determinismAnalysisDetails,
Optional<String> resolveMessage,
QueryInfo controlQueryInfo,
Optional<QueryInfo> controlQueryInfo,
QueryInfo testQueryInfo,
Optional<String> errorCode,
Optional<String> errorMessage,
Expand All @@ -89,7 +89,7 @@ public VerifierQueryEvent(
this.determinismAnalysis = determinismAnalysis.map(DeterminismAnalysis::name).orElse(null);
this.determinismAnalysisDetails = determinismAnalysisDetails.orElse(null);
this.resolveMessage = resolveMessage.orElse(null);
this.controlQueryInfo = requireNonNull(controlQueryInfo, "controlQueryInfo is null");
this.controlQueryInfo = controlQueryInfo.orElse(null);
this.testQueryInfo = requireNonNull(testQueryInfo, "testQueryInfo is null");
this.errorCode = errorCode.orElse(null);
this.errorMessage = errorMessage.orElse(null);
Expand All @@ -102,7 +102,8 @@ public static VerifierQueryEvent skipped(
String suite,
String testId,
SourceQuery sourceQuery,
SkippedReason skippedReason)
SkippedReason skippedReason,
boolean skipControl)
{
return new VerifierQueryEvent(
suite,
Expand All @@ -113,10 +114,12 @@ public static VerifierQueryEvent skipped(
Optional.empty(),
Optional.empty(),
Optional.empty(),
new QueryInfo(
sourceQuery.getControlConfiguration().getCatalog(),
sourceQuery.getControlConfiguration().getSchema(),
sourceQuery.getControlQuery()),
skipControl ?
Optional.empty() :
Optional.of(new QueryInfo(
sourceQuery.getControlConfiguration().getCatalog(),
sourceQuery.getControlConfiguration().getSchema(),
sourceQuery.getControlQuery())),
new QueryInfo(
sourceQuery.getTestConfiguration().getCatalog(),
sourceQuery.getTestConfiguration().getSchema(),
Expand Down
105 changes: 63 additions & 42 deletions ...o-verifier/src/main/java/com/facebook/presto/verifier/framework/AbstractVerification.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ public abstract class AbstractVerification

private final boolean setupOnMainClusters;
private final boolean teardownOnMainClusters;
private final boolean skipControl;

public AbstractVerification(
QueryActions queryActions,
Expand All @@ -106,6 +107,7 @@ public AbstractVerification(
this.verificationResubmissionLimit = verifierConfig.getVerificationResubmissionLimit();
this.setupOnMainClusters = verifierConfig.isSetupOnMainClusters();
this.teardownOnMainClusters = verifierConfig.isTeardownOnMainClusters();
this.skipControl = verifierConfig.isSkipControl();
}

protected abstract MatchResult verify(QueryBundle control, QueryBundle test, ChecksumQueryContext controlContext, ChecksumQueryContext testContext);
Expand Down Expand Up @@ -145,25 +147,32 @@ public VerificationResult run()

try {
// Rewrite queries
control = Optional.of(queryRewriter.rewriteQuery(sourceQuery.getControlQuery(), CONTROL));
if (!skipControl) {
control = Optional.of(queryRewriter.rewriteQuery(sourceQuery.getControlQuery(), CONTROL));
}
test = Optional.of(queryRewriter.rewriteQuery(sourceQuery.getTestQuery(), TEST));

// Run queries
QueryBundle controlQueryBundle = control.get();
QueryBundle testQueryBundle = test.get();
// Run control queries
if (skipControl) {
controlQueryContext.setState(NOT_RUN);
}
else {
QueryBundle controlQueryBundle = control.get();
QueryAction controlSetupAction = setupOnMainClusters ? queryActions.getControlAction() : queryActions.getHelperAction();
controlQueryBundle.getSetupQueries().forEach(query -> runAndConsume(
() -> controlSetupAction.execute(query, CONTROL_SETUP),
controlQueryContext::addSetupQuery,
controlQueryContext::setException));
runAndConsume(
() -> queryActions.getControlAction().execute(controlQueryBundle.getQuery(), CONTROL_MAIN),
controlQueryContext::setMainQueryStats,
controlQueryContext::setException);
controlQueryContext.setState(QueryState.SUCCEEDED);
}

QueryAction controlSetupAction = setupOnMainClusters ? queryActions.getControlAction() : queryActions.getHelperAction();
// Run test queries
QueryBundle testQueryBundle = test.get();
QueryAction testSetupAction = setupOnMainClusters ? queryActions.getTestAction() : queryActions.getHelperAction();
controlQueryBundle.getSetupQueries().forEach(query -> runAndConsume(
() -> controlSetupAction.execute(query, CONTROL_SETUP),
controlQueryContext::addSetupQuery,
controlQueryContext::setException));
runAndConsume(
() -> queryActions.getControlAction().execute(controlQueryBundle.getQuery(), CONTROL_MAIN),
controlQueryContext::setMainQueryStats,
controlQueryContext::setException);
controlQueryContext.setState(QueryState.SUCCEEDED);

testQueryBundle.getSetupQueries().forEach(query -> runAndConsume(
() -> testSetupAction.execute(query, TEST_SETUP),
testQueryContext::addSetupQuery,
Expand All @@ -175,22 +184,24 @@ public VerificationResult run()
testQueryContext.setState(QueryState.SUCCEEDED);

// Verify results
matchResult = Optional.of(verify(controlQueryBundle, test.get(), controlChecksumQueryContext, testChecksumQueryContext));
if (!skipControl) {
matchResult = Optional.of(verify(control.get(), test.get(), controlChecksumQueryContext, testChecksumQueryContext));

// Determinism analysis
if (matchResult.get().isMismatchPossiblyCausedByNonDeterminism()) {
determinismAnalysis = Optional.of(determinismAnalyzer.analyze(controlQueryBundle, matchResult.get().getControlChecksum(), determinismAnalysisDetails));
// Determinism analysis
if (matchResult.get().isMismatchPossiblyCausedByNonDeterminism()) {
determinismAnalysis = Optional.of(determinismAnalyzer.analyze(control.get(), matchResult.get().getControlChecksum(), determinismAnalysisDetails));
}
}

partialResult = Optional.of(concludeVerificationPartial(control, test, controlQueryContext, matchResult, determinismAnalysis, Optional.empty()));
partialResult = Optional.of(concludeVerificationPartial(control, test, controlQueryContext, testQueryContext, matchResult, determinismAnalysis, Optional.empty()));
}
catch (Throwable t) {
if (exceptionClassifier.shouldResubmit(t)
&& verificationContext.getResubmissionCount() < verificationResubmissionLimit) {
return new VerificationResult(this, true, Optional.empty());
}
throwable = Optional.of(t);
partialResult = Optional.of(concludeVerificationPartial(control, test, controlQueryContext, matchResult, determinismAnalysis, Optional.of(t)));
partialResult = Optional.of(concludeVerificationPartial(control, test, controlQueryContext, testQueryContext, matchResult, determinismAnalysis, Optional.of(t)));
}
finally {
if (!smartTeardown
Expand All @@ -203,18 +214,20 @@ public VerificationResult run()
}
}

return concludeVerification(
partialResult.get(),
control,
test,
controlQueryContext,
testQueryContext,
matchResult,
determinismAnalysis,
controlChecksumQueryContext,
testChecksumQueryContext,
determinismAnalysisDetails.build(),
throwable);
return

concludeVerification(
partialResult.get(),
control,
test,
controlQueryContext,
testQueryContext,
matchResult,
determinismAnalysis,
controlChecksumQueryContext,
testChecksumQueryContext,
determinismAnalysisDetails.build(),
throwable);
}

private Optional<String> resolveFailure(
Expand All @@ -238,15 +251,17 @@ private Optional<String> resolveFailure(
private EventStatus getEventStatus(
Optional<SkippedReason> skippedReason,
Optional<String> resolveMessage,
Optional<MatchResult> matchResult)
Optional<MatchResult> matchResult,
QueryContext testQueryContext)
{
if (skippedReason.isPresent()) {
return SKIPPED;
}
if (resolveMessage.isPresent()) {
return FAILED_RESOLVED;
}
if (matchResult.isPresent() && matchResult.get().isMatched()) {
if ((skipControl && testQueryContext.getState() == QueryState.SUCCEEDED)
|| (!skipControl && matchResult.isPresent() && matchResult.get().isMatched())) {
return SUCCEEDED;
}
return FAILED;
Expand All @@ -256,13 +271,14 @@ private PartialVerificationResult concludeVerificationPartial(
Optional<QueryBundle> control,
Optional<QueryBundle> test,
QueryContext controlQueryContext,
QueryContext testQueryContext,
Optional<MatchResult> matchResult,
Optional<DeterminismAnalysis> determinismAnalysis,
Optional<Throwable> throwable)
{
Optional<SkippedReason> skippedReason = getSkippedReason(throwable, controlQueryContext.getState(), determinismAnalysis);
Optional<String> resolveMessage = resolveFailure(control, test, controlQueryContext, matchResult, throwable);
EventStatus status = getEventStatus(skippedReason, resolveMessage, matchResult);
EventStatus status = getEventStatus(skippedReason, resolveMessage, matchResult, testQueryContext);
return new PartialVerificationResult(skippedReason, resolveMessage, status);
}

Expand Down Expand Up @@ -300,12 +316,14 @@ private VerificationResult concludeVerification(
Optional.of(determinismAnalysisDetails) :
Optional.empty(),
partialResult.getResolveMessage(),
buildQueryInfo(
sourceQuery.getControlConfiguration(),
sourceQuery.getControlQuery(),
controlChecksumQueryContext,
control,
controlQueryContext),
skipControl ?
Optional.empty() :
Optional.of(buildQueryInfo(
sourceQuery.getControlConfiguration(),
sourceQuery.getControlQuery(),
controlChecksumQueryContext,
control,
controlQueryContext)),
buildQueryInfo(
sourceQuery.getTestConfiguration(),
sourceQuery.getTestQuery(),
Expand Down Expand Up @@ -360,6 +378,9 @@ private Optional<SkippedReason> getSkippedReason(Optional<Throwable> throwable,
if (throwable.isPresent() && !(throwable.get() instanceof QueryException)) {
return Optional.of(VERIFIER_INTERNAL_ERROR);
}
if (skipControl) {
return Optional.empty();
}
switch (controlState) {
case FAILED:
return Optional.of(CONTROL_QUERY_FAILED);
Expand Down
12 changes: 7 additions & 5 deletions ...to-verifier/src/main/java/com/facebook/presto/verifier/framework/VerificationManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ public class VerificationManager
private final int suiteRepetitions;
private final int queryRepetitions;
private int verificationResubmissionLimit;
private boolean skipControl;

private ExecutorService executor;
private CompletionService<VerificationResult> completionService;
Expand Down Expand Up @@ -115,6 +116,7 @@ public VerificationManager(
this.suiteRepetitions = config.getSuiteRepetitions();
this.queryRepetitions = config.getQueryRepetitions();
this.verificationResubmissionLimit = config.getVerificationResubmissionLimit();
this.skipControl = config.isSkipControl();
}

@PostConstruct
Expand Down Expand Up @@ -213,21 +215,21 @@ private List<SourceQuery> filterQueryType(List<SourceQuery> sourceQueries)
QueryType testQueryType = QueryType.of(sqlParser.createStatement(sourceQuery.getTestQuery(), PARSING_OPTIONS));

if (controlQueryType != testQueryType) {
postEvent(VerifierQueryEvent.skipped(sourceQuery.getSuite(), testId, sourceQuery, MISMATCHED_QUERY_TYPE));
postEvent(VerifierQueryEvent.skipped(sourceQuery.getSuite(), testId, sourceQuery, MISMATCHED_QUERY_TYPE, skipControl));
}
else if (controlQueryType.getCategory() != DATA_PRODUCING) {
postEvent(VerifierQueryEvent.skipped(sourceQuery.getSuite(), testId, sourceQuery, UNSUPPORTED_QUERY_TYPE));
postEvent(VerifierQueryEvent.skipped(sourceQuery.getSuite(), testId, sourceQuery, UNSUPPORTED_QUERY_TYPE, skipControl));
}
else {
selected.add(sourceQuery);
}
}
catch (ParsingException e) {
log.warn("Failed to parse query: %s", sourceQuery.getName());
postEvent(VerifierQueryEvent.skipped(sourceQuery.getSuite(), testId, sourceQuery, SYNTAX_ERROR));
postEvent(VerifierQueryEvent.skipped(sourceQuery.getSuite(), testId, sourceQuery, SYNTAX_ERROR, skipControl));
}
catch (UnsupportedQueryTypeException ignored) {
postEvent(VerifierQueryEvent.skipped(sourceQuery.getSuite(), testId, sourceQuery, UNSUPPORTED_QUERY_TYPE));
postEvent(VerifierQueryEvent.skipped(sourceQuery.getSuite(), testId, sourceQuery, UNSUPPORTED_QUERY_TYPE, skipControl));
}
}
List<SourceQuery> selectQueries = selected.build();
Expand All @@ -249,7 +251,7 @@ private List<SourceQuery> applyCustomFilters(List<SourceQuery> sourceQueries)
SourceQuery sourceQuery = iterator.next();
if (!filter.test(sourceQuery)) {
iterator.remove();
postEvent(VerifierQueryEvent.skipped(sourceQuery.getSuite(), testId, sourceQuery, CUSTOM_FILTER));
postEvent(VerifierQueryEvent.skipped(sourceQuery.getSuite(), testId, sourceQuery, CUSTOM_FILTER, skipControl));
}
}
log.info("Applying custom filter %s... Remaining queries: %s", filter.getClass().getSimpleName(), sourceQueries.size());
Expand Down
14 changes: 14 additions & 0 deletions presto-verifier/src/main/java/com/facebook/presto/verifier/framework/VerifierConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ public class VerifierConfig

private boolean setupOnMainClusters = true;
private boolean teardownOnMainClusters = true;
private boolean skipControl;

@NotNull
public Optional<Set<String>> getWhitelist()
Expand Down Expand Up @@ -288,4 +289,17 @@ public VerifierConfig setTeardownOnMainClusters(boolean teardownOnMainClusters)
this.teardownOnMainClusters = teardownOnMainClusters;
return this;
}

public boolean isSkipControl()
{
return skipControl;
}

@ConfigDescription("Skip control queries and result comparison, only run test queries.")
@Config("skip-control")
public VerifierConfig setSkipControl(boolean skipControl)
{
this.skipControl = skipControl;
return this;
}
}
7 changes: 5 additions & 2 deletions presto-verifier/src/test/java/com/facebook/presto/verifier/framework/TestVerifierConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ public void testDefault()
.setSmartTeardown(false)
.setVerificationResubmissionLimit(2)
.setSetupOnMainClusters(true)
.setTeardownOnMainClusters(true));
.setTeardownOnMainClusters(true)
.setSkipControl(false));
}

@Test
Expand All @@ -68,6 +69,7 @@ public void testExplicitPropertyMappings()
.put("verification-resubmission.limit", "1")
.put("setup-on-main-clusters", "false")
.put("teardown-on-main-clusters", "false")
.put("skip-control", "true")
.build();
VerifierConfig expected = new VerifierConfig()
.setWhitelist("a,b,c")
Expand All @@ -86,7 +88,8 @@ public void testExplicitPropertyMappings()
.setSmartTeardown(true)
.setVerificationResubmissionLimit(1)
.setSetupOnMainClusters(false)
.setTeardownOnMainClusters(false);
.setTeardownOnMainClusters(false)
.setSkipControl(true);

assertFullMapping(properties, expected);
}
Expand Down