-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
🎉 Python CDK: Allow setting network adapter args on outgoing HTTP requests #4493
Changes from 8 commits
e0178bd
dd8f924
e0e5817
39b1c12
bf39c43
1bc4bb1
bbc6d7e
d021dd4
df5d4bf
72663c5
332f61d
e5710c5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -118,6 +118,19 @@ def request_body_json( | |
""" | ||
return None | ||
|
||
def request_kwargs( | ||
self, | ||
stream_state: Mapping[str, Any], | ||
stream_slice: Mapping[str, Any] = None, | ||
next_page_token: Mapping[str, Any] = None, | ||
) -> Mapping[str, Any]: | ||
""" | ||
Override to return a mapping of keyword arguments to be used when creating the HTTP request. | ||
Any option listed in https://docs.python-requests.org/en/latest/api/#requests.adapters.BaseAdapter.send for can be returned from | ||
this method. Note that these options do not conflict with request-level options such as headers, request params, etc.. | ||
""" | ||
return {} | ||
|
||
@abstractmethod | ||
def parse_response( | ||
self, | ||
|
@@ -172,7 +185,7 @@ def _create_prepared_request( | |
# see https://github.com/litl/backoff/pull/122 | ||
@default_backoff_handler(max_tries=5, factor=5) | ||
@user_defined_backoff_handler(max_tries=5) | ||
def _send_request(self, request: requests.PreparedRequest) -> requests.Response: | ||
def _send_request(self, request: requests.PreparedRequest, request_kwargs: Mapping[str, Any]) -> requests.Response: | ||
""" | ||
Wraps sending the request in rate limit and error handlers. | ||
|
||
|
@@ -190,9 +203,8 @@ def _send_request(self, request: requests.PreparedRequest) -> requests.Response: | |
Unexpected transient exceptions use the default backoff parameters. | ||
Unexpected persistent exceptions are not handled and will cause the sync to fail. | ||
""" | ||
response: requests.Response = self._session.send(request) | ||
response: requests.Response = self._session.send(request, **request_kwargs) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see these changes as potentially destructive. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, I checked the source code and it seems that the only parameters that we can use here are:
|
||
if self.should_retry(response): | ||
|
||
custom_backoff_time = self.backoff_time(response) | ||
if custom_backoff_time: | ||
raise UserDefinedBackoffException(backoff=custom_backoff_time, request=request, response=response) | ||
|
@@ -224,8 +236,8 @@ def read_records( | |
params=self.request_params(stream_state=stream_state, stream_slice=stream_slice, next_page_token=next_page_token), | ||
json=self.request_body_json(stream_state=stream_state, stream_slice=stream_slice, next_page_token=next_page_token), | ||
) | ||
|
||
response = self._send_request(request) | ||
request_kwargs = self.request_kwargs(stream_state=stream_state, stream_slice=stream_slice, next_page_token=next_page_token) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure I understand why you split kwargs here? can't you pass it directly to PreparedRequest? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, I see you tried to mimic There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we need to use prepared request for good idea, changed to use |
||
response = self._send_request(request, request_kwargs) | ||
yield from self.parse_response(response, stream_state=stream_state, stream_slice=stream_slice) | ||
|
||
next_page_token = self.next_page_token(response) | ||
|
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.
see #4507