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 2022/03/31 06:06:50 UTC
[GitHub] [airflow] dstandish opened a new pull request #22641: Disable SLAs for mapped operators
dstandish opened a new pull request #22641:
URL: https://github.com/apache/airflow/pull/22641
When trying to update SLA logic to handle mapped operators we discovered some odd behavior and decided to defer adding support for SLAs with mapped tasks.
--
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] eladkal commented on a change in pull request #22641: Disable SLAs for mapped operators
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #22641:
URL: https://github.com/apache/airflow/pull/22641#discussion_r839410735
##########
File path: airflow/models/mappedoperator.py
##########
@@ -278,6 +278,11 @@ def __attrs_post_init__(self):
for k, v in self.partial_kwargs.items():
if k in self.template_fields:
XComArg.apply_upstream_relationship(self, v)
+ if self.partial_kwargs.get('sla'):
+ raise AirflowException(
+ f"SLAs are unsupported with mapped tasks. Please set `sla=None` for task "
+ f"{self.task_id!r}."
+ )
Review comment:
Should we consider chaning it to a notice/warnning and set it to None rather than raising exception?
--
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 #22641: Disable SLAs for mapped operators
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #22641:
URL: https://github.com/apache/airflow/pull/22641#discussion_r839228853
##########
File path: airflow/models/mappedoperator.py
##########
@@ -278,6 +278,11 @@ def __attrs_post_init__(self):
for k, v in self.partial_kwargs.items():
if k in self.template_fields:
XComArg.apply_upstream_relationship(self, v)
+ if self.partial_kwargs.get('sla'):
+ raise AirflowException(
+ f"SLAs are unsupported with mapped tasks. Please set `sla=None` for task "
+ f"{self.task_id!r}."
+ )
Review comment:
yeah true
--
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] uranusjr commented on a change in pull request #22641: Disable SLAs for mapped operators
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #22641:
URL: https://github.com/apache/airflow/pull/22641#discussion_r839216124
##########
File path: airflow/models/mappedoperator.py
##########
@@ -278,6 +278,11 @@ def __attrs_post_init__(self):
for k, v in self.partial_kwargs.items():
if k in self.template_fields:
XComArg.apply_upstream_relationship(self, v)
+ if self.partial_kwargs.get('sla'):
+ raise AirflowException(
+ f"SLAs are unsupported with mapped tasks. Please set `sla=None` for task "
+ f"{self.task_id!r}."
+ )
Review comment:
Would it be cleaner to do this in `partial()` in `baseoperator.py` instead?
--
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 #22641: Disable SLAs for mapped operators
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #22641:
URL: https://github.com/apache/airflow/pull/22641#discussion_r839220775
##########
File path: airflow/models/mappedoperator.py
##########
@@ -278,6 +278,11 @@ def __attrs_post_init__(self):
for k, v in self.partial_kwargs.items():
if k in self.template_fields:
XComArg.apply_upstream_relationship(self, v)
+ if self.partial_kwargs.get('sla'):
+ raise AirflowException(
+ f"SLAs are unsupported with mapped tasks. Please set `sla=None` for task "
+ f"{self.task_id!r}."
+ )
Review comment:
i am only scratching the surface with this code so i appreciate the hand holding
--
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 #22641: Disable SLAs for mapped operators
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #22641:
URL: https://github.com/apache/airflow/pull/22641#discussion_r839220454
##########
File path: airflow/models/mappedoperator.py
##########
@@ -278,6 +278,11 @@ def __attrs_post_init__(self):
for k, v in self.partial_kwargs.items():
if k in self.template_fields:
XComArg.apply_upstream_relationship(self, v)
+ if self.partial_kwargs.get('sla'):
+ raise AirflowException(
+ f"SLAs are unsupported with mapped tasks. Please set `sla=None` for task "
+ f"{self.task_id!r}."
+ )
Review comment:
sure can do
--
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] uranusjr commented on a change in pull request #22641: Disable SLAs for mapped operators
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #22641:
URL: https://github.com/apache/airflow/pull/22641#discussion_r839228274
##########
File path: airflow/models/mappedoperator.py
##########
@@ -278,6 +278,11 @@ def __attrs_post_init__(self):
for k, v in self.partial_kwargs.items():
if k in self.template_fields:
XComArg.apply_upstream_relationship(self, v)
+ if self.partial_kwargs.get('sla'):
+ raise AirflowException(
+ f"SLAs are unsupported with mapped tasks. Please set `sla=None` for task "
+ f"{self.task_id!r}."
+ )
Review comment:
Hmm
```pycon
>>> import datetime
>>> bool(datetime.timedelta())
False
```
so we should do `is not None` here instead to not accidentally allowing a zero-duration timedelta.
--
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 #22641: Disable SLAs for mapped operators
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #22641:
URL: https://github.com/apache/airflow/pull/22641#discussion_r839795836
##########
File path: airflow/models/mappedoperator.py
##########
@@ -278,6 +278,11 @@ def __attrs_post_init__(self):
for k, v in self.partial_kwargs.items():
if k in self.template_fields:
XComArg.apply_upstream_relationship(self, v)
+ if self.partial_kwargs.get('sla'):
+ raise AirflowException(
+ f"SLAs are unsupported with mapped tasks. Please set `sla=None` for task "
+ f"{self.task_id!r}."
+ )
Review comment:
That's what I was thinking originally and is probably what would get my vote. But @ashb argued that it's better to force users to set `sla=None` because SLAs already have issues and adding a warning sortof makes them "more broken", as opposed to disallowing their use for this new class of task.
I can see an argument for the fail because (1) it's a new type of task so it's not gonna break anything existing and (2) we already have enough warnings, maybe it's better to force folks to fix so as not to create _more_ noise to filter through.
--
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