-
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: fix retry attempts in case of user defined backoff time #5707
Conversation
1 to expected retries attempts. | ||
""" | ||
max_tries = self.max_retries | ||
""" |
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.
lets not use docstrings in the middle of the function
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.
Its not a docstring unless it occurs as the first statement in a module, function, class, or method definition. In this case its just a multi line comment.
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.
what I mean is that having novels like this in the middle of the function makes it hard to read and understand. please move it to the top.
max_tries: The maximum number of attempts to make before giving | ||
up ...The default value of None means there is no limit to | ||
the number of tries. | ||
This implies that is max_tries is excplicitly set to None there is no |
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.
This implies that is max_tries is excplicitly set to None there is no | |
This implies that if max_tries is excplicitly set to None there is no |
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.
fixed
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.
lgtm, just a few comments.
Also, please, check @keu's comments again.
airbyte-cdk/python/unit_tests/sources/streams/http/test_http.py
Outdated
Show resolved
Hide resolved
with pytest.raises(UserDefinedBackoffException): | ||
list(stream.read_records(SyncMode.full_refresh)) | ||
if retries <= 0: |
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.
with pytest.raises(UserDefinedBackoffException): | |
list(stream.read_records(SyncMode.full_refresh)) | |
if retries <= 0: | |
with pytest.raises(UserDefinedBackoffException): | |
list(stream.read_records(SyncMode.full_refresh)) | |
if retries <= 0: |
airbyte-cdk/python/unit_tests/sources/streams/http/test_http.py
Outdated
Show resolved
Hide resolved
list(stream.read_records(SyncMode.full_refresh)) | ||
assert send_mock.call_count == infinite_number + 1 |
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.
list(stream.read_records(SyncMode.full_refresh)) | |
assert send_mock.call_count == infinite_number + 1 | |
list(stream.read_records(SyncMode.full_refresh)) | |
assert send_mock.call_count == infinite_number + 1 |
with pytest.raises(DefaultBackoffException): | ||
list(stream.read_records(SyncMode.full_refresh)) | ||
assert send_mock.call_count == stream.max_retries + 1 |
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.
assert send_mock.call_count == stream.max_retries + 1 | |
assert send_mock.call_count == stream.max_retries + 1 |
explicitly (i.e. max_retries is not None). | ||
""" | ||
if max_tries is not None: | ||
max_tries = max(0, max_tries) + 1 |
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.
can it just be max(1, max_tries)
?
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.
Yeah, but max(1, max_tries + 1)
/publish-cdk dry-run=false
|
/publish-cdk dry-run=false
|
What
Part of fix for #5683
How
Describe the solution
Recommended reading order
x.java
y.python
Pre-merge Checklist
Expand the relevant checklist and delete the others.
New Connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
docs/SUMMARY.md
docs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampledocs/integrations/README.md
airbyte-integrations/builds.md
Airbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing./publish
command described hereUpdating a connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
docs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing./publish
command described hereConnector Generator
-scaffold
in their name) have been updated with the latest scaffold by running./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates
then checking in your changes