Lee-W commented on code in PR #31999:
URL: https://github.com/apache/airflow/pull/31999#discussion_r1234847480
##########
airflow/jobs/triggerer_job_runner.py:
##########
@@ -676,11 +676,14 @@ def update_triggers(self, requested_trigger_ids:
set[int]):
try:
new_trigger_orm = new_triggers[new_id]
trigger_class =
self.get_trigger_by_classpath(new_trigger_orm.classpath)
- except BaseException as e:
- # Either the trigger code or the path to it is bad. Fail the
trigger.
- self.failed_triggers.append((new_id, e))
+ new_trigger_instance = trigger_class(**new_trigger_orm.kwargs)
+ except (BaseException, TypeError) as err:
+ # BaseException: Either the trigger code or the path to it is
bad. Fail the trigger.
+ # TypeError: The argument of the __init__ method in the
trigger might have been changed.
+ self.log.error("Trigger failed; message=%s", err)
+ self.failed_triggers.append((new_id, err))
continue
- new_trigger_instance = trigger_class(**new_trigger_orm.kwargs)
Review Comment:
This is the most straightforward way of what exceptions might happen in
which part of the code. The first thought I had when I received the previous
comment was whether it's difficult to read to have so many try-catch blocks,
and their error handling is almost the same. That's why I change it to the
current implementation.
Another implementation I'm thinking of is
```python
try:
new_trigger_orm = new_triggers[new_id]
trigger_class =
self.get_trigger_by_classpath(new_trigger_orm.classpath)
new_trigger_instance =
trigger_class(**new_trigger_orm.kwargs)
except TypeError as err:
# The argument of the __init__ method in the trigger might
have been changed.
self.log.error("Trigger failed; message=%s", err)
self.failed_triggers.append((new_id, e))
except BaseException as err:
# Either the trigger code or the path to it is bad. Fail the
trigger.
self.failed_triggers.append((new_id, err))
```
But we still need to handle the `failed_triggeres` on both sections in this
implementation. The only benefit is that we reduce one try-catch block.
Please let me know which is preferred. Thanks!
##########
airflow/jobs/triggerer_job_runner.py:
##########
@@ -676,11 +676,14 @@ def update_triggers(self, requested_trigger_ids:
set[int]):
try:
new_trigger_orm = new_triggers[new_id]
trigger_class =
self.get_trigger_by_classpath(new_trigger_orm.classpath)
- except BaseException as e:
- # Either the trigger code or the path to it is bad. Fail the
trigger.
- self.failed_triggers.append((new_id, e))
+ new_trigger_instance = trigger_class(**new_trigger_orm.kwargs)
+ except (BaseException, TypeError) as err:
Review Comment:
Yes, if that's the case, does it mean we catch an exception too broad for
the following section? Or is it something expected?
```python
new_trigger_orm = new_triggers[new_id]
trigger_class =
self.get_trigger_by_classpath(new_trigger_orm.classpath)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
