[GitHub] [airflow] potiuk commented on a diff in pull request #24967: Add TCP_KEEPALIVE option to http provider

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

potiuk commented on code in PR #24967:
URL: https://github.com/apache/airflow/pull/24967#discussion_r921006014


##########
airflow/providers/http/sensors/http.py:
##########
@@ -91,15 +100,26 @@ def __init__(
         self.headers = headers or {}
         self.extra_options = extra_options or {}
         self.response_check = response_check
-
-        self.hook = HttpHook(method=method, http_conn_id=http_conn_id)

Review Comment:
   Well. I have not thought too much about this use case when I added it - it was just bringing a general approach that we usually do where Hooks are created in execute() method rather than in constructor. 
   
   But if there are good reasons why it should not be moved I can remove that one from the batch and make RC2 and restore the self.hook. 
   
   I think with async operators approach we miss a bit of best practices:
   
   * should we create Hook in init()?
   * should we have a Hook as Operator attribute (self.hook)?
   * or should we restore the hook every time when we come back from deferred state?
   
   I think the last option is most "reasonable" because first two might make (an incorrect) assumption that the same hook object is available during the whole lifecycle of an operator's methods, and if Hook is not "stateless", it might break when operator is deferred (because the Hook will be recreated when operator resumes from deferred state). If you keep the hook as `self.` property, you might (unconciously) rely on the fact that the hook is the same object in various methods of deferred operator.
   
   WDYT? Does it sound like a good practice ?  Also others @dstandish maybe? I think we had some discussion about this in the past (can't recall exactly) 
   



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