steveyz-astro commented on a change in pull request #19097:
URL: https://github.com/apache/airflow/pull/19097#discussion_r733850107
##########
File path: airflow/models/dagrun.py
##########
@@ -526,6 +526,28 @@ def update_state(
else:
self.set_state(State.RUNNING)
+ if self.get_state() == State.FAILED or self.get_state() ==
State.SUCCESS:
Review comment:
I did consider doing this in set_state(), but decided against it a for a
few reasons
- The existing logging messages are at this layer
- While I believe think it would logically equivalent right now, it's
possible the further code changes could end up tweaking the value of the fields
that this is logging after the set_state() call, and that would be more obvious
if the logging was at this layer rather than in the setter.
- Generally i prefer not having logging in the lowest level getter/setter
method because they rarely have the full context across why the value is being
referenced. In this case, the logging message isn't using anything that's
specific to the update_state() context, but we could easily augment the message
in the future to provide more context. E.g. failed due to deadlock vs task
failure.
I can certainly change to use self._state directly here though.
--
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]
