You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/11/17 21:41:04 UTC
[GitHub] [airflow] dstandish opened a new pull request #19662: BaseOperator.on_kill should optionally accept airflow context
dstandish opened a new pull request #19662:
URL: https://github.com/apache/airflow/pull/19662
There are times, particularly as we incomporate more deferrable operators, when the on_kill logic would benefit from access to the airflow context. For example in the kubernetes context we might need to use context information to look up the pod name. Having context available in on_kill frees us from having to ensure in all possible entrypoints that we set instance attributes that on_kill would need in order to do a proper cleanup.
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r751711095
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
> The following change should handle airflow<2.3 -- it checks the function signature and see if it has context in it or no
Yeah. I understand that. I am thinking more about implementation of operators in providers. If people will start taking advantage of the context, their implementations might start (even accidentally) depend on context being available to perform "proper" cleanup. I think in pretty much all cases this will be the case (why else would you use it?). Either you use context to do the proper cleanup, or you leave stuff behind. If the implemetnation "depends" on the on_kill having context it might make those future operators provide poor experience on < 2.3.0 even accidentally.
I see why we want to implement it - and I am not at all opposing it, I am just thinking how to "make sure" we have some protections so that somoene who did not yet migrate to 2.3.0 will use providers released after this change (that might rely on the context being present).
I thought a bit on that and another option is to "force" Airflow >= 2.3.0" for any provider that declares 'context' in on_kill - this can be automateded with pre-commit check. This way we add the >= 2.3.0 limitation only for those providers that use the context.
For example the first time we implement `on_kill(context)` in KubernetesPodOperator, we add `apache-airflow >= 2.3.0`. - so that it will not be usable in 2.2.2 for example
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r751782933
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
Hmm. I went to sleep but could not - started to think about it and I think there is a possibility to implement the "meta-class" behaviour.
The solution has a nice property thatt it coulld enable us in the future to add more such API-breaking changes to providers but keep a compatibility layer with earlier airlfow versions, with very little effort.
1) we could introiduce a new package `airflow-proviiders-compat`
2) The new providers would have the "compat" as dependency
3) The `compat` package can contain some common classes that can be used in providers. For example in this case that could be BaseContextKill class witt the meta-class behaviour of passing the context from "execute" to "on_kill". That could also be done with decorators not classes I think.
4) all new operators implemeting on_kill(context) should have both BaseOperator and BaseContextKill as parents;
5) The`BaseContextKill` meta class could do all the monkey-patching for airflow < 2.3
Then any operator that wants to use the on_kill(context) could just do this (and we can also autoamatically verify it).
```
class KubernetesPodOpertor(BaseOperator,BaseContextKill):
def on_kill(context):
,,,,,
```
This way the new providers (with nice clean-up) could continue to use the kill-with-context even for Airflow 2.1.
This might be too much to handle **just** the "context" addition, but it also opens up some interesting possibilities of adding other behaviours to operators with "compat" layer, that could make them work the same for earlier versions of Airlfow. Also this is very much in line with similar "compat" packages for Python.
Thoughts?
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r751736159
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
Another idea (maybe a bit better).
Is it always the case (I am thinking about defferable operators here) that on_kill() is executed always after execute() for the same instance of operator?
If the answer is `yes`, then maybe we can figure out a scheme (using BaseOperator meta-classes similarly as with apply_defaults) to make context non-optional?
If the `on_kill(context)` operator is instantiated for Airflow <2.3 the meta-class could monkey-patch the execute() and on_kill() methods of that operator. It could 'catch' the execute() method call and store the `context` as attribute of the operator instance, and add to on_kill() automatically.
This way, the new providers with `on_kill(context)` instaled for Airflow < 2.3 will also get the context passed to their on_kill method.
This would mean that:
* context, when used in on_kill, will not be optional
* operators that have on_kill(context) implemented will not have do `if context:`
* any sophisticated cleanup implemented for those operators will also work for Airlfow < 2.3
* the performance penalty for monkeypatching (small) will be only paid for Airflow < 2.3
* the interface of each operator using on_kill(context) will be even cleaner because of the non-optionality
The only problem with that solution is if we have a case when on_kill() method is called for instance of operator without prior execute() call for the same instance - but I think this is not the case currently (so we should be safe).
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r751662986
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
I think we should very carefully approach this change.
This is not really `future-compatible`.
Any of the operators using the context in on-kill will be - by definition - not compatible with airflow < 2.3.0. This means that if someone implements a "crucial" logic in any of the providers those providers will have to add `airflow >= 2.3.0`.
I do not have a solution for that yet but I think this should be more than optional parameter .
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r751686441
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
I do not yet know :( .. My first thought is though - how to make sure that "crucial" functionality does not depend on the context being provided.
Basically every on_kill implementation that will use `context` will have to do something like that:
```
if context:
# do this and perform some actions
else:
# hmmmm what should we do there if context is missing?
```
My initial thought is;
a) how to make sure that every operator does the if context (bringing back - currently disabled - MyPy and adding Context as TypedDict derivation migh help with that)
b) how to make sure that the "# do this and perform some actions" does not become "actually mandatory",
For the b) I can imagine that you will implement some handling in the "if context" that will render such operator virtually useless when context is None. Such operators can leak memory, leave the pods behind etc. etc. And while those operators will be "technically compatible" with Airlfow < 2.3.0, they will provide a very poor experience there.
I have no answers yet, This is just a feeling that we somehow have to address those two problems.
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r751782933
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
Hmm. I went to sleep but could not - started to think about it and I think there is a possibility to implement the "meta-class" behaviour.
The solution has a nice property thatt it coulld enable us in the future to add more such API-breaking changes to providers but keep a compatibility layer with earlier airlfow versions, with very little effort.
1) we could introiduce a new package `airflow-proviiders-compat`
2) The new providers would have the "compat" as dependency
3) The `compat` package can contain some common classes that can be used in providers. For example in this case that could be BaseContextKill class witt the meta-class behaviour of passing the context from "execute" to "on_kill". That could also be set of decorators.
4) all new operators implemeting on_kill(context) should have both BaseOperator and BaseContextKill as parents;
5) The`BaseContextKill` meta class could do all the monkey-patching for airflow < 2.3
Then any operator that wants to use the on_kill(context) could just do this (and we can also autoamatically verify it).
```
class KubernetesPodOpertor(BaseOperator,BaseContextKill):
def on_kill(context):
,,,,,
```
This way the new providers (with nice clean-up) could continue to use the kill-with-context even for Airflow 2.1.
This might be too much to handle **just** the "context" addition, but it also opens up some interesting possibilities of adding other behaviours to operators with "compat" latyer, that could make them work the same for earlier versions of Airlfow. Also this is very much in line with similar "compat" packages for Python.
Thoughts?
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r753577455
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
another way we could accomplish the same thing is if there were a reliable way to store state. e.g. if we could store pod details, then as soon as we have the pod info we could store it with `self.state.set('pod')` or similar. Then within `on_kill` could retrieve this with `self.state.get('pod')` or similar
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r751666934
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
@potiuk can you clarify
> I do not have a solution for that yet but I think this should be more than optional parameter ..
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
@potiuk can you clarify
> I do not have a solution for that yet but I think this should be more than optional parameter .
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r751666934
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
@potiuk can you clarify this bit
> I do not have a solution for that yet but I think **this should be more than optional parameter** .
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r751666934
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
@potiuk can you clarify
> I do not have a solution for that yet but I think **this should be more than optional parameter** .
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r751689583
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
One (easy but a bit "tough") solution to that is that we do the same as in "apply_defaults" case: Adding apache-airflow >=2.3.0 to all providers. But this also means that we should not merge that change until **just before** 2.3.0 branch is cut off and coordinate relasing all providers to be compatible with >= 2.3.0 **just** before 2.3.0 gets released.
This is possible option, but I am not sure if adding "on_kill" alone is strong enough justrification for such a move.
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r752556309
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
One other comment here:
The "metaclass" thingie is of course very complex and I am not really sure if the case is 'strong enough' for it. For me also adding the > 2.3.0 limits automatically for providers that use "on_kill(context) is acceptable approach. It's also way simpler and faster to implement and test.
It also has the additional benefit, that we can gently "push" the users to newer Airflow versions - which is generally speaking a "nice" side-effect of such limits.
But if we go with "bumping the min airflow version" approach - one thing to consider is actually using a different method and leave `on_kill` as is.
For example we could add a new `on_terminate(context)` method that will only be in Airlfow 2.3.0+. That would be more explicit and difficult to miss - and that would be a real "new feature" added, rather than `kind-a-compatible-augmented-old-feature`. I think that might be far less confusing and easier to explain.
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r751711095
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
> The following change should handle airflow<2.3 -- it checks the function signature and see if it has context in it or no
Yeah. I understand that. I am thinking more about implementation of operators in providers. If people will start taking advantage of the context, their implementations might start (even accidentally) depend on context being available to perform "proper" cleanup. I think in pretty much all cases this will be the case (why else would you use it?). Either you use context to the proper cleanup, or you leave stuff behind. If the implemetnation "depends" on the on_kill having context it might make those future operators provide poor experience on < 2.3.0 even accidentally.
I see why we want to implement it - and I am not at all opposing it, I am just thinking how to "make sure" we have some protections so that somoene who did not yet migrate to 2.3.0 will use providers released after this change (that might rely on the context being present).
I thought a bit on that and another option is to "force" Airflow >= 2.3.0" for any provider that declares 'context' in on_kill - this can be automateded with pre-commit check. This way we add the >= 2.3.0 limitation only for those providers that use the context.
For example the first time we implement `on_kill(context)` in KubernetesPodOperator, we add `apache-airflow >= 2.3.0`. - so that it will not be usable in 2.2.2 for example
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r751711095
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
> The following change should handle airflow<2.3 -- it checks the function signature and see if it has context in it or no
Yeah. I understand that. I am thinking more about implementation of operators in providers. If people will start taking advantage of the context, their implementations might start (even accidentally) depend on context being available to perform "proper" cleanup. I think in pretty much all cases this will be the case. Either you use context to the proper cleanup, or you leave stuff behind. If the implemetnation "depends" on the on_kill having context it might make those future operators provide poor experience on < 2.3.0 even accidentally.
I see why we want to implement it - and I am not at all opposing it, I am just thinking how to "make sure" we have some protections so that somoene who did not yet migrate to 2.3.0 will use providers released after this change (that might rely on the context being present).
I thought a bit on that and another option is to "force" Airflow >= 2.3.0" for any provider that declares 'context' in on_kill - this can be automateded with pre-commit check. This way we add the >= 2.3.0 limitation only for those providers that use the context.
For example the first time we implement `on_kill(context)` in KubernetesPodOperator, we add `apache-airflow >= 2.3.0`. - so that it will not be usable in 2.2.2 for example
--
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
[GitHub] [airflow] kaxil commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r751684859
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
The following change should handle airflow<2.3 -- it checks the function signature and see if it has `context` in it or no
https://github.com/apache/airflow/blob/849ef70cc444342b1cc3fcc71f4e435e4075e48d/airflow/models/taskinstance.py#L1405-L1414
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r752655282
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
hmm well, it's also confusing to have both `on_terminate` and `on_kill`... signature evolution is a thing that happens... seems preferable to me to evolve it in place.
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r753577455
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
another way we could accomplish the same thing is if there were a reliable way to store state. e.g. if we could store pod details, then as soon as we have the pod info we could store it with `self.state.set('pod')` or similar. Then within `on_kill` could retrieve this with `self.state.get('pod')` or similar
i think ultimately airflow needs something like this... interested in your take
--
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
[GitHub] [airflow] github-actions[bot] closed pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #19662:
URL: https://github.com/apache/airflow/pull/19662
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r751782933
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
Hmm. I went to sleep but could not - started to think about it and I think there is a possibility to implement the "meta-class" behaviour.
The solution has a nice property thatt it coulld enable us in the future to add more such API-breaking changes to providers but keep a compatibility layer with earlier airlfow versions, with very little effort.
1) we could introiduce a new package `airflow-proviiders-compat`
2) The providers would have the "compat" as dependency
3) `compat` could add some common classes that can be used in providers (for example in this case that could be BaseContextKill class (wiht the meta-class behaviour of passing the context from "execute" to "on_kill". That could also be set of decorators.
4) all new operators implemeting on_kill(context) should have both BaseOperator and BaseContextKill as parents;
5) The`BaseContextKill` meta class could do all the monkey-patching for airflow < 2.3
Then any operator that wants to use the on_kill(context) could just do this (and we can also autoamatically verify it).
```
class KubernetesPodOpertor(BaseOperator,BaseContextKill):
def on_kill(context):
,,,,,
```
This way the new providers (with nice clean-up) could continue to use the kill-with-context even for Airflow 2.1.
This might be too much to handle **just** the "context" addition, but it also opens up some interesting possibilities of adding other behaviours to operators with "compat" latyer, that could make them work the same for earlier versions of Airlfow. Also this is very much in line with similar "compat" packages for Python.
Thoughts?
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r752746375
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
Yeah. I do not feel strong for the other name. can be also signature change - no problem with that.
--
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
[GitHub] [airflow] github-actions[bot] commented on pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#issuecomment-1023755225
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r751689583
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
One (easy but a bit tough) solution to that is that we do the same as in "apply_defaults" case: Adding apache-airflow >=2.3.0 to all providers. But this also means that we should not merge that change until **just before** 2.3.0 branch is cut off and coordinate relasing all providers to be compatible with >= 2.3.0 **just** before 2.3.0 gets released.
This is possible option, but I am not sure if adding "on_kill" alone is strong enough justrification for such a move.
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r751711095
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
> The following change should handle airflow<2.3 -- it checks the function signature and see if it has context in it or no
Yeah. I understand that. I am thinking more about implementation of operators in providers. If people will start taking advantage of the context, their implementations might start (even accidentally) depend on context being available to perform "proper" cleanup. I think in pretty much all cases this will be the case (why else would you use it?). Either you use context to do the proper cleanup, or you leave stuff behind. If the implemetnation "depends" on the on_kill having context it might make those future operators provide poor experience on < 2.3.0 even accidentally.
I see why we want to implement it - and I am not at all opposing it, I am just thinking how to "make sure" we have some protections so that somoene who did not yet migrate to 2.3.0 will use providers released after this change (that might rely on the context being present) and got memory/pod leaks etc..
I thought a bit on that and another option is to "force" Airflow >= 2.3.0" for any provider that declares 'context' in on_kill - this can be automateded with pre-commit check. This way we add the >= 2.3.0 limitation only for those providers that use the context.
For example the first time we implement `on_kill(context)` in KubernetesPodOperator, we add `apache-airflow >= 2.3.0`. - so that it will not be usable in 2.2.2 for example
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r751686441
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
I do not yet know :( .. My first thought is though - how to make sure that "crucial" functionality does not depend on the context being provided.
Basically every on_kill implementation that will use `context` will have to do something like that:
```
if context:
# do this and perform some actions
else:
# hmmmm what should we do there if context is missing?
```
My initial thought is;
a) how to make sure that every operator does the if context (bringing back - currently disabled - MyPy and adding Context as TypedDict derivation migh help with that)
b) how to make sure that the "# do this and perform some actions" does not become "really mandatory",
For the b) I can imagine that you will implement some handling in the "if context" that will rended such operator virtually useless when context is None. Such operators can leak memory, leave the pods behind etc. etc. And while those operators will be "technically compatible" with Airlfow < 2.3.0, they will provide a very poor experience there.
I have no answers yet, This is just a feeling that we somehow have to address those two problems.
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r751686441
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
I do not yet know :( .. My first thought is though - how to make sure that "crucial" functionality does not depend on the context being provided.
Basically every on_kill implementation that will use `context` will have to do something like that:
```
if context:
# do this and perform some actions
else:
# hmmmm what should we do there if context is missing?
```
My initial thought is;
a) how to make sure that every operator does the `if context` (bringing back - currently disabled - MyPy and adding Context as TypedDict derivation migh help with that)
b) how to make sure that the "# do this and perform some actions" does not become "actually mandatory",
For the b) I can imagine that you will implement some handling in the "if context" that will render such operator virtually useless when context is None. Such operators can leak memory, leave the pods behind etc. etc. And while those operators will be "technically compatible" with Airlfow < 2.3.0, they will provide a very poor experience there.
I have no answers yet, This is just a feeling that we somehow have to address those two problems.
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r751689583
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
One (easy but a bit "tough") solution to that is that we do the same as in "apply_defaults" case: Adding apache-airflow >=2.3.0 to all providers. But this also means that we should not merge that change until **just before** 2.3.0 branch is cut off and coordinate relasing all providers to be compatible with >= 2.3.0 **just before** 2.3.0 gets released.
This is possible option, but I am not sure if adding "on_kill" alone is strong enough justification for such a move.
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r752556309
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
One other comment here:
The "metaclass" thingie is of course very complex and I am not really sure if the case is 'strong enough' for it. For me also adding the > 2.3.0 limits automatically for providers that use "on_kill(context) is acceptable approach. It's also way simpler and faster to implemetne and test.
It also has the additional benefit, that we can gently "push" the users to newer Airflow versions - which is generally speaking a "nice" side-effect of such limits.
But if we go with "bumping the min airflow version" approach - one thing to consider is actually using a different method and leave `on_kill` as is.
For example we could add a new 'on_terminate(context)` method that will only be in Airlfow 2.3.0+. That would be more explicit and difficult to miss - and that would be a real "new feature" added, rather than `kind-a-compatible-augmented-old-feature`. I think that might be far less confusing and easier to explain.
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
One other comment here:
The "metaclass" thingie is of course very complex and I am not really sure if the case is 'strong enough' for it. For me also adding the > 2.3.0 limits automatically for providers that use "on_kill(context) is acceptable approach. It's also way simpler and faster to implemetne and test.
It also has the additional benefit, that we can gently "push" the users to newer Airflow versions - which is generally speaking a "nice" side-effect of such limits.
But if we go with "bumping the min airflow version" approach - one thing to consider is actually using a different method and leave `on_kill` as is.
For example we could add a new `on_terminate(context)` method that will only be in Airlfow 2.3.0+. That would be more explicit and difficult to miss - and that would be a real "new feature" added, rather than `kind-a-compatible-augmented-old-feature`. I think that might be far less confusing and easier to explain.
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r752556309
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
One other comment here:
The "metaclass" thingie is of course very complex and I am not really sure if the case is 'strong enough' for it. For me also adding the > 2.3.0 limits automatically for providers that use `on_kill(context)` is acceptable approach. It's also way simpler and faster to implement and test.
It also has the additional benefit, that we can gently "push" the users to newer Airflow versions - which is generally speaking a "nice" side-effect of such limits.
But if we go with "bumping the min airflow version" approach - one thing to consider is actually using a different method and leave `on_kill` as is.
For example we could add a new `on_terminate(context)` method that will only be in Airlfow 2.3.0+. That would be more explicit and difficult to miss - and that would be a real "new feature" added, rather than `kind-a-compatible-augmented-old-feature`. I think that might be far less confusing and easier to explain.
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r751782933
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
Hmm. I went to sleep but could not - started to think about it and I think there is a possibility to implement the "meta-class" behaviour.
The solution has a nice property thatt it coulld enable us in the future to add more such API-breaking changes to providers but keep a compatibility layer with earlier airlfow versions, with very little effort.
1) we could introiduce a new package `airflow-proviiders-compat`
2) The new providers would have the "compat" as dependency
3) The `compat` package can contain some common classes that can be used in providers. For example in this case that could be BaseContextKill class witt the meta-class behaviour of passing the context from "execute" to "on_kill". That could also be done with decorators not classes I think.
4) all new operators implemeting on_kill(context) should have both BaseOperator and BaseContextKill as parents;
5) The`BaseContextKill` meta class could do all the monkey-patching for airflow < 2.3
Then any operator that wants to use the on_kill(context) could just do this (and we can also autoamatically verify it).
```
class KubernetesPodOpertor(BaseOperator,BaseContextKill):
def on_kill(context):
,,,,,
```
This way the new providers (with nice clean-up) could continue to use the kill-with-context even for Airflow 2.1.
This might be too much to handle **just** the "context" addition, but it also opens up some interesting possibilities of adding other behaviours to operators with "compat" latyer, that could make them work the same for earlier versions of Airlfow. Also this is very much in line with similar "compat" packages for Python.
Thoughts?
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r751686441
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
I do not yet know :( .. My first thought is though - how to make sure that "crucial" functionality does not depend on the context being provided.
Basically every on_kill implementation that will use `context` will have to do something like that:
```
if context:
# do this and perform some actions
else:
# hmmmm what should we do there if context is missing?
```
My initial thought is;
a) how to make sure that every operator does the if context (bringing back - currently disabled - MyPy and adding Context as TypedDict derivation migh help with that)
b) how to make sure that the "# do this and perform some actions" does not become "really mandatory",
For the b) I can imagine that you will implement some handling in the "if context" that will render such operator virtually useless when context is None. Such operators can leak memory, leave the pods behind etc. etc. And while those operators will be "technically compatible" with Airlfow < 2.3.0, they will provide a very poor experience there.
I have no answers yet, This is just a feeling that we somehow have to address those two problems.
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r756469139
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
Yep. Very much so - all those require either using new ariflow version or some other "common library" - I'd say forcing airlow >=2.3 has some nice properties here (no need to do complex workaround and we are promoting upgrading).
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r753602666
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
another option would be to make `context` dictionary available as an operator attribute perhaps as a property.
we could count on `TaskInstance` to set the attribtue after building the template context.
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r752549672
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
I have never taken the time to understand meta classes, though I know we use them a bit here. I need will have to do that and then come back to this. Thanks for thinking this through and trying to find a good solution.
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r751712579
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
> I thought a bit on that and another option is to "force" Airflow >= 2.3.0" for any provider that declares 'context' in on_kill - this can be automateded with pre-commit check. This way we add the >= 2.3.0 limitation only for those providers that use the context.
smart :)
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r751689583
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
One (easy but a bit "tough") solution to that is that we do the same as in "apply_defaults" case: Adding apache-airflow >=2.3.0 to all providers. But this also means that we should not merge that change until **just before** 2.3.0 branch is cut off and coordinate relasing all providers to be compatible with >= 2.3.0 **just before** 2.3.0 gets released.
This is possible option, but I am not sure if adding "on_kill" alone is strong enough justrification for such a move.
--
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
[GitHub] [airflow] dstandish commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r751666934
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
@potiuk can you clarify
> but I think this should be more than optional parameter .
--
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
[GitHub] [airflow] potiuk commented on a change in pull request #19662: BaseOperator.on_kill should optionally accept airflow context
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #19662:
URL: https://github.com/apache/airflow/pull/19662#discussion_r751737465
##########
File path: airflow/models/baseoperator.py
##########
@@ -1000,7 +1000,7 @@ def post_execute(self, context: Any, result: Any = None):
if self._post_execute_hook is not None:
self._post_execute_hook(context, result)
- def on_kill(self) -> None:
Review comment:
Ah no... BaseOperator will come from Airlfow <2.3 in this case. ....But maybe we can solve it somehow?
--
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