Skip to content
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

Raise an error when Airbyte sync job fails to mark the task run as Failed #5362

Merged
merged 2 commits into from Jan 24, 2022

Conversation

anna-geller
Copy link
Contributor

Summary

Airbyte task currently checks for the job status here:

while job_status not in [self.JOB_STATUS_FAILED, self.JOB_STATUS_SUCCEEDED]:

However, when the job status is failed, it only logs the error rather than raising an exception:

self.logger.error(f"Job {job_id} failed.")

Changes

Raising an exception to mark the task run as Failed.

Importance

Fixes a bug flagged by a community user in this Slack thread.

Checklist

This PR:

  • [] adds new tests (if appropriate)
  • adds a change file in the changes/ directory (if appropriate)
  • [] updates docstrings for any new functions or function arguments, including docs/outline.toml for API reference docs (if appropriate)

@anna-geller anna-geller marked this pull request as ready for review January 24, 2022 11:42
@zanieb
Copy link
Contributor

zanieb commented Jan 24, 2022

Let's add a changelog entry for this, it's a significant change in behavior.

@anna-geller
Copy link
Contributor Author

Let's add a changelog entry for this, it's a significant change in behavior.

I added changes/pr5362.yml already - or do you mean something else?

@zanieb
Copy link
Contributor

zanieb commented Jan 24, 2022

Sorry I guess I missed that :)

@zanieb zanieb merged commit c44fd8f into PrefectHQ:master Jan 24, 2022
@zanieb zanieb mentioned this pull request Jan 25, 2022
@b4stien
Copy link

b4stien commented Feb 2, 2022

Don't we want the same behaviour for cancelled jobs? Otherwise we're stuck in an infinite loop.

@anna-geller
Copy link
Contributor Author

Don't we want the same behaviour for cancelled jobs? Otherwise we're stuck in an infinite loop.

I see. If you would be willing to contribute a PR for it, we would take time to review and QA this behavior together. Is this common that someone cancels Airbyte sync jobs? I imagined it as a data replication mechanism you wouldn't want to cancel to avoid some inconsistent database state, but I'm not that familiar with Airbyte

@b4stien
Copy link

b4stien commented Feb 2, 2022

Is this common that someone cancels Airbyte sync jobs?

There is a button to cancel one in Airbyte's interface, so it can happen. Not that frequently I'd agree, but disproportionately during test/setup to have a fast feedback loop.

I imagined it as a data replication mechanism you wouldn't want to cancel to avoid some inconsistent database state

Depending on the mode you chose the next synchronization will overwrite previous data, so there won't be any inconsistency.

I'd see if I have time to draft a PR, thanks.

@desertaxle desertaxle added the integrations Related to integrations with other services label Mar 30, 2022
lance0805 pushed a commit to hyl2015/prefect that referenced this pull request Aug 2, 2022
…iled (PrefectHQ#5362)

* Raise an error when Airbyte sync job fails

* Create pr5362.yml

Co-authored-by: Anna Geller <anna.m.geller@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrations Related to integrations with other services
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants