🚨🐛 Source Shopify: Retry requests that return HTTP 500

[keiththompson]

What

I've observed the orders stream getting a HTTP 500 and marking the stream as complete. I would have expected this to either retry or fail. Please let me know if this was a conscious design choice. In my mind, a sync succeeding should mean all of the data is fully in place.

From looking at the implementation, it looks as though there is an expectation of some retries, but with the ShopifyNonRetryableErrors class configured as it is, none of these will happen on HTTP 500.

In my case running the sync again did fill in the gaps, which means the issue was transient and therefore, retryable.

Logs

{
    "type": "STATE",
    "state": {
        "type": "STREAM",
        "stream": {
            "stream_descriptor": {
                "name": "orders"
            },
            "stream_state": {
                "updated_at": "2024-11-18T17:06:34-05:00",
                "deleted": {
                    "deleted_at": ""
                }
            }
        },
        "sourceStats": {
            "recordCount": 250
        }
    }
}
{
    "type": "LOG",
    "log": {
        "level": "INFO",
        "message": "Stream `orders`. Entity might not be available or missing."
    }
}
{
    "type": "STATE",
    "state": {
        "type": "STREAM",
        "stream": {
            "stream_descriptor": {
                "name": "orders"
            },
            "stream_state": {
                "updated_at": "2024-11-18T17:06:34-05:00",
                "deleted": {
                    "deleted_at": ""
                }
            }
        },
        "sourceStats": {
            "recordCount": 0
        }
    }
}
{
    "type": "LOG",
    "log": {
        "level": "INFO",
        "message": "Read xxx records from orders stream"
    }
}
{
    "type": "LOG",
    "log": {
        "level": "INFO",
        "message": "Marking stream orders as STOPPED"
    }
}
{
    "type": "TRACE",
    "trace": {
        "type": "STREAM_STATUS",
        "emitted_at": 1736229749445.883,
        "stream_status": {
            "stream_descriptor": {
                "name": "orders"
            },
            "status": "COMPLETE"
        }
    }
}
{
    "type": "LOG",
    "log": {
        "level": "INFO",
        "message": "Finished syncing orders"
    }
}

How

Removed HTTP 500 from the list of non retryable errors.

Review guide

1. airbyte-integrations/connectors/source-shopify/source_shopify/utils.py
2. airbyte-integrations/connectors/source-shopify/source_shopify/streams/base_streams.py (get_error_handler func)

User Impact

HTTP 500 will Retry rather than be ignored.

Can this PR be safely reverted and rolled back?

• YES 💚
• NO ❌

[vercel]

@keiththompson is attempting to deploy a commit to the Airbyte Growth Team on Vercel.

A member of the Team first needs to authorize it.

[marcosmarxm]

Hello @keiththompson, I asked the team to review your contribution. Since it involves changes to a widely used connector, it may take some time to ensure that the change is the best option. If you need any updates on the status of your contribution, feel free to message me on Slack.

[bazarnov]

I'm fine with the change, but we've seen unpredictable api behavior in the past, for some entities were not available alongside the HTTP-500 (which is apparently incorrectly provided error status code, because the HTTP-404 was expected).

This is hard to predict what would it be for the rest of the Customers, but i'm pretty sure by far, this collisions might have been already resolved by Shopify.

Let's merge it.

[bazarnov]

@marcosmarxm I believe we need the approval here to run the required CI checks. Could you please take a look?

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 15, 2025 11:26am

[marcosmarxm]

Running tests @keiththompson and @bazarnov let's wait.

[maxi297]

It seems like there is a unit test failure related to this change. Can we fix that?

[keiththompson]

It seems like there is a unit test failure related to this change. Can we fix that?

I think these are passing now.