-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
🐛 Source Shopify: Allow the known HTTP errors
to be retried more than once for the BULK streams
#37589
🐛 Source Shopify: Allow the known HTTP errors
to be retried more than once for the BULK streams
#37589
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…add-retry-for-bulk-http-errors
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 you provide more information on the comments and ping me right after so that I can re-review with the added information?
airbyte-integrations/connectors/source-shopify/source_shopify/shopify_graphql/bulk/job.py
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-shopify/source_shopify/shopify_graphql/bulk/job.py
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-shopify/source_shopify/shopify_graphql/bulk/job.py
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-shopify/source_shopify/shopify_graphql/bulk/job.py
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-shopify/source_shopify/shopify_graphql/bulk/job.py
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-shopify/source_shopify/shopify_graphql/bulk/job.py
Outdated
Show resolved
Hide resolved
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 codes looks OK, please address Max's concerns. I also added a few suggestions to improve readability.
airbyte-integrations/connectors/source-shopify/source_shopify/shopify_graphql/bulk/job.py
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-shopify/source_shopify/shopify_graphql/bulk/job.py
Outdated
Show resolved
Hide resolved
…add-retry-for-bulk-http-errors
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.
I think that until I understand the flow, I'll have to mark this as "Request changes". Based on what I see, there is potential for data loss which I'm not comfortable with. We can sync on this if it's easier to address
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.
Forgot to add the flag based on #37589 (review)
Would be happy to discuss this once you're available.
Thank you for the review. |
…add-retry-for-bulk-http-errors
…add-retry-for-bulk-http-errors
…add-retry-for-bulk-http-errors
…add-retry-for-bulk-http-errors
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.
I prefer the newest changes. I like that:
- method visibility (private or public) is now explicit
- the retry is with an annotation on the
_job_check_state
method
I have a couple of concerns which you can push back on but once we see the same path forward, I'll approve
airbyte-integrations/connectors/source-shopify/unit_tests/graphql_bulk/test_job.py
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-shopify/source_shopify/shopify_graphql/bulk/job.py
Show resolved
Hide resolved
airbyte-integrations/connectors/source-shopify/source_shopify/streams/base_streams.py
Show resolved
Hide resolved
…add-retry-for-bulk-http-errors
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.
Cool, thanks for the changes. !
…add-retry-for-bulk-http-errors
…ttps://github.com/airbytehq/airbyte into baz/source/shopify/add-retry-for-bulk-http-errors
…add-retry-for-bulk-http-errors
…an once for the BULK streams (#37589)
What
Given this connection, we frequently see the
Internal Server Errors
orUnknown Errors
, which could and should be retried more than once to overcome the server availability and not to raise the error in between, allowing the source to retry more than once.How
bulk_retry_on_exception
decorator to catch and retry the known bulk errors (max attempts = 6)job_manager
to:checking the state of RUNNING job
_private
when they are not used elsewhere but the module itselfunit_tests
to cover the changes (the logic didn't change!)retry
for known HTTP errors for BULK job:test_job.py > test_retry_on_job_exception
User Impact
No impact is expected.
Can this PR be safely reverted and rolled back?