[GitHub] [airflow] potiuk commented on pull request #32627: Replace State by TaskInstanceState in Airflow executors

["potiuk (via GitHub)" <gi...@apache.org>]

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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org