Python CDK: fix retry attempts in case of user defined backoff time #5707

avida · 2021-08-27T14:46:26Z

What

Part of fix for #5683

How

Describe the solution

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

Community member? Grant edit access to maintainers (instructions)
Secrets in the connector's spec are annotated with airbyte_secret
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Code reviews completed
Documentation updated
- Connector's README.md
- docs/SUMMARY.md
- docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
- docs/integrations/README.md
- airbyte-integrations/builds.md
PR name follows PR naming conventions
Connector added to connector index like described here

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
Credentials added to Github CI. Instructions.
/test connector=connectors/<name> command is passing.
New Connector version released on Dockerhub by running the /publish command described here

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Secrets in the connector's spec are annotated with airbyte_secret
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Code reviews completed
Documentation updated
- Connector's README.md
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions
Connector version bumped like described here

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
Credentials added to Github CI. Instructions.
/test connector=connectors/<name> command is passing.
New Connector version released on Dockerhub by running the /publish command described here

Connector Generator

Issue acceptance criteria met
PR name follows PR naming conventions
If adding a new generator, add it to the list of scaffold modules being tested
The generator test modules (all connectors with -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
Documentation which references the generator is updated as needed.

keu · 2021-08-27T15:21:24Z

airbyte-cdk/python/airbyte_cdk/sources/streams/http/http.py

+ 1 to expected retries attempts.
+ """
+ max_tries = self.max_retries
+ """


lets not use docstrings in the middle of the function

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.

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.

airbyte-cdk/python/airbyte_cdk/sources/streams/http/http.py

Zirochkaa · 2021-08-30T05:47:26Z

airbyte-cdk/python/airbyte_cdk/sources/streams/http/http.py

+ 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


Suggested change

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

airbyte-cdk/python/airbyte_cdk/sources/streams/http/http.py

Zirochkaa

lgtm, just a few comments.
Also, please, check @keu's comments again.

airbyte-cdk/python/unit_tests/sources/streams/http/test_http.py

Zirochkaa · 2021-08-30T06:10:36Z

airbyte-cdk/python/unit_tests/sources/streams/http/test_http.py

+ with pytest.raises(UserDefinedBackoffException):
+ list(stream.read_records(SyncMode.full_refresh))
+ if retries <= 0:


Suggested change

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

Zirochkaa · 2021-08-30T06:12:19Z

airbyte-cdk/python/unit_tests/sources/streams/http/test_http.py

+ list(stream.read_records(SyncMode.full_refresh))
+ assert send_mock.call_count == infinite_number + 1


Suggested change

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

Zirochkaa · 2021-08-30T06:12:36Z

airbyte-cdk/python/unit_tests/sources/streams/http/test_http.py

+ send_mock = mocker.patch.object(requests.Session, "send", return_value=req)
 with pytest.raises(DefaultBackoffException):
 list(stream.read_records(SyncMode.full_refresh))
+ assert send_mock.call_count == stream.max_retries + 1


Suggested change

assert send_mock.call_count == stream.max_retries + 1

assert send_mock.call_count == stream.max_retries + 1

sherifnada · 2021-08-30T14:53:10Z

airbyte-cdk/python/airbyte_cdk/sources/streams/http/http.py

+ explicitly (i.e. max_retries is not None).
+ """
+ if max_tries is not None:
+ max_tries = max(0, max_tries) + 1


can it just be max(1, max_tries)?

Yeah, but max(1, max_tries + 1)

avida · 2021-08-31T06:57:08Z

/publish-cdk dry-run=false

Error: No ref found for: drezchykov/5683-CDK-fix

avida · 2021-08-31T06:59:25Z

/publish-cdk dry-run=false

🕑 https://github.com/airbytehq/airbyte/actions/runs/1185106177
✅ https://github.com/airbytehq/airbyte/actions/runs/1185106177

Python CDK: fix retry attempts in case of user defined backoff time

3ad4e66

avida requested review from Zirochkaa, htrueman and yevhenii-ldv August 27, 2021 14:46

github-actions bot added the CDK Connector Development Kit label Aug 27, 2021

keu reviewed Aug 27, 2021

View reviewed changes

airbyte-cdk/python/airbyte_cdk/sources/streams/http/http.py Show resolved Hide resolved

keu reviewed Aug 27, 2021

View reviewed changes

airbyte-cdk/python/airbyte_cdk/sources/streams/http/http.py Outdated Show resolved Hide resolved

Fix review comments

47c8c3a

avida requested a review from keu August 27, 2021 16:01

Zirochkaa reviewed Aug 30, 2021

View reviewed changes

Zirochkaa approved these changes Aug 30, 2021

View reviewed changes

fix review comments

1ce67fe

avida requested a review from sherifnada August 30, 2021 13:39

avida linked an issue Aug 30, 2021 that may be closed by this pull request

Source Slack: hitting max retries #5683

Closed

sherifnada requested review from Phlair and removed request for sherifnada August 30, 2021 14:50

sherifnada approved these changes Aug 30, 2021

View reviewed changes

sherifnada removed the request for review from Phlair August 30, 2021 14:53

avida merged commit eed2e10 into master Aug 31, 2021

avida deleted the drezchykov/5683-CDK-fix branch August 31, 2021 06:55

avida restored the drezchykov/5683-CDK-fix branch August 31, 2021 06:58

jrhizor mentioned this pull request Sep 3, 2021

Bump Airbyte version from 0.29.13-alpha to 0.29.14-alpha #5831

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Python CDK: fix retry attempts in case of user defined backoff time #5707

Python CDK: fix retry attempts in case of user defined backoff time #5707

Uh oh!

avida commented Aug 27, 2021

keu Aug 27, 2021

avida Aug 27, 2021

keu Aug 28, 2021

Uh oh!

Uh oh!

Zirochkaa Aug 30, 2021

avida Aug 30, 2021

Uh oh!

Uh oh!

Zirochkaa left a comment

Uh oh!

Zirochkaa Aug 30, 2021

Uh oh!

Zirochkaa Aug 30, 2021

Zirochkaa Aug 30, 2021

sherifnada Aug 30, 2021

avida Aug 31, 2021

avida commented Aug 31, 2021 •

edited by github-actions bot

Loading

avida commented Aug 31, 2021 •

edited by github-actions bot

Loading

Labels

5 participants

	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

		list(stream.read_records(SyncMode.full_refresh))
		assert send_mock.call_count == infinite_number + 1

	assert send_mock.call_count == stream.max_retries + 1

	assert send_mock.call_count == stream.max_retries + 1

Python CDK: fix retry attempts in case of user defined backoff time #5707

Python CDK: fix retry attempts in case of user defined backoff time #5707

Uh oh!

Conversation

avida commented Aug 27, 2021

What

How

Recommended reading order

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Zirochkaa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avida commented Aug 31, 2021 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

avida commented Aug 31, 2021 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Labels

5 participants

avida commented Aug 31, 2021 •

edited by github-actions bot

Loading

avida commented Aug 31, 2021 •

edited by github-actions bot

Loading