Skip to content

Populate ClientInfo in Verifier JDBC queries#14941

Merged
caithagoras merged 2 commits intoprestodb:masterfrom
caithagoras:s4
Aug 3, 2020
Merged

Populate ClientInfo in Verifier JDBC queries#14941
caithagoras merged 2 commits intoprestodb:masterfrom
caithagoras:s4

Conversation

@caithagoras
Copy link
Contributor

@caithagoras caithagoras commented Jul 31, 2020 •
edited
Loading 

== RELEASE NOTES ==

Verifier Changes
* Add support to populate client info for the queries issued by Verifier.
* Add configuration property ``test_name``, to be passed in to the client info blob.

wenleix
wenleix approved these changes Jul 31, 2020
View reviewed changes
Copy link
Contributor

@wenleix wenleix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

presto-verifier/src/main/java/com/facebook/presto/verifier/framework/VerificationContext.java Outdated
Copy link
Contributor

@wenleix wenleix Jul 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just double check: is this the original query id? :)

Copy link
Contributor Author

@caithagoras caithagoras Jul 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theoretically this could be anything provided in the xdb, but yes it is the original query ID for FB use cases.

@caithagoras caithagoras force-pushed the s4 branch 3 times, most recently from 0629b27 to 96717cc Compare August 1, 2020 00:14
@caithagoras
Copy link
Contributor Author

caithagoras commented Aug 1, 2020

@wenleix Added a 2nd commit: Introduce config property test-name, to be passed into client info

wenleix
wenleix approved these changes Aug 1, 2020
View reviewed changes
Copy link
Contributor

@wenleix wenleix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even just populating the suite name is good name -- for example I can just make sure all my suite name starting with something like "wxie_jumbo_pos_XXX". But a specific test name is definitely even better. Thanks @caithagoras !

wenleix
wenleix reviewed Aug 1, 2020
View reviewed changes
presto-verifier/src/main/java/com/facebook/presto/verifier/prestoaction/ClientInfo.java

public String serialize() {
try {
return new ObjectMapper().writeValueAsString(this);
Copy link
Contributor

@wenleix wenleix Aug 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A common pattern in Presto code base is to have a private static final OBJECT_MAPPER in this class 😃 . But either way is fine :)

Copy link
Contributor Author

@caithagoras caithagoras Aug 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I missed this comment, will piggy-back into a future PR.

@caithagoras caithagoras merged commit 8e272aa into prestodb:master Aug 3, 2020
@caithagoras caithagoras deleted the s4 branch August 3, 2020 18:47
@caithagoras caithagoras mentioned this pull request Aug 14, 2020
Merged
7 tasks
@caithagoras caithagoras mentioned this pull request Aug 25, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aweisberg aweisberg Awaiting requested review from aweisberg

@arhimondr arhimondr Awaiting requested review from arhimondr

1 more reviewer

@wenleix wenleix wenleix approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@caithagoras @wenleix @caithagoras0