-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
feat(source-sendgrid): Upgrade to latest manifest-only CDK version to enable concurrency for all streams #49812
feat(source-sendgrid): Upgrade to latest manifest-only CDK version to enable concurrency for all streams #49812
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
regression test notes:
acceptance test notes:
Despite the mismatches, I believe this upgrade is working as expected |
@@ -640,6 +640,8 @@ definitions: | |||
path: "{{stream_slice['url']}}" | |||
url_base: "" | |||
http_method: GET | |||
download_extractor: | |||
type: ResponseToFileExtractor |
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.
There was a subtle change in this PR: https://github.com/airbytehq/airbyte-python-cdk/pull/50/files#diff-1d9bc4ca384e5c00867da05e9db9d919aba908c59bb73aab83883b9ed20def05L2040
Where we no longer automatically used the ResponseToFileExtractor
if download_extractor
was not defined. Instead we defaulted to the JsonDecoder which broke sendgrid. Since this was the only use of AsyncRetriever, its easy enough to just fix it here instead of a full breaking change.
/approve-regression-tests validated mismatches and mentioned in above notes
|
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.
changes looks safe to merge.
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
…_retriever_refactor
closed in favor of #48238 |
What
Sendgrid is currently our only certified connector that uses the new(ish)
AsyncRetriever
. In order to test the component migration to make async job streams concurrent, we want to release it to this connector.The
contacts
stream is the only async stream. But we've made some improvements forSimpleRetriever
to be stateless which should not impact things under the hood.User Impact
n/a
Can this PR be safely reverted and rolled back?