[GitHub] [airflow] dstandish edited a comment on pull request #13423: WIP - add bash hook

[GitBox <gi...@apache.org>]

dstandish edited a comment on pull request #13423:
URL: https://github.com/apache/airflow/pull/13423#issuecomment-753511787


   > For few minutes I was considering making this hook stateless and keeping the state in operators but this adds unnecessary complexity.
   
   Yes @turbaszek, this is one of the things I was thinking about also.
   
   I could not think of a clean way, apart from having BashHook return a "runner" object that has methods `run` and `send_sigterm`.... but in essense this would just be another stateful hook, so might as well just leave it on BashHook, I figure.
   
   I also considered making `command` `env` and `output_encoding` hook init params.  But then you would not be able to use cached property in BashOperator, and this would add complexity to BashOperator as a result.  And we should be able to reuse the hook multiple times in different ways.
   
   Another thing I considered was, does it make sense to ensure that `sub_process` is unset at successful completion?  But I think there's no benefit to aggressive unsetting.  If `run_command` is used again, then `sub_process` will be overwritten with new process.  We could add a check, to see if sub_process is still running (if is already set when Popen is about to be called), or at least a warning.  However, these I think should probably be considered separately from this refactor.
   
   In sum, I too think current approach is good enough and now will work on updating docs and possibly tests as necessary.
   
   Thanks


----------------------------------------------------------------
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.

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