Skip to content
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

[PECO-1083] Updated thrift files and added check for protocol version #229

Merged
merged 5 commits into from
Sep 29, 2023

Conversation

nithinkdb
Copy link
Contributor

@nithinkdb nithinkdb commented Sep 26, 2023

Since we've migrated to a new version of runtime that supports parameterized queries, we need to add a protocol check to ensure that the server side is up to date in supporting such queries. Hence, we're updating the thrift files to access the new protocol, and adding a version check when doing parameterized queries.

Copy link
Contributor

@susodapop susodapop left a comment

Choose a reason for hiding this comment

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

Can you please fill out the PR description?

I don't understand how ttypes changed without also changing TCLIService.py. Can you confirm that this is expected?

src/databricks/sql/thrift_backend.py Outdated Show resolved Hide resolved
src/databricks/sql/client.py Outdated Show resolved Hide resolved
Signed-off-by: nithinkdb <[email protected]>
@nithinkdb nithinkdb merged commit 241e934 into databricks:main Sep 29, 2023
2 of 3 checks passed
@susodapop
Copy link
Contributor

susodapop commented Oct 1, 2023

Hey there, did you run the e2e tests for this before merging? They all fail in the case where DBR doesn't return serverProtocolVersion in the session handle, which is the case on the internal test instances we're using today. I suggest we revert and implement with a solution that doesn't cause every single connection to fail, since not all connections will be used for passing parameters.

susodapop pushed a commit that referenced this pull request Oct 1, 2023
susodapop pushed a commit that referenced this pull request Oct 1, 2023
…otocol version (#229)"

This reverts commit 241e934.

Signed-off-by: Jesse Whitehouse <[email protected]>
susodapop pushed a commit that referenced this pull request Oct 2, 2023
…otocol version (#229)"

This reverts commit 241e934.

Signed-off-by: Jesse Whitehouse <[email protected]>
susodapop pushed a commit that referenced this pull request Oct 2, 2023
… version" (#237)

Reverts #229 as it causes all of our e2e tests to fail on some versions of DBR.

We'll reimplement the protocol version check in a follow-up.

This reverts commit 241e934.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants