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/21 00:01:03 UTC
[GitHub] [airflow] sagmansercan opened a new pull request #22389: make operator's execution_timeout configurable
sagmansercan opened a new pull request #22389:
URL: https://github.com/apache/airflow/pull/22389
This PR aims to make the execution_timeout attribute to be configurable globally via airflow.cfg.
* The default value is still `None`. Users are expected to
define a positive integer value to be passed into timedelta object
to set the timeout in terms of seconds by default, via configuration.
* If the key is missing or is set to a non-positive value, then it is
considered as `None`.
* Added `gettimedelta` method in configuration to be used in abstractoperator
to get timedelta or None type object. The method raises exceptions
for the values that are not convertible to integer and/or the values
too large to be converted to C int.
* Sample config cases are added into unit tests.
Closes #18578
--
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 #22389: make operator's execution_timeout configurable
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #22389:
URL: https://github.com/apache/airflow/pull/22389#discussion_r839421774
##########
File path: airflow/configuration.py
##########
@@ -608,6 +609,48 @@ def getjson(self, section, key, fallback=_UNSET, **kwargs) -> Union[dict, list,
except JSONDecodeError as e:
raise AirflowConfigException(f'Unable to parse [{section}] {key!r} as valid json') from e
+ def gettimedelta(self, section, key, fallback=None, **kwargs) -> Optional[datetime.timedelta]:
+ """
+ Gets the config value for the given section and key, and converts it into datetime.timedelta object.
+ If the key is missing, then it is considered as `None`.
+
+ :param section: the section from the config
+ :param key: the key defined in the given section
+ :param fallback: fallback value when no config value is given, defaults to None
+ :raises AirflowConfigException: raised because ValueError or OverflowError
+ :return: datetime.timedelta(seconds=<config_value>) or None
+ """
+ val = self.get(section, key, fallback=fallback, **kwargs)
+
+ if val:
+ # the given value must be convertible to integer
+ try:
+ int_val = int(val)
+ except ValueError:
+ raise AirflowConfigException(
+ f'Failed to convert value to int. Please check "{key}" key in "{section}" section. '
+ f'Current value: "{val}".'
+ )
+
+ # the given value must be positive.
+ if int_val <= 0:
+ raise AirflowConfigException(
+ f'Failed to convert value to timedelta in `seconds`. '
+ f'Value must be greater than zero. '
+ f'Please check "{key}" key in "{section}" section. Current value: "{val}".'
+ )
Review comment:
Why? timedelta support negative values just fine.
--
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] sagmansercan commented on a change in pull request #22389: make operator's execution_timeout configurable
Posted by GitBox <gi...@apache.org>.
sagmansercan commented on a change in pull request #22389:
URL: https://github.com/apache/airflow/pull/22389#discussion_r831413837
##########
File path: airflow/configuration.py
##########
@@ -608,6 +609,43 @@ def getjson(self, section, key, fallback=_UNSET, **kwargs) -> Union[dict, list,
except JSONDecodeError as e:
raise AirflowConfigException(f'Unable to parse [{section}] {key!r} as valid json') from e
+ def gettimedelta(self, section, key, fallback=None, **kwargs) -> Optional[datetime.timedelta]:
+ """
+ Gets the config value for the given section and key, and converts it into datetime.timedelta object.
+ If the key is missing or set to a non-positive value, then it is considered as `None`.
+
+ :param section: the section from the config
+ :param key: the key defined in the given section
+ :param fallback: fallback value when no config value is given, defaults to None
+ :raises AirflowConfigException: raised because ValueError or OverflowError
+ :return: datetime.timedelta(seconds=<config_value>) or None
+ """
+ val = self.get(section, key, fallback=fallback, **kwargs)
+
+ if val:
+ # the given value must be convertible to integer
+ try:
+ int_val = int(val)
+ except ValueError:
+ raise AirflowConfigException(
+ f'Failed to convert value to int. Please check "{key}" key in "{section}" section. '
+ f'Current value: "{val}".'
+ )
+
+ # the given value must be positive. Otherwise fallback value is returned.
+ if int_val <= 0:
+ return fallback
Review comment:
done ✅
--
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] Bowrna commented on pull request #22389: make operator's execution_timeout configurable
Posted by GitBox <gi...@apache.org>.
Bowrna commented on pull request #22389:
URL: https://github.com/apache/airflow/pull/22389#issuecomment-1082696740
@sagmansercan you have to fix the failing `static check` in the black formatting. Please check.
--
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 #22389: make operator's execution_timeout configurable
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #22389:
URL: https://github.com/apache/airflow/pull/22389#discussion_r831371212
##########
File path: airflow/config_templates/config.yml
##########
@@ -361,6 +361,15 @@
type: string
example: ~
default: "downstream"
+ - name: default_execution_timeout
Review comment:
I think it should be
```suggestion
- name: default_task_execution_timeout
```
like: `default_task_retries`, `default_task_weight_rule`, etc...
--
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] sagmansercan commented on a change in pull request #22389: make operator's execution_timeout configurable
Posted by GitBox <gi...@apache.org>.
sagmansercan commented on a change in pull request #22389:
URL: https://github.com/apache/airflow/pull/22389#discussion_r831409759
##########
File path: airflow/config_templates/config.yml
##########
@@ -361,6 +361,15 @@
type: string
example: ~
default: "downstream"
+ - name: default_execution_timeout
Review comment:
renamed all the configuration parameter and variable names accordingly ✅
--
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] sagmansercan commented on a change in pull request #22389: make operator's execution_timeout configurable
Posted by GitBox <gi...@apache.org>.
sagmansercan commented on a change in pull request #22389:
URL: https://github.com/apache/airflow/pull/22389#discussion_r831562453
##########
File path: airflow/configuration.py
##########
@@ -608,6 +609,48 @@ def getjson(self, section, key, fallback=_UNSET, **kwargs) -> Union[dict, list,
except JSONDecodeError as e:
raise AirflowConfigException(f'Unable to parse [{section}] {key!r} as valid json') from e
+ def gettimedelta(self, section, key, fallback=None, **kwargs) -> Optional[datetime.timedelta]:
+ """
+ Gets the config value for the given section and key, and converts it into datetime.timedelta object.
+ If the key is missing or set to a non-positive value, then it is considered as `None`.
+
+ :param section: the section from the config
+ :param key: the key defined in the given section
+ :param fallback: fallback value when no config value is given, defaults to None
+ :raises AirflowConfigException: raised because ValueError or OverflowError
+ :return: datetime.timedelta(seconds=<config_value>) or None
+ """
+ val = self.get(section, key, fallback=fallback, **kwargs)
+
+ if val:
+ # the given value must be convertible to integer
+ try:
+ int_val = int(val)
+ except ValueError:
+ raise AirflowConfigException(
+ f'Failed to convert value to int. Please check "{key}" key in "{section}" section. '
+ f'Current value: "{val}".'
+ )
+
+ # the given value must be positive. Otherwise fallback value is returned.
+ if int_val <= 0:
+ raise AirflowConfigException(
Review comment:
Thanks for the warning. The description is up to date now ✅
--
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] sagmansercan commented on a change in pull request #22389: make operator's execution_timeout configurable
Posted by GitBox <gi...@apache.org>.
sagmansercan commented on a change in pull request #22389:
URL: https://github.com/apache/airflow/pull/22389#discussion_r831410437
##########
File path: airflow/config_templates/config.yml
##########
@@ -361,6 +361,15 @@
type: string
example: ~
default: "downstream"
+ - name: default_execution_timeout
+ description: |
+ The default execution_timeout value for the operators. Expected positive integer value to
+ be passed into timedelta as seconds. The missing and the non-positive values are considered as None,
+ means that the operators are never timed out.
+ version_added: 2.3.1
Review comment:
done ✅
--
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] pingzh commented on a change in pull request #22389: make operator's execution_timeout configurable
Posted by GitBox <gi...@apache.org>.
pingzh commented on a change in pull request #22389:
URL: https://github.com/apache/airflow/pull/22389#discussion_r835673203
##########
File path: airflow/configuration.py
##########
@@ -608,6 +609,48 @@ def getjson(self, section, key, fallback=_UNSET, **kwargs) -> Union[dict, list,
except JSONDecodeError as e:
raise AirflowConfigException(f'Unable to parse [{section}] {key!r} as valid json') from e
+ def gettimedelta(self, section, key, fallback=None, **kwargs) -> Optional[datetime.timedelta]:
Review comment:
nit, wondering if we can call it `get_timedelta` given there are two `t`s.
--
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] sagmansercan commented on a change in pull request #22389: make operator's execution_timeout configurable
Posted by GitBox <gi...@apache.org>.
sagmansercan commented on a change in pull request #22389:
URL: https://github.com/apache/airflow/pull/22389#discussion_r831399202
##########
File path: airflow/configuration.py
##########
@@ -608,6 +609,43 @@ def getjson(self, section, key, fallback=_UNSET, **kwargs) -> Union[dict, list,
except JSONDecodeError as e:
raise AirflowConfigException(f'Unable to parse [{section}] {key!r} as valid json') from e
+ def gettimedelta(self, section, key, fallback=None, **kwargs) -> Optional[datetime.timedelta]:
+ """
+ Gets the config value for the given section and key, and converts it into datetime.timedelta object.
+ If the key is missing or set to a non-positive value, then it is considered as `None`.
+
+ :param section: the section from the config
+ :param key: the key defined in the given section
+ :param fallback: fallback value when no config value is given, defaults to None
+ :raises AirflowConfigException: raised because ValueError or OverflowError
+ :return: datetime.timedelta(seconds=<config_value>) or None
+ """
+ val = self.get(section, key, fallback=fallback, **kwargs)
+
+ if val:
+ # the given value must be convertible to integer
+ try:
+ int_val = int(val)
+ except ValueError:
+ raise AirflowConfigException(
+ f'Failed to convert value to int. Please check "{key}" key in "{section}" section. '
+ f'Current value: "{val}".'
+ )
+
+ # the given value must be positive. Otherwise fallback value is returned.
+ if int_val <= 0:
+ return fallback
+
+ try:
+ return datetime.timedelta(seconds=int_val)
+ except OverflowError:
+ raise AirflowConfigException(
+ f'Failed to convert value to timedelta in `seconds`. '
+ f'Please check "{key}" key in "{section}" section. Current value: "{val}".'
Review comment:
You're right, I'll include the error message.
--
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 pull request #22389: make operator's execution_timeout configurable
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #22389:
URL: https://github.com/apache/airflow/pull/22389#issuecomment-1073437849
Since it is possible to provide a *None* `execution_timeout`, the config format also need to support 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] sagmansercan commented on pull request #22389: make operator's execution_timeout configurable
Posted by GitBox <gi...@apache.org>.
sagmansercan commented on pull request #22389:
URL: https://github.com/apache/airflow/pull/22389#issuecomment-1081681400
Hi @ashb @eladkal @uranusjr @SamWheating
I've addressed all the comments above, could you please review them again?
--
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] sagmansercan commented on pull request #22389: make operator's execution_timeout configurable
Posted by GitBox <gi...@apache.org>.
sagmansercan commented on pull request #22389:
URL: https://github.com/apache/airflow/pull/22389#issuecomment-1082975404
> @sagmansercan you have to fix the failing `static check` in the black formatting. Please check.
Thank you for warning @Bowrna, seems like it is related to black version abd fixed in https://github.com/apache/airflow/pull/22598
I will try rebasing and see the result after that because it has not resulted from my changes
--
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] sagmansercan commented on a change in pull request #22389: make operator's execution_timeout configurable
Posted by GitBox <gi...@apache.org>.
sagmansercan commented on a change in pull request #22389:
URL: https://github.com/apache/airflow/pull/22389#discussion_r839473755
##########
File path: airflow/configuration.py
##########
@@ -608,6 +609,48 @@ def getjson(self, section, key, fallback=_UNSET, **kwargs) -> Union[dict, list,
except JSONDecodeError as e:
raise AirflowConfigException(f'Unable to parse [{section}] {key!r} as valid json') from e
+ def gettimedelta(self, section, key, fallback=None, **kwargs) -> Optional[datetime.timedelta]:
Review comment:
Oh, now I see your point :) Yes, it may be a little bit annoying, but as you said, it is compatible with the naming convention in the `configuration.py `. I can apply renaming If everyone is okay with _snake_case_, but for now I am leaving it as it is
--
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] sagmansercan commented on a change in pull request #22389: make operator's execution_timeout configurable
Posted by GitBox <gi...@apache.org>.
sagmansercan commented on a change in pull request #22389:
URL: https://github.com/apache/airflow/pull/22389#discussion_r831543894
##########
File path: airflow/configuration.py
##########
@@ -608,6 +609,48 @@ def getjson(self, section, key, fallback=_UNSET, **kwargs) -> Union[dict, list,
except JSONDecodeError as e:
raise AirflowConfigException(f'Unable to parse [{section}] {key!r} as valid json') from e
+ def gettimedelta(self, section, key, fallback=None, **kwargs) -> Optional[datetime.timedelta]:
+ """
+ Gets the config value for the given section and key, and converts it into datetime.timedelta object.
+ If the key is missing or set to a non-positive value, then it is considered as `None`.
+
+ :param section: the section from the config
+ :param key: the key defined in the given section
+ :param fallback: fallback value when no config value is given, defaults to None
+ :raises AirflowConfigException: raised because ValueError or OverflowError
+ :return: datetime.timedelta(seconds=<config_value>) or None
+ """
+ val = self.get(section, key, fallback=fallback, **kwargs)
+
+ if val:
+ # the given value must be convertible to integer
+ try:
+ int_val = int(val)
+ except ValueError:
+ raise AirflowConfigException(
+ f'Failed to convert value to int. Please check "{key}" key in "{section}" section. '
+ f'Current value: "{val}".'
+ )
+
+ # the given value must be positive. Otherwise fallback value is returned.
+ if int_val <= 0:
+ raise AirflowConfigException(
Review comment:
I've just updated the behavior of this logic based on [this comment](https://github.com/apache/airflow/pull/22389#discussion_r831289387). So, config reference has become misleading, sorry about that. Let me revisit the explanations again.
--
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] sagmansercan commented on a change in pull request #22389: make operator's execution_timeout configurable
Posted by GitBox <gi...@apache.org>.
sagmansercan commented on a change in pull request #22389:
URL: https://github.com/apache/airflow/pull/22389#discussion_r831377508
##########
File path: airflow/config_templates/config.yml
##########
@@ -361,6 +361,15 @@
type: string
example: ~
default: "downstream"
+ - name: default_execution_timeout
Review comment:
Thank you, you're quite right.
--
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 #22389: make operator's execution_timeout configurable
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #22389:
URL: https://github.com/apache/airflow/pull/22389#discussion_r831367280
##########
File path: airflow/config_templates/config.yml
##########
@@ -361,6 +361,15 @@
type: string
example: ~
default: "downstream"
+ - name: default_execution_timeout
+ description: |
+ The default execution_timeout value for the operators. Expected positive integer value to
+ be passed into timedelta as seconds. The missing and the non-positive values are considered as None,
+ means that the operators are never timed out.
+ version_added: 2.3.1
Review comment:
```suggestion
version_added: 2.3.0
```
We can target 2.3.0 for this one
--
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] boring-cyborg[bot] commented on pull request #22389: make operator's execution_timeout configurable
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #22389:
URL: https://github.com/apache/airflow/pull/22389#issuecomment-1073377613
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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] sagmansercan commented on a change in pull request #22389: make operator's execution_timeout configurable
Posted by GitBox <gi...@apache.org>.
sagmansercan commented on a change in pull request #22389:
URL: https://github.com/apache/airflow/pull/22389#discussion_r831411667
##########
File path: airflow/configuration.py
##########
@@ -608,6 +609,43 @@ def getjson(self, section, key, fallback=_UNSET, **kwargs) -> Union[dict, list,
except JSONDecodeError as e:
raise AirflowConfigException(f'Unable to parse [{section}] {key!r} as valid json') from e
+ def gettimedelta(self, section, key, fallback=None, **kwargs) -> Optional[datetime.timedelta]:
+ """
+ Gets the config value for the given section and key, and converts it into datetime.timedelta object.
+ If the key is missing or set to a non-positive value, then it is considered as `None`.
+
+ :param section: the section from the config
+ :param key: the key defined in the given section
+ :param fallback: fallback value when no config value is given, defaults to None
+ :raises AirflowConfigException: raised because ValueError or OverflowError
+ :return: datetime.timedelta(seconds=<config_value>) or None
+ """
+ val = self.get(section, key, fallback=fallback, **kwargs)
+
+ if val:
+ # the given value must be convertible to integer
+ try:
+ int_val = int(val)
+ except ValueError:
+ raise AirflowConfigException(
+ f'Failed to convert value to int. Please check "{key}" key in "{section}" section. '
+ f'Current value: "{val}".'
+ )
+
+ # the given value must be positive. Otherwise fallback value is returned.
+ if int_val <= 0:
+ return fallback
+
+ try:
+ return datetime.timedelta(seconds=int_val)
+ except OverflowError:
+ raise AirflowConfigException(
+ f'Failed to convert value to timedelta in `seconds`. '
+ f'Please check "{key}" key in "{section}" section. Current value: "{val}".'
Review comment:
The error message is now included in the 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] sagmansercan commented on a change in pull request #22389: make operator's execution_timeout configurable
Posted by GitBox <gi...@apache.org>.
sagmansercan commented on a change in pull request #22389:
URL: https://github.com/apache/airflow/pull/22389#discussion_r839700305
##########
File path: airflow/configuration.py
##########
@@ -608,6 +609,48 @@ def getjson(self, section, key, fallback=_UNSET, **kwargs) -> Union[dict, list,
except JSONDecodeError as e:
raise AirflowConfigException(f'Unable to parse [{section}] {key!r} as valid json') from e
+ def gettimedelta(self, section, key, fallback=None, **kwargs) -> Optional[datetime.timedelta]:
+ """
+ Gets the config value for the given section and key, and converts it into datetime.timedelta object.
+ If the key is missing, then it is considered as `None`.
+
+ :param section: the section from the config
+ :param key: the key defined in the given section
+ :param fallback: fallback value when no config value is given, defaults to None
+ :raises AirflowConfigException: raised because ValueError or OverflowError
+ :return: datetime.timedelta(seconds=<config_value>) or None
+ """
+ val = self.get(section, key, fallback=fallback, **kwargs)
+
+ if val:
+ # the given value must be convertible to integer
+ try:
+ int_val = int(val)
+ except ValueError:
+ raise AirflowConfigException(
+ f'Failed to convert value to int. Please check "{key}" key in "{section}" section. '
+ f'Current value: "{val}".'
+ )
+
+ # the given value must be positive.
+ if int_val <= 0:
+ raise AirflowConfigException(
+ f'Failed to convert value to timedelta in `seconds`. '
+ f'Value must be greater than zero. '
+ f'Please check "{key}" key in "{section}" section. Current value: "{val}".'
+ )
Review comment:
I thought that in the case of non-positive values, tasks reach the timeout immediately and this did not make sense to me. So, restricted non-positive values. However, I guess I see your point, users can be held responsible for adjusting this parameter accordingly, then we don't need to handle this in the method, right?
--
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] pingzh commented on a change in pull request #22389: make operator's execution_timeout configurable
Posted by GitBox <gi...@apache.org>.
pingzh commented on a change in pull request #22389:
URL: https://github.com/apache/airflow/pull/22389#discussion_r838800949
##########
File path: airflow/configuration.py
##########
@@ -608,6 +609,48 @@ def getjson(self, section, key, fallback=_UNSET, **kwargs) -> Union[dict, list,
except JSONDecodeError as e:
raise AirflowConfigException(f'Unable to parse [{section}] {key!r} as valid json') from e
+ def gettimedelta(self, section, key, fallback=None, **kwargs) -> Optional[datetime.timedelta]:
Review comment:
hi @sagmansercan i was thinking about the naming of this method, `gettimedelta` vs `get_timedelta`.
`gettimedelta` conforms the current naming convention under `conf`, like `getint`. However, there are two `t`s in the naming, making it harder to read. but it is not a big deal
--
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] sagmansercan commented on a change in pull request #22389: make operator's execution_timeout configurable
Posted by GitBox <gi...@apache.org>.
sagmansercan commented on a change in pull request #22389:
URL: https://github.com/apache/airflow/pull/22389#discussion_r836385110
##########
File path: airflow/configuration.py
##########
@@ -608,6 +609,48 @@ def getjson(self, section, key, fallback=_UNSET, **kwargs) -> Union[dict, list,
except JSONDecodeError as e:
raise AirflowConfigException(f'Unable to parse [{section}] {key!r} as valid json') from e
+ def gettimedelta(self, section, key, fallback=None, **kwargs) -> Optional[datetime.timedelta]:
Review comment:
hi, this method aims to provide a simple functionality to take a string value from the config file, then try to convert it into an integer, and pass it into timedelta in seconds. So, it is expected to be returned as _a single timedelta object_. It is very similar to the other helper methods like _getint_, _getfloat_, _getboolean_, etc. So I think the method does not support the operation for multiple `t`s.
If I misunderstood your comment, I would like to hear more on this to understand the use cases.
--
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] sagmansercan edited a comment on pull request #22389: make operator's execution_timeout configurable
Posted by GitBox <gi...@apache.org>.
sagmansercan edited a comment on pull request #22389:
URL: https://github.com/apache/airflow/pull/22389#issuecomment-1082975404
> @sagmansercan you have to fix the failing `static check` in the black formatting. Please check.
Thank you for warning @Bowrna, seems like it is related to the black version and fixed in https://github.com/apache/airflow/pull/22598
I will try rebasing and see the result after that because it has not resulted from my changes
--
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] sagmansercan commented on a change in pull request #22389: make operator's execution_timeout configurable
Posted by GitBox <gi...@apache.org>.
sagmansercan commented on a change in pull request #22389:
URL: https://github.com/apache/airflow/pull/22389#discussion_r831543894
##########
File path: airflow/configuration.py
##########
@@ -608,6 +609,48 @@ def getjson(self, section, key, fallback=_UNSET, **kwargs) -> Union[dict, list,
except JSONDecodeError as e:
raise AirflowConfigException(f'Unable to parse [{section}] {key!r} as valid json') from e
+ def gettimedelta(self, section, key, fallback=None, **kwargs) -> Optional[datetime.timedelta]:
+ """
+ Gets the config value for the given section and key, and converts it into datetime.timedelta object.
+ If the key is missing or set to a non-positive value, then it is considered as `None`.
+
+ :param section: the section from the config
+ :param key: the key defined in the given section
+ :param fallback: fallback value when no config value is given, defaults to None
+ :raises AirflowConfigException: raised because ValueError or OverflowError
+ :return: datetime.timedelta(seconds=<config_value>) or None
+ """
+ val = self.get(section, key, fallback=fallback, **kwargs)
+
+ if val:
+ # the given value must be convertible to integer
+ try:
+ int_val = int(val)
+ except ValueError:
+ raise AirflowConfigException(
+ f'Failed to convert value to int. Please check "{key}" key in "{section}" section. '
+ f'Current value: "{val}".'
+ )
+
+ # the given value must be positive. Otherwise fallback value is returned.
+ if int_val <= 0:
+ raise AirflowConfigException(
Review comment:
I've just updated the behavior of this logic based on [this comment](https://github.com/apache/airflow/pull/22389#discussion_r831289387). So, config reference has become misleading, sorry about that. Let me revisit the explanation again.
##########
File path: airflow/configuration.py
##########
@@ -608,6 +609,48 @@ def getjson(self, section, key, fallback=_UNSET, **kwargs) -> Union[dict, list,
except JSONDecodeError as e:
raise AirflowConfigException(f'Unable to parse [{section}] {key!r} as valid json') from e
+ def gettimedelta(self, section, key, fallback=None, **kwargs) -> Optional[datetime.timedelta]:
+ """
+ Gets the config value for the given section and key, and converts it into datetime.timedelta object.
+ If the key is missing or set to a non-positive value, then it is considered as `None`.
+
+ :param section: the section from the config
+ :param key: the key defined in the given section
+ :param fallback: fallback value when no config value is given, defaults to None
+ :raises AirflowConfigException: raised because ValueError or OverflowError
+ :return: datetime.timedelta(seconds=<config_value>) or None
+ """
+ val = self.get(section, key, fallback=fallback, **kwargs)
+
+ if val:
+ # the given value must be convertible to integer
+ try:
+ int_val = int(val)
+ except ValueError:
+ raise AirflowConfigException(
+ f'Failed to convert value to int. Please check "{key}" key in "{section}" section. '
+ f'Current value: "{val}".'
+ )
+
+ # the given value must be positive. Otherwise fallback value is returned.
+ if int_val <= 0:
+ raise AirflowConfigException(
Review comment:
I've just updated the behavior of this logic based on [this comment](https://github.com/apache/airflow/pull/22389#discussion_r831289387). So, the config reference has become misleading, sorry about that. Let me revisit the explanation again.
--
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] sagmansercan commented on pull request #22389: make operator's execution_timeout configurable
Posted by GitBox <gi...@apache.org>.
sagmansercan commented on pull request #22389:
URL: https://github.com/apache/airflow/pull/22389#issuecomment-1073576408
> Since it is possible to provide a _None_ `execution_timeout`, the config format also need to support that.
Thanks for the comment, could you provide more details on your suggestion with an example use case?
The default behavior is already `None`. One can leave this section key empty, or even not define it in its cfg file. This approach tries to cover the "providing None" case with an empty value.
Are you suggesting that the string "None" should also be handled? If so, It would be great to hear what is the difference between "empty string" and "None" is, based on use cases. Additionally, how can we achieve that without explicitly checking if the given string is equal to "None". Also `eval` does this, but either way both of them do not sound right to me.
--
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] SamWheating commented on a change in pull request #22389: make operator's execution_timeout configurable
Posted by GitBox <gi...@apache.org>.
SamWheating commented on a change in pull request #22389:
URL: https://github.com/apache/airflow/pull/22389#discussion_r831538637
##########
File path: airflow/configuration.py
##########
@@ -608,6 +609,48 @@ def getjson(self, section, key, fallback=_UNSET, **kwargs) -> Union[dict, list,
except JSONDecodeError as e:
raise AirflowConfigException(f'Unable to parse [{section}] {key!r} as valid json') from e
+ def gettimedelta(self, section, key, fallback=None, **kwargs) -> Optional[datetime.timedelta]:
+ """
+ Gets the config value for the given section and key, and converts it into datetime.timedelta object.
+ If the key is missing or set to a non-positive value, then it is considered as `None`.
+
+ :param section: the section from the config
+ :param key: the key defined in the given section
+ :param fallback: fallback value when no config value is given, defaults to None
+ :raises AirflowConfigException: raised because ValueError or OverflowError
+ :return: datetime.timedelta(seconds=<config_value>) or None
+ """
+ val = self.get(section, key, fallback=fallback, **kwargs)
+
+ if val:
+ # the given value must be convertible to integer
+ try:
+ int_val = int(val)
+ except ValueError:
+ raise AirflowConfigException(
+ f'Failed to convert value to int. Please check "{key}" key in "{section}" section. '
+ f'Current value: "{val}".'
+ )
+
+ # the given value must be positive. Otherwise fallback value is returned.
+ if int_val <= 0:
+ raise AirflowConfigException(
Review comment:
Is this error handled anywhere, or will this cause unhandled exceptions? The config reference entry says that:
```
The missing and the non-positive values are considered as None, means that the operators are never timed out.
```
Which suggests that a missing or non-positive value shouldn't raise an error and should instead just return None.
--
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] sagmansercan commented on a change in pull request #22389: make operator's execution_timeout configurable
Posted by GitBox <gi...@apache.org>.
sagmansercan commented on a change in pull request #22389:
URL: https://github.com/apache/airflow/pull/22389#discussion_r831364124
##########
File path: airflow/configuration.py
##########
@@ -608,6 +609,43 @@ def getjson(self, section, key, fallback=_UNSET, **kwargs) -> Union[dict, list,
except JSONDecodeError as e:
raise AirflowConfigException(f'Unable to parse [{section}] {key!r} as valid json') from e
+ def gettimedelta(self, section, key, fallback=None, **kwargs) -> Optional[datetime.timedelta]:
+ """
+ Gets the config value for the given section and key, and converts it into datetime.timedelta object.
+ If the key is missing or set to a non-positive value, then it is considered as `None`.
+
+ :param section: the section from the config
+ :param key: the key defined in the given section
+ :param fallback: fallback value when no config value is given, defaults to None
+ :raises AirflowConfigException: raised because ValueError or OverflowError
+ :return: datetime.timedelta(seconds=<config_value>) or None
+ """
+ val = self.get(section, key, fallback=fallback, **kwargs)
+
+ if val:
+ # the given value must be convertible to integer
+ try:
+ int_val = int(val)
+ except ValueError:
+ raise AirflowConfigException(
+ f'Failed to convert value to int. Please check "{key}" key in "{section}" section. '
+ f'Current value: "{val}".'
+ )
+
+ # the given value must be positive. Otherwise fallback value is returned.
+ if int_val <= 0:
+ return fallback
Review comment:
I thought that it is better not to raise an error for these values and it does not affect the current behavior. However, you're also right. It would be much simpler to not have this logic. I am going to change this, as you've suggested, including zero.
--
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] ashb commented on a change in pull request #22389: make operator's execution_timeout configurable
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #22389:
URL: https://github.com/apache/airflow/pull/22389#discussion_r831290550
##########
File path: airflow/configuration.py
##########
@@ -608,6 +609,43 @@ def getjson(self, section, key, fallback=_UNSET, **kwargs) -> Union[dict, list,
except JSONDecodeError as e:
raise AirflowConfigException(f'Unable to parse [{section}] {key!r} as valid json') from e
+ def gettimedelta(self, section, key, fallback=None, **kwargs) -> Optional[datetime.timedelta]:
+ """
+ Gets the config value for the given section and key, and converts it into datetime.timedelta object.
+ If the key is missing or set to a non-positive value, then it is considered as `None`.
+
+ :param section: the section from the config
+ :param key: the key defined in the given section
+ :param fallback: fallback value when no config value is given, defaults to None
+ :raises AirflowConfigException: raised because ValueError or OverflowError
+ :return: datetime.timedelta(seconds=<config_value>) or None
+ """
+ val = self.get(section, key, fallback=fallback, **kwargs)
+
+ if val:
+ # the given value must be convertible to integer
+ try:
+ int_val = int(val)
+ except ValueError:
+ raise AirflowConfigException(
+ f'Failed to convert value to int. Please check "{key}" key in "{section}" section. '
+ f'Current value: "{val}".'
+ )
+
+ # the given value must be positive. Otherwise fallback value is returned.
+ if int_val <= 0:
+ return fallback
+
+ try:
+ return datetime.timedelta(seconds=int_val)
+ except OverflowError:
+ raise AirflowConfigException(
+ f'Failed to convert value to timedelta in `seconds`. '
+ f'Please check "{key}" key in "{section}" section. Current value: "{val}".'
Review comment:
This error doesn't give the user any clue as to what the error actually is -- as the value it certainly looks like it is a valid number.
--
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] ashb commented on a change in pull request #22389: make operator's execution_timeout configurable
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #22389:
URL: https://github.com/apache/airflow/pull/22389#discussion_r831289387
##########
File path: airflow/configuration.py
##########
@@ -608,6 +609,43 @@ def getjson(self, section, key, fallback=_UNSET, **kwargs) -> Union[dict, list,
except JSONDecodeError as e:
raise AirflowConfigException(f'Unable to parse [{section}] {key!r} as valid json') from e
+ def gettimedelta(self, section, key, fallback=None, **kwargs) -> Optional[datetime.timedelta]:
+ """
+ Gets the config value for the given section and key, and converts it into datetime.timedelta object.
+ If the key is missing or set to a non-positive value, then it is considered as `None`.
+
+ :param section: the section from the config
+ :param key: the key defined in the given section
+ :param fallback: fallback value when no config value is given, defaults to None
+ :raises AirflowConfigException: raised because ValueError or OverflowError
+ :return: datetime.timedelta(seconds=<config_value>) or None
+ """
+ val = self.get(section, key, fallback=fallback, **kwargs)
+
+ if val:
+ # the given value must be convertible to integer
+ try:
+ int_val = int(val)
+ except ValueError:
+ raise AirflowConfigException(
+ f'Failed to convert value to int. Please check "{key}" key in "{section}" section. '
+ f'Current value: "{val}".'
+ )
+
+ # the given value must be positive. Otherwise fallback value is returned.
+ if int_val <= 0:
+ return fallback
Review comment:
Why do we need to support negative for none? It feels simpler to have "blank" be None, and negative be an error.
--
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