databricks-sql-python icon indicating copy to clipboard operation
databricks-sql-python copied to clipboard

Support proxy per-connection rather than just environment variables

Open jakewski opened this issue 1 year ago • 4 comments

Problem

I opened this issue: https://github.com/databricks/databricks-sql-python/issues/318. My use case involves using the Databricks SQL connector in a multi-threaded web application where we may not want to set the proxy for all traffic. The current method of using environment variables would not allow that.

Solution

Proxy specification works same way as the Python requests module - it will default to whatever is set in the environment variable, but it can manually be overridden for a single request / connection if desired.

jakewski avatar Jan 16 '24 22:01 jakewski

I would like to add a test for this in client tests - but seems they don't actually run in CI. Any guidance would be much appreciated! https://github.com/databricks/databricks-sql-python/issues/317

jakewski avatar Jan 16 '24 22:01 jakewski

Thanks for this contribution. We have rather poor testing facilities for proxies at present (we need proper CI for this repository -- that's coming soon). Will review and perform some manual testing in the next couple weeks.

susodapop avatar Jan 25 '24 21:01 susodapop

Thanks for this contribution. We have rather poor testing facilities for proxies at present (we need proper CI for this repository -- that's coming soon). Will review and perform some manual testing in the next couple weeks.

sounds good, appreciate the review!

jakewski avatar Jan 26 '24 19:01 jakewski

Hi @jakewski! Sorry for not getting back for so long. First of all, thank you for your effort - this indeed is a useful feature to have in this library. The changes you made look good. However, there are two more places that need to be updated as well:

  • OAuth manager. OAuth endpoints are located on the same host as Thrift backend, so if you need proxy to execute query - you definitely need the same proxy for OAuth
  • CloudFetch handler - same reasoning as above.

Indeed, we can merge this as is, but in this case the feature will be limited, and eventually other users will complain about it

kravets-levko avatar May 30 '24 14:05 kravets-levko