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

Fix proxy connection pool creation #158

Merged
merged 4 commits into from
Jul 12, 2023
Merged

Conversation

sebbegg
Copy link
Contributor

@sebbegg sebbegg commented Jun 23, 2023

Stumbled across this while trying to use the connection from within a proxy environment.

As far as I can tell the proxy cool creation is passed the wrong host/port and was missing the https scheme.
Without the proper schema urllib won't do a proper CONNECT call and the requests later on will fail.
It appears realhost and realport in the THttpClient were not used anywhere, but were probably intended for the proxy use-case?

Tested this locally against our databricks subscription.

Sebastian Eckweiler (sebastian.eckweiler@mercedes-benz.com), Mercedes-Benz AG on behalf of MBition GmbH (Imprint)

Signed-off-by: Sebastian Eckweiler <sebastian.eckweiler@mercedes-benz.com>
Signed-off-by: Sebastian Eckweiler <sebastian.eckweiler@mercedes-benz.com>
@danigian
Copy link

@susodapop please give the proper love to this PR.
We were having issues with latest 2.7.0 and I can confirm that when using a proxy, the library requires the changes in this PR 👍

Thanks @sebbegg for coming up with this

@susodapop
Copy link
Contributor

susodapop commented Jun 27, 2023 via email

@marcgs
Copy link

marcgs commented Jul 10, 2023

Any update when this fix will be released @susodapop ? We are facing the same issue and are working with a patched version with @sebbegg 's fix ATM.

@susodapop
Copy link
Contributor

susodapop commented Jul 10, 2023 via email

Jesse Whitehouse added 2 commits July 11, 2023 19:32
Signed-off-by: Jesse Whitehouse <jesse.whitehouse@databricks.com>
Signed-off-by: Jesse Whitehouse <jesse.whitehouse@databricks.com>
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.

LGTM thanks for this!

@susodapop susodapop merged commit 1965df5 into databricks:main Jul 12, 2023
14 checks passed
susodapop pushed a commit to unj1m/databricks-sql-python that referenced this pull request Sep 19, 2023
Signed-off-by: Sebastian Eckweiler <sebastian.eckweiler@mercedes-benz.com>
Signed-off-by: Jesse Whitehouse <jesse.whitehouse@databricks.com>
Co-authored-by: Sebastian Eckweiler <sebastian.eckweiler@mercedes-benz.com>
Co-authored-by: Jesse Whitehouse <jesse.whitehouse@databricks.com>
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