-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
Extend QueryManager
query type
#9874
Conversation
(cherry picked from commit 7c33f22)
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a newcomer here, but .. it seems odd to me to add this possibility to the validation checks, without a corresponding change to handle the case of query being a mapping. Has support for that case already been merged?
Co-authored-by: Dustin J. Mitchell <[email protected]>
@djmitche The
QueryManager may want to pass queries with additional context to the executor; in particular we want to use this for a future check (IBM i) where the executor also gets a timeout for each query. Existing users of QueryManager would keep getting just queries (strings) and would not need to support this.
Instead of doing this, we could pass the executor argument type as a new |
I think this is OK as-is. As you've said, it will allow for future support for mappings, and in the interim only increases the set of valid configs. Thanks for the explanation! |
Co-authored-by: Ofek Lev <[email protected]>
eb15705
to
6a88158
Compare
* Add 'timeout' field to QueryManager queries (cherry picked from commit 7c33f22) * Apply suggestions from code review Co-authored-by: Dustin J. Mitchell <[email protected]> * Address review comment Co-authored-by: Ofek Lev <[email protected]> Co-authored-by: Dustin J. Mitchell <[email protected]> Co-authored-by: Ofek Lev <[email protected]> 36dfad0
Revert "Add 'timeout' field to QueryManager queries" This reverts commit 7c33f22. Extend `QueryManager` query type (#9874) * Add 'timeout' field to QueryManager queries (cherry picked from commit 7c33f22) * Apply suggestions from code review Co-authored-by: Dustin J. Mitchell <[email protected]> * Address review comment Co-authored-by: Ofek Lev <[email protected]> Co-authored-by: Dustin J. Mitchell <[email protected]> Co-authored-by: Ofek Lev <[email protected]> (cherry picked from commit 36dfad0)
Revert "Add 'timeout' field to QueryManager queries" This reverts commit 7c33f22. Extend `QueryManager` query type (#9874) * Add 'timeout' field to QueryManager queries (cherry picked from commit 7c33f22) * Apply suggestions from code review Co-authored-by: Dustin J. Mitchell <[email protected]> * Address review comment Co-authored-by: Ofek Lev <[email protected]> Co-authored-by: Dustin J. Mitchell <[email protected]> Co-authored-by: Ofek Lev <[email protected]> (cherry picked from commit 36dfad0)
What does this PR do?
Extends
QueryManager
query type to allow mappings.Motivation
Needed for IBM i integration, see #9720.
Additional Notes
As of now it should be a no-op.
Review checklist (to be filled by reviewers)
changelog/
andintegration/
labels attached