Replace AirflowException with ValueError in ProduceToTopicOperator - #70351
FrankYang0529 wants to merge 1 commit into
Conversation
shahar1
left a comment
There was a problem hiding this comment.
Apparently #70333 was opened earlier and addresses the problem more precisely.
One thing in mind - here you tried to address two issues in one PR (validate_operators_init_exemption.txt + known_airflow_execptions) - I prefer to handle each separtely.
Feel free to tackle the other operators (there are still plenty), or to turn this PR into addressing the known_airflow_exceptions part once the other is merged.
|
I will turn this one into addressing the known_airflow_exceptions part after #70333 is merged. |
c6e42a0 to
3555991
Compare
shahar1
left a comment
There was a problem hiding this comment.
We might need to take an intermediate step of creating an exception class that derives from both AirflowException and ValueError, because it will break the retry mechanism that is based on detecting the exception type.
It had been discussed in the past but was put on hold, I'll try to revive this discussion.
Signed-off-by: PoAn Yang <payang@apache.org>
3555991 to
8667cd6
Compare
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
jason810496
left a comment
There was a problem hiding this comment.
I wonder should we introduced compatible layer for the exception (e.g. subclass of both AirflowException and ValueError) just in case users leverage the callback hook and catch against the AirflowException.
If the scenario I mentioned is valid, this is kind of a breaking change for the users.
|
What is the goal of changing the exception class? Is it to take a step closer to cleaning up the
I've looked at the code and I don't think that there is an issue there. I don't see any check for But there is a good chance that users are wrapping the operator's If we can determine the goal for this change, then we can figure out what direction to follow. For example, if this change is going to happen across all operators then we might want to create a new class type extending |
|
Thanks for the review. This PR started as part of #70296, which moves template field validation out of Several merged PRs have already replaced
A user who wraps any of these in |
jason810496
left a comment
There was a problem hiding this comment.
A user who wraps any of these in try/except AirflowException runs into the same change as with this PR.
My concern is the user facing exception handler like on_failure_callback since 2.x or the retry policy introduced in 3.x.
Should these also get a new class that extends AirflowException? If so, it seems better to decide that once for all providers rather than in this PR alone. I'm happy to add the class here if we go that way.
The best approach I can think of is introducing compatible exception in core and wait for the new core release, then add the compatible import of that new exception in the providers.
Or another direction is just keep the exception as-is and don't fix, since the swapping the exception for code style best practices might introduce bigger user impact than we expect.
Based on
CLAUDE.md, the community is actively reducing AirflowException usage. Replaceraise AirflowException(...)withraise ValueError(...)in ProduceToTopicOperator.Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.