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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
