Yaml test option to force continuations #3075
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…from "set to 0", so we can skip the forceContinuations option if maxRows explicitly set to 0
Result of fdb-record-layer-pr on Linux CentOS 7
|
…nConfig is required for assertions
Result of fdb-record-layer-pr on Linux CentOS 7
|
Result of fdb-record-layer-pr on Linux CentOS 7
|
ScottDugas
requested changes
Feb 3, 2025
| import java.util.Optional; | ||
| import java.util.function.Supplier; | ||
|
|
||
| @SuppressWarnings({"PMD.GuardLogStatement"}) // It already is, but PMD is confused and reporting error in unrelated locations. | ||
| public final class YamlExecutionContext { | ||
| public static final String OPTION_FORCE_CONTINUATIONS = "optionForceContinuations"; |
There was a problem hiding this comment.
Would it make sense to allow the compiler to do more protecting here, and have an object with setters, e.g.
new Options().setForceContinuations(true)
Or do you see some reason that this may have to pass options along that it is unaware of?
There was a problem hiding this comment.
There is a pattern that we can use to enforce types by creating a generic-typed enum constants. Let me see if it creates a better experience.
There was a problem hiding this comment.
I'll get that done in a followup in an attempt to get this PR in sooner to get coverage increase
yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/command/QueryExecutor.java
Outdated
Show resolved
Hide resolved
yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/command/QueryExecutor.java
Outdated
Show resolved
Hide resolved
yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/command/QueryExecutor.java
Outdated
Show resolved
Hide resolved
yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/command/QueryExecutor.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| @Disabled("Infinite continuation loop") |
There was a problem hiding this comment.
Instead of just comments, should these reference issues?
yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/AggregateResultSet.java
Outdated
Show resolved
Hide resolved
...ts/src/main/java/com/apple/foundationdb/relational/yamltests/AbstractAggregateResultSet.java
Outdated
Show resolved
Hide resolved
...ts/src/main/java/com/apple/foundationdb/relational/yamltests/AbstractAggregateResultSet.java
Outdated
Show resolved
Hide resolved
ScottDugas
requested changes
Feb 4, 2025
yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/command/QueryExecutor.java
Outdated
Show resolved
Hide resolved
yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/command/QueryExecutor.java
Show resolved
Hide resolved
yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/AggregateResultSet.java
Show resolved
Hide resolved
ScottDugas
approved these changes
Feb 5, 2025
This was referenced Feb 10, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds the ability to force
maxRows:1onto YAML tests that do not otherwise specify the maxRows. The forced maxRows is set to 1, which means that every test that has more than 0 rows back will result in a continuation being issued and another request being made. The tests will be run in a separate and independent run, not impacting any of the existing coverage. The existing assertions will be used without modification to verify the results of the modified tests: The result set returned to the Matcher is an aggregate result set that contains all the "one liner" result sets returned from the query requests.Another new feature is the ability to distinguish between
marrows:0and not havingmaxRowsspecified. The former will prevent the forcing of continuations while maintaining the "no limit" semantics. The latter will use the default "no limit" normally but will allow the forcing of continuations when running in that mode.