potiuk commented on PR #32627:
URL: https://github.com/apache/airflow/pull/32627#issuecomment-1669498104
> This should be good. The TaskInstanceState is downcasted to str when it
goes into self.event_buffer, which is unfortunate, but at least this tightens
the signature in the public interface.
Are you sure this backwards compatible @uranusjr ? the `change_state()`
method is particularly worrying. Imagine someone writing an executor and handle
`change_state(state)`:
```
if state == State.FAILED:
do_some_processing
```
This would fail after that change. The public API has changed. The
`change_state` is a public API of BaseExecutor and it is supposed to be
overridden by someone implementing custom executor. In fact even our community
executors do (by chance they do not use `state` parameter - but they could
have, and other custom executors could have used it too.
After seeing how easy it is to miss the change I am going in fact to
implement similar protection for the public API as we have for common.sql - so
that whenever public API changes, we have a failure of CI. But in the meantime
- I am not sure we have an easy way to solve that, other than keep
`change_state` using `State enum` and maybe just convert all the TaskInstance.*
to State.* in the code.
--
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]
