Support customizable ways of launching Presto queries in Verifier#14823
Merged
caithagoras merged 10 commits intoprestodb:masterfrom Jul 30, 2020
Merged
Support customizable ways of launching Presto queries in Verifier#14823caithagoras merged 10 commits intoprestodb:masterfrom
caithagoras merged 10 commits intoprestodb:masterfrom
Conversation
caithagoras
force-pushed
the
s1
branch
3 times, most recently
from
3f61b1d to
be26ad5
Compare
caithagoras
changed the title
caithagoras
force-pushed
the
s1
branch
4 times, most recently
from
c150bde to
899498a
Compare
arhimondr
reviewed
Jul 10, 2020
Member
arhimondr
left a comment
arhimondr
left a comment
There was a problem hiding this comment.
Style improvments in DeterminismAnalyzer
LGTM
Member
|
|
arhimondr
reviewed
Jul 10, 2020
Member
arhimondr
left a comment
arhimondr
left a comment
There was a problem hiding this comment.
Make control.http-port optional
LGTM
arhimondr
reviewed
Jul 10, 2020
Member
arhimondr
left a comment
arhimondr
left a comment
There was a problem hiding this comment.
Make the entire JDBC QueryStats available in verification results
LGTM
arhimondr
approved these changes
Jul 10, 2020
Member
There was a problem hiding this comment.
Should it be called JdbcPrestoActionConfig?
caithagoras
force-pushed
the
s1
branch
6 times, most recently
from
0a84b58 to
dabde7b
Compare
Contributor
Author
|
[test facebook] |
caithagoras
force-pushed
the
s1
branch
2 times, most recently
from
8345cf3 to
55727ca
Compare
wenleix
reviewed
Jul 23, 2020
Contributor
wenleix
left a comment
wenleix
left a comment
There was a problem hiding this comment.
"Make control.http-port optional " . LGTM
"Make the entire JDBC QueryStats available in verification results". LGTM. curious what's the motivation here?
Contributor
There was a problem hiding this comment.
wow. why not using OptionalLong/OptionalDouble? -- this is irrelevant to this PR though
Contributor
Author
There was a problem hiding this comment.
Since we have everything in the QueryStatsEvent, we'll eventually deprecate those fields.
wenleix
reviewed
Jul 23, 2020
wenleix
reviewed
Jul 23, 2020
Contributor
wenleix
left a comment
wenleix
left a comment
There was a problem hiding this comment.
LGTM. Is there any document about how to mangling session properties in verifier run? (e.g. tweak some session properties, etc).
wenleix
approved these changes
Jul 23, 2020
caithagoras
force-pushed
the
s1
branch
10 times, most recently
from
985d029 to
753c4f7
Compare
Make fields final and remove unused fields.
http-port is used for fetching cluster size via /v1/node. We only need it when TooManyOpenPartitionsFailureResolver requires the test cluster size. Hence: - Make control.http-port optional - Make test.http-port required if the specific failure resolver is enabled, else optional. Changes also needed in this commit: - Convert NodeResourceClient to ClusterSizeSupplier. - Convert HttpNodeResourceClient to ClusterSizeFetcher.
There are use cases to launch control and test Presto queries through a different way other than JDBC. This commit provides a way to write customized QueryAction and plug it into the Verifer. - Introduce QueryAction-related interfaces. - Support specifying a "helper" cluster, a cluster to run helper queries (describe, checksum, etc.). By default, help cluster is the control cluster. If the control QueryAction does not support data fetching, a separate helper cluster must be specified. - Replace RoutingPrestoAction with QueryActions. - Add an option to specify whether to run setup/teardown queries on the control/test clusters or on the helper cluster.
caithagoras
force-pushed
the
s1
branch
2 times, most recently
from
043832a to
3f99da3
Compare
metadata-timeout and checksum-timeout can be defined for control cluster and test cluster separately. Now that we have 3 different cluster types, this could be confusing. Move both config to QueryActionsConfig so that there is only one top level config for metadata-timeout and checksum-timeout.
Also, add a constructor in QueryException to take message
Contributor
Author
|
[test facebook] |
7 tasks
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Depended by https://github.com/facebookexternal/presto-facebook/pull/1081