-
Notifications
You must be signed in to change notification settings - Fork 251
Implement no_retries_on_test_failure on DbtTestLocalOperator
#1563
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
base: main
Are you sure you want to change the base?
Implement no_retries_on_test_failure on DbtTestLocalOperator
#1563
Conversation
✅ Deploy Preview for sunny-pastelito-5ecb04 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
pankajastro
left a comment
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
no_retries_on_test_failure on DbtTestLocalOperator
440201c to
3307086
Compare
3307086 to
47ed26c
Compare
pankajkoti
left a comment
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.
Hi @okayhooni, thanks a lot for the report and your contribution!
Apologies for the delayed review—I was out last week and have been catching up this week.
After discussing this internally with @tatiana, we feel it would be better to unify this configuration with our existing interface for similar scenarios. For reference, in PR #1492, we added support for overriding the profile per dbt node. Similarly, we think it would be more consistent to allow this configuration to be overridden at the test level for specific nodes and at the dbt_project.yml level for global application.
For example, similar to this configuration, we could support something like:
tests:
jaffle_shop:
+meta:
cosmos:
retries: 0
Another point to consider is that test failures may also occur due to connectivity issues, such as being unable to connect to the datasource. In such cases, it might make sense to retry the task. If we can determine the actual reason for the failure—whether it was due to a failed dbt check or a connectivity issue—then we could apply this configuration accordingly (perhaps by analyzing the logs).
Additionally, we might not want to disable retries by default, as it maybe considered breaking change for existing versions. Right now, tasks may retry for various reasons beyond dbt test failures, such as network interruptions.
Looking forward to your thoughts on this!
|
@pankajkoti I see your point, and it makes sense. In our pipeline's use case, data testing is just an additional safeguard, and the test results should not impact the main batch flow. Because of this, retries for test tasks should generally be avoided, regardless of the reason for failure. However, as you mentioned, a more precise approach would be to parse the error message from the |
|
hi @okayhooni we're planning to release Cosmos 1.10.0 sometime next week and wished to check if you'd have a chance to iterate on this PR ? |
Description
Added
no_retries_on_test_failureparameter toDbtTestLocalOperatorwith a default value ofTrue.This parameter raises
AirflowFailException, ensuring that failed DBT tests are not retried unnecessarily, even if a common retries value of 1 or more is set inDbtDag/DbtTaskGroup.Unlike the
on_warning_callbackparameter, it does not seem necessary to propagateno_retries_on_test_failurethrough theconverterandairflow/graphmodules. Instead, similar to thecallbackparameter inAbstractDbtLocalBase, it can be passed viaoperator_argswhen needed.Related Issue(s)
closes #1537
Breaking Change?
Nope.
Checklist