-
Notifications
You must be signed in to change notification settings - Fork 116
Refactor codebase to use a unified http client #668
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
Refactor codebase to use a unified http client #668
Conversation
Signed-off-by: Vikrant Puppala <[email protected]>
url=token_request_url, data=data, headers=headers, auth=IgnoreNetrcAuth() | ||
# Use unified HTTP client | ||
from databricks.sql.common.unified_http_client import IgnoreNetrcAuth | ||
response = self.http_client.request( |
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.
How is a static method using class http_client ?
@@ -292,6 +291,7 @@ def read(self) -> Optional[OAuthToken]: | |||
auth_provider=self.session.auth_provider, | |||
host_url=self.session.host, | |||
batch_size=self.telemetry_batch_size, | |||
http_client=self.session.http_client, |
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.
so we are not keeping different client for Telemetry ( I believe there is different retry behaviour for it )
src/databricks/sql/client.py
Outdated
@@ -744,16 +744,20 @@ def _handle_staging_put( | |||
) | |||
|
|||
with open(local_file, "rb") as fh: | |||
r = requests.put(url=presigned_url, data=fh, headers=headers) | |||
r = self.connection.session.http_client.request('PUT', presigned_url, body=fh.read(), headers=headers) |
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.
@sreekanth-db Can you discuss with @vikrantpuppala and integrate the new common client approach in volume operations
def upload_file(self, url: str, file_path: str, headers: Optional[Dict[str, str]] = None) -> urllib3.HTTPResponse: | ||
""" | ||
Upload a file using PUT method. | ||
|
||
Args: | ||
url: URL to upload to | ||
file_path: Path to the file to upload | ||
headers: Optional headers | ||
|
||
Returns: | ||
urllib3.HTTPResponse: The response from the server | ||
""" | ||
with open(file_path, 'rb') as file_obj: | ||
return self.request('PUT', url, body=file_obj.read(), headers=headers) | ||
|
||
def download_file(self, url: str, file_path: str, headers: Optional[Dict[str, str]] = None) -> None: | ||
""" | ||
Download a file using GET method. | ||
|
||
Args: | ||
url: URL to download from | ||
file_path: Path where to save the downloaded file | ||
headers: Optional headers |
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.
don't think they should be part of the Http client
max_download_threads=max_download_threads, | ||
ssl_options=ssl_options, | ||
session_id_hex=session_id_hex, | ||
statement_id=statement_id, | ||
chunk_id=chunk_id, | ||
http_client=http_client, |
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.
why is the order of params changed ?
Signed-off-by: Vikrant Puppala <[email protected]>
Signed-off-by: Vikrant Puppala <[email protected]>
Signed-off-by: Vikrant Puppala <[email protected]>
What type of PR is this?
Description
Refactor codebase to use a unified http client
How is this tested?
Related Tickets & Documents