CDK: `AbstractSource.read()` skips syncing stream if its unavailable (add `AvailabilityStrategy` concept) #19977

erohmensing · 2022-12-01T14:47:02Z

What

Closes Skip stream syncing when stream is unavailable #17617
Fixes issue where an entire sync would fail if one or more streams were unavailable due to 403: Forbidden permission errors (usually due to missing scopes in an OAuth connection)

How

Adds a check_availability method to the Stream class to check availability
Adds an AvailabilityStrategy class to the Stream class to perform this check
Adds an HttpAvailabilityStrategy class to the HTTPStream class, and sets it as the default AvailabilityStrategy for this class
- This strategy tries to read the first record, excepts a 403:Forbidden HTTPError, and returns that the stream is unavailable, why, and where to learn more to fix this issue
Adds logic to AbstractSource.read() to skip syncing the stream if the stream is unavailable
Updates CheckStream ConnectionChecker class to first try using the availability strategy, if there is one

🚨 User Impact 🚨

End users:

For a given source, if CDK version is updated and source streams subclasses HttpStream, the HTTPAvailabilityStrategy will now be in effect for that stream, which means:
- Syncs that fail due to an expected HTTP error (403) while reading a stream will no longer fail. Information about the stream that is skipped is currently only listed as a warning in the logs. The skipped stream will not sync, the other streams will.

Source developers:

Source developers will now be able to write AvailabilityStrategies if they update their CDK version.
If CDK version is updated and source streams only subclasses Stream, no change.
If CDK version is updated and source streams subclasses HttpStream, the HTTPAvailabilityStrategy will now be in effect for that stream.

Note: Sources that already handle 403 errors in their read() methods should move that handling to their own AvailabilityStrategy (if handling is more specific than the default implementation). That will be addressed in #17853

…make default no AvailabilityStrategy

…y, flakecheck

airbyte-cdk/python/airbyte_cdk/sources/availability_strategy.py

alafanechere

Nice work here and great readability 👏

airbyte-cdk/python/airbyte_cdk/sources/availability_strategy.py

airbyte-cdk/python/airbyte_cdk/sources/abstract_source.py

airbyte-cdk/python/airbyte_cdk/sources/availability_strategy.py

airbyte-cdk/python/airbyte_cdk/sources/abstract_source.py

airbyte-cdk/python/airbyte_cdk/sources/availability_strategy.py

…tyStrategy unit tests passing for per-stream strategies

…t mocking doesn't support this

…andled HTTPErrors

airbyte-cdk/python/airbyte_cdk/sources/declarative/checks/check_stream.py

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

alafanechere

Nice work 👏 I left minor comments. LGTM, but please address @girarda questions before publishing the CDK and merging 😄

airbyte-cdk/python/airbyte_cdk/sources/abstract_source.py

airbyte-cdk/python/unit_tests/sources/test_source.py

…if possible

… if possible

erohmensing · 2022-12-12T19:00:14Z

/publish-cdk dry-run=true

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

erohmensing · 2022-12-12T19:24:43Z

/publish-cdk dry-run=false

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

airbyte-cdk/python/airbyte_cdk/sources/utils/stream_helpers.py

…ailable (add `AvailabilityStrategy` concept) (#19977)" This reverts commit 55a3288.

…hanges (#20523) * Revert "source-github: move known error handling to GithubAvailabilityStrategy (#19978)" This reverts commit f97db17. * Revert "🐛 Python CDK: fix `StopIteration` error for `check_availability` (#20429)" This reverts commit 4e9b014. * Revert "CDK: `AbstractSource.read()` skips syncing stream if its unavailable (add `AvailabilityStrategy` concept) (#19977)" This reverts commit 55a3288. * Restore changelog entries * bump CDK version * Bump Github version * Re-add removed dependencies * auto-bump connector version Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>

…(add `AvailabilityStrategy` concept) (#19977) * Rough first implememtation of AvailabilityStrategy s * Basic unit tests for AvailabilityStrategy and ScopedAvailabilityStrategy * Make availability_strategy a property, separate out tests * Remove from DeclarativeSource, remove Source parameter from methods, make default no AvailabilityStrategy * Add skip stream if not available to read() * Changes to CDK to get source-github working using AvailabilityStrategy, flakecheck * reorganize cdk class, add HTTPAvailabilityStrategy test * cleanup, docstrings * pull out error handling into separate method * Pass source and logger to check_connection method * Add documentation links, handle 403 specifically * Fix circular import * Add AvailabilityStrategy to Stream and HTTPStream classes * Remove AS from abstract_source, add to Stream, HTTPStream, AvailabilityStrategy unit tests passing for per-stream strategies * Modify MockHttpStream to set no AvailabilityStrategy since source test mocking doesn't support this * Move AvailabilityStrategy class to sources.streams * Move HTTPAvailabilityStrategy to http module * Use pascal case for HttpAvailabilityStrategy * Remove docs message method :( and default to True availability on unhandled HTTPErrors * add check_availability method to stream class * Add optional source parameter * Add test for connector-specific documentation, small tests refactor * Add test that performs the read() function for stream with default availability strategy * Add test for read function behavior when stream is unavailable * Add 403 info in logger message * Don't return error for other HTTPErrors * Split up error handling into methods 'unavailable_error_codes' and 'get_reason_for_error' * rework overrideable list of status codes to be a dict with reasons, to enforce that users provide reasons for all listed errors * Fix incorrect typing * Move HttpAvailability to its own module, fix flake errors * Fix ScopedAvailabilityStrategy, docstrings and types for streams/availability_strategy.py * Docstrings and types for core.py and http/availability_strategy.py * Move _get_stream_slices to a StreamHelper class * Docstrings + types for stream_helpers.py, cleanup test_availability.py * Clean up test_source.py * Move logic of getting the initial record from a stream to StreamHelper class * Add changelog and bump minor version * change 'is True' and 'is False' behavior * use mocker.MagicMock * Remove ScopedAvailabilityStrategy * Don't except non-403 errors, check_stream uses availability_strategy if possible * CDK: pass error to reasons_for_error_codes * make get_stream_slice public * Add tests for raising unhandled errors and retries are handled * Add tests for CheckStream via AvailabilityStrategy * Add documentation for stream availability of http streams * Move availability unit tests to correct modules, report error message if possible * Add test for reporting specific error if available

erohmensing added 8 commits November 23, 2022 11:03

Rough first implememtation of AvailabilityStrategy s

f1a228d

Basic unit tests for AvailabilityStrategy and ScopedAvailabilityStrategy

e2247f4

Make availability_strategy a property, separate out tests

8e22d0a

Remove from DeclarativeSource, remove Source parameter from methods, …

b824fb1

…make default no AvailabilityStrategy

Add skip stream if not available to read()

05b3220

Changes to CDK to get source-github working using AvailabilityStrateg…

6f1c2a6

…y, flakecheck

reorganize cdk class, add HTTPAvailabilityStrategy test

b880559

cleanup, docstrings

b604151

octavia-squidington-iv added the CDK Connector Development Kit label Dec 1, 2022

Merge branch 'master' into ella/availabilitystrategy

eefed8a

erohmensing commented Dec 1, 2022

View reviewed changes

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

erohmensing commented Dec 1, 2022

View reviewed changes

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

erohmensing added 3 commits December 1, 2022 11:01

pull out error handling into separate method

095cf75

Pass source and logger to check_connection method

e17831b

Add documentation links, handle 403 specifically

7dee650

erohmensing commented Dec 5, 2022

View reviewed changes

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

erohmensing requested a review from a team December 5, 2022 19:41

bleonard reviewed Dec 5, 2022

View reviewed changes

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

alafanechere reviewed Dec 6, 2022

View reviewed changes

erohmensing added 8 commits December 6, 2022 18:19

Fix circular import

c2ca4b4

Add AvailabilityStrategy to Stream and HTTPStream classes

7437078

Remove AS from abstract_source, add to Stream, HTTPStream, Availabili…

849dd5f

…tyStrategy unit tests passing for per-stream strategies

Modify MockHttpStream to set no AvailabilityStrategy since source tes…

56aaaa2

…t mocking doesn't support this

Move AvailabilityStrategy class to sources.streams

dfbb6fa

Move HTTPAvailabilityStrategy to http module

2dc1840

Use pascal case for HttpAvailabilityStrategy

a917de4

Remove docs message method :( and default to True availability on unh…

8079e26

…andled HTTPErrors

erohmensing changed the title ~~CDK: AvailabilityStrategy source property class - check stream's available before syncing~~ CDK: AvailabilityStrategy stream property class - check stream's available before syncing Dec 7, 2022

erohmensing added 2 commits December 7, 2022 10:24

add check_availability method to stream class

864bcb7

Add optional source parameter

b4b825e

girarda reviewed Dec 8, 2022

View reviewed changes

airbyte-cdk/python/airbyte_cdk/sources/declarative/checks/check_stream.py Show resolved Hide resolved

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

alafanechere approved these changes Dec 9, 2022

View reviewed changes

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

airbyte-cdk/python/unit_tests/sources/test_source.py Show resolved Hide resolved

airbyte-cdk/python/unit_tests/sources/test_source.py Outdated Show resolved Hide resolved

erohmensing added 11 commits December 9, 2022 09:50

change 'is True' and 'is False' behavior

a04ac98

use mocker.MagicMock

3a37e9c

Remove ScopedAvailabilityStrategy

527087e

Don't except non-403 errors, check_stream uses availability_strategy …

f872623

…if possible

CDK: pass error to reasons_for_error_codes

8a41807

make get_stream_slice public

101b74c

Add tests for raising unhandled errors and retries are handled

75a09ee

Add tests for CheckStream via AvailabilityStrategy

5ae0fbc

Add documentation for stream availability of http streams

e85362e

Move availability unit tests to correct modules, report error message…

162aa94

… if possible

Add test for reporting specific error if available

1a14238

girarda approved these changes Dec 12, 2022

View reviewed changes

erohmensing merged commit 55a3288 into master Dec 12, 2022

erohmensing deleted the ella/availabilitystrategy branch December 12, 2022 19:32

github-actions bot mentioned this pull request Dec 13, 2022

Bump helm chart version reference to 0.43.3 #20408

Merged

bazarnov reviewed Dec 13, 2022

View reviewed changes

airbyte-cdk/python/airbyte_cdk/sources/utils/stream_helpers.py Show resolved Hide resolved

bazarnov mentioned this pull request Dec 13, 2022

🐛 Python CDK: fix StopIteration error for check_availability #20429

Merged

erohmensing mentioned this pull request Dec 14, 2022

Roll out AvailabilityStrategy - slowly for GA connectors, as default for others #20401

Closed

erohmensing added a commit that referenced this pull request Dec 15, 2022

Revert "CDK: AbstractSource.read() skips syncing stream if its unav…

8b70ab2

…ailable (add `AvailabilityStrategy` concept) (#19977)" This reverts commit 55a3288.

octavia-squidington-iii mentioned this pull request Dec 18, 2022

Bump Airbyte version from 0.40.25 to 0.40.26 #20630

Merged

erohmensing mentioned this pull request Jan 18, 2023

Reintroduce AvailabilityStrategy into the CDK (HttpAvailabilityStrategy default not turned on yet) #21484

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CDK: `AbstractSource.read()` skips syncing stream if its unavailable (add `AvailabilityStrategy` concept) #19977

CDK: `AbstractSource.read()` skips syncing stream if its unavailable (add `AvailabilityStrategy` concept) #19977

erohmensing commented Dec 1, 2022 •

edited

Loading

alafanechere left a comment

alafanechere left a comment

erohmensing commented Dec 12, 2022 •

edited by github-actions bot

Loading

erohmensing commented Dec 12, 2022 •

edited by github-actions bot

Loading

CDK: AbstractSource.read() skips syncing stream if its unavailable (add AvailabilityStrategy concept) #19977

CDK: AbstractSource.read() skips syncing stream if its unavailable (add AvailabilityStrategy concept) #19977

Conversation

erohmensing commented Dec 1, 2022 • edited Loading

What

How

Recommended reading order

🚨 User Impact 🚨

alafanechere left a comment

Choose a reason for hiding this comment

alafanechere left a comment

Choose a reason for hiding this comment

erohmensing commented Dec 12, 2022 • edited by github-actions bot Loading

erohmensing commented Dec 12, 2022 • edited by github-actions bot Loading

CDK: `AbstractSource.read()` skips syncing stream if its unavailable (add `AvailabilityStrategy` concept) #19977

CDK: `AbstractSource.read()` skips syncing stream if its unavailable (add `AvailabilityStrategy` concept) #19977

erohmensing commented Dec 1, 2022 •

edited

Loading

erohmensing commented Dec 12, 2022 •

edited by github-actions bot

Loading

erohmensing commented Dec 12, 2022 •

edited by github-actions bot

Loading