You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Xiroo (via GitHub)" <gi...@apache.org> on 2023/10/02 10:31:43 UTC
[PR] Use execution timeout in TaskStateTrigger run method [airflow]
Xiroo opened a new pull request, #34715:
URL: https://github.com/apache/airflow/pull/34715
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
Use execution timeout in task state trigger run method.
Previously, self._timeout_sec = 60 fixed timeout to check if dag run exists.
It caused timeout even if the dag run state is in queued or scheduled due to resource issue like hundreds of dag runs exists and they are pending transitting into running states.
So I changed to use execution_timeout instead of fixed 60 seconds.
But I'm wondering if using execution_timeout makes sense so I want to get some suggestions.
I know the current logic has some bugs.
I want to address the all following issues but I'll do it step by step.
This is the first step of fixing the deferred mode of ExternalTaskSensor
#34204
#34205
#34207
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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
Re: [PR] Add deferrable_timeout to inntialize _timeout_sec of TaskStateTrigger [airflow]
Posted by "Xiroo (via GitHub)" <gi...@apache.org>.
Xiroo commented on PR #34715:
URL: https://github.com/apache/airflow/pull/34715#issuecomment-2031097434
@hussein-awala
Hello,
I hope this message finds you well. I understand you have a busy schedule, and I just wanted to gently remind you about the review of my pull request. You mentioned planning to review it last night, and I was wondering if you had a chance to look at it.
Your guidance is greatly appreciated, and Iām more than willing to make any necessary adjustments.
Thank you very much for your time and support.
--
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
Re: [PR] Use timeout when inntializing _timeout_sec of TaskStateTrigger [airflow]
Posted by "Xiroo (via GitHub)" <gi...@apache.org>.
Xiroo commented on code in PR #34715:
URL: https://github.com/apache/airflow/pull/34715#discussion_r1349613367
##########
airflow/sensors/external_task.py:
##########
@@ -341,6 +341,7 @@ def execute(self, context: Context) -> None:
states=self.allowed_states,
trigger_start_time=utcnow(),
poll_interval=self.poll_interval,
+ timeout=self.timeout,
Review Comment:
@utkarsharma2 Should I change to set default value to 60 sec?
However, I don't understand the meaning of "people 'using' the existing default value of 60 sec kind of breaking change". It's just a default and fixed value so no one could choose to use it.
Also, they even don't know there's default value of 60 if they don't look at the source code or failure log. (Even if they saw the failure log or source code, they can't change it)
--
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
Re: [PR] Use execution timeout in TaskStateTrigger run method [airflow]
Posted by "Xiroo (via GitHub)" <gi...@apache.org>.
Xiroo closed pull request #34715: Use execution timeout in TaskStateTrigger run method
URL: https://github.com/apache/airflow/pull/34715
--
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
Re: [PR] Use timeout when inntializing _timeout_sec of TaskStateTrigger [airflow]
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #34715:
URL: https://github.com/apache/airflow/pull/34715#discussion_r1352086054
##########
airflow/sensors/external_task.py:
##########
@@ -341,6 +341,7 @@ def execute(self, context: Context) -> None:
states=self.allowed_states,
trigger_start_time=utcnow(),
poll_interval=self.poll_interval,
+ timeout=self.timeout,
Review Comment:
@Xiroo I saw the other comments and as long as we add a new parameter to get the value from the user and they are aware of this change it should be fine. Also, like the @uranusjr said it should be optional and the default of this can be 60sec as before.
##########
airflow/sensors/external_task.py:
##########
@@ -341,6 +341,7 @@ def execute(self, context: Context) -> None:
states=self.allowed_states,
trigger_start_time=utcnow(),
poll_interval=self.poll_interval,
+ timeout=self.timeout,
Review Comment:
@Xiroo I saw the other comments and as long as we add a new parameter to get the value from the user and they are aware of this change it should be fine. Also, like @uranusjr said it should be optional and the default of this can be 60sec as before.
--
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
Re: [PR] Use timeout when inntializing _timeout_sec of TaskStateTrigger [airflow]
Posted by "Xiroo (via GitHub)" <gi...@apache.org>.
Xiroo commented on code in PR #34715:
URL: https://github.com/apache/airflow/pull/34715#discussion_r1352795395
##########
airflow/sensors/external_task.py:
##########
@@ -341,6 +341,7 @@ def execute(self, context: Context) -> None:
states=self.allowed_states,
trigger_start_time=utcnow(),
poll_interval=self.poll_interval,
+ timeout=self.timeout,
Review Comment:
I changed to 60 sec as default value if timeout has value of 'conf.getfloat("sensors", "default_timeout")'
--
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
Re: [PR] Add deferrable_timeout to inntialize _timeout_sec of TaskStateTrigger [airflow]
Posted by "nathadfield (via GitHub)" <gi...@apache.org>.
nathadfield commented on PR #34715:
URL: https://github.com/apache/airflow/pull/34715#issuecomment-1886952539
This might be slightly off topic for this PR but, when talking about the behaviour of sensors in general, I feel like there's a discrepancy between what a timeout does in normal mode versus deferrable.
When the `timeout` is reached in non-deferrable, this causes an [AirflowSensorTimeout](https://airflow.apache.org/docs/apache-airflow/stable/core-concepts/tasks.html#timeouts) and the task fails without retrying, which is the correct behaviour IMO.
I would like to see this be consistent across ALL sensors regardless of whether it is running in deferrable mode or not.
--
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
Re: [PR] Use execution_timeout when inntializing _timeout_sec of TaskStateTrigger [airflow]
Posted by "Xiroo (via GitHub)" <gi...@apache.org>.
Xiroo commented on PR #34715:
URL: https://github.com/apache/airflow/pull/34715#issuecomment-1743440172
I changed to use 'timeout' parameter.
I think I don't have to make this parameter optional since baseoperator has default value of it
https://github.com/apache/airflow/blob/6618c5f90d037d57e9f3bf1e90cd0712426d6caa/airflow/sensors/base.py#L138
and have validating logic to have int or float
https://github.com/apache/airflow/blob/6618c5f90d037d57e9f3bf1e90cd0712426d6caa/airflow/sensors/base.py#L185-L186
--
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
Re: [PR] Use timeout when inntializing _timeout_sec of TaskStateTrigger [airflow]
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #34715:
URL: https://github.com/apache/airflow/pull/34715#discussion_r1343582659
##########
airflow/sensors/external_task.py:
##########
@@ -341,6 +341,7 @@ def execute(self, context: Context) -> None:
states=self.allowed_states,
trigger_start_time=utcnow(),
poll_interval=self.poll_interval,
+ timeout=self.timeout,
Review Comment:
@Xiroo `self.timeout` is getting initialized from `conf.getfloat("sensors", "default_timeout"),` in base sensor and it's default value is `Sensor default timeout, 7 days by default (7 * 24 * 60 * 60).` I don't know what should be a meaningful default but this will be a drastic change for people using the existing default value of 60 sec kind of breaking change IMO.
cc: @uranusjr @hussein-awala
--
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
Re: [PR] Add deferrable_timeout to inntialize _timeout_sec of TaskStateTrigger [airflow]
Posted by "itstalmeez (via GitHub)" <gi...@apache.org>.
itstalmeez commented on PR #34715:
URL: https://github.com/apache/airflow/pull/34715#issuecomment-1843640604
Implemented execution_timeout in place of a fixed 60-second timeout (self._timeout_sec) for checking dag run existence in the ExternalTaskSensor. The previous fixed timeout caused issues when numerous dag runs were pending transition into running states. Now, with execution_timeout, the sensor dynamically adapts to varying execution times.
Seeking feedback on the use of execution_timeout and open to suggestions. Currently addressing deferred mode issues in ExternalTaskSensor step by step. This PR (Pull Request) addresses the first set of fixes for the deferred mode:
#34204
#34205
#34207
Note: Acknowledge known bugs in the current deferred mode logic and plan to address them incrementally. Refer to the Pull Request Guidelines for more details. For significant code changes, adhere to the Airflow Improvement Proposal (AIP) and ASF 3rd Party License Policy. Additionally, document backwards incompatible changes in newsfragments with appropriate naming conventions.
--
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
Re: [PR] Use timeout when inntializing _timeout_sec of TaskStateTrigger [airflow]
Posted by "Xiroo (via GitHub)" <gi...@apache.org>.
Xiroo commented on code in PR #34715:
URL: https://github.com/apache/airflow/pull/34715#discussion_r1343843951
##########
airflow/sensors/external_task.py:
##########
@@ -341,6 +341,7 @@ def execute(self, context: Context) -> None:
states=self.allowed_states,
trigger_start_time=utcnow(),
poll_interval=self.poll_interval,
+ timeout=self.timeout,
Review Comment:
I think 60 seconds don't have any meaning because it is the number fixed at 60 seconds not user's choice. Most of the users don't think too much about it and use it because it is just a fixed value. Rather, it makes users wonder why the deferrable mode's timeout is 60 seconds and not be able to find the answer in the docs, so it make users look at the code like I did.
IMHO, 60 seconds is not a proper value for default value since it does not raise any kind of time error(in general case) when using the non-deferrable sensor with default value. It can be confusing for users when the numbers aren't consistent.
--
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
Re: [PR] Use timeout when inntializing _timeout_sec of TaskStateTrigger [airflow]
Posted by "Xiroo (via GitHub)" <gi...@apache.org>.
Xiroo commented on code in PR #34715:
URL: https://github.com/apache/airflow/pull/34715#discussion_r1343843951
##########
airflow/sensors/external_task.py:
##########
@@ -341,6 +341,7 @@ def execute(self, context: Context) -> None:
states=self.allowed_states,
trigger_start_time=utcnow(),
poll_interval=self.poll_interval,
+ timeout=self.timeout,
Review Comment:
I think 60 seconds don't have any meaning because it is the number fixed at 60 seconds not user's choice. Most of the users don't think too much about it and use it because it is just a fixed value. Rather, it makes users wonder why the deferrable mode's timeout is 60 seconds and not be able to find the answer in the docs, so it make users look at the code like I did.
IMHO, 60 seconds is not a proper value for default value since it does not raise any kind of time error(in general case) when using the sensor with default value. It can be confusing for users when the numbers aren't consistent.
--
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
Re: [PR] Use timeout when inntializing _timeout_sec of TaskStateTrigger [airflow]
Posted by "Xiroo (via GitHub)" <gi...@apache.org>.
Xiroo commented on code in PR #34715:
URL: https://github.com/apache/airflow/pull/34715#discussion_r1363949141
##########
airflow/sensors/external_task.py:
##########
@@ -351,7 +352,7 @@ def execute_complete(self, context, event=None):
self.log.info("External task %s has executed successfully.", self.external_task_id)
return None
elif event["status"] == "timeout":
- raise AirflowException("Dag was not started within 1 minute, assuming fail.")
+ raise AirflowException("Dag was not started within timeout, assuming fail.")
Review Comment:
changed text to f-string with self.timeout
--
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
Re: [PR] Use timeout when inntializing _timeout_sec of TaskStateTrigger [airflow]
Posted by "Xiroo (via GitHub)" <gi...@apache.org>.
Xiroo commented on code in PR #34715:
URL: https://github.com/apache/airflow/pull/34715#discussion_r1343843951
##########
airflow/sensors/external_task.py:
##########
@@ -341,6 +341,7 @@ def execute(self, context: Context) -> None:
states=self.allowed_states,
trigger_start_time=utcnow(),
poll_interval=self.poll_interval,
+ timeout=self.timeout,
Review Comment:
I think 60 seconds don't have any meaning because it is the number fixed at 60 seconds not user's choice. Most of the users don't think too much about it and use it because it is just a fixed value. Rather, it makes users wonder why the deferrable mode's timeout is 60 seconds and not be able to find the answer in the docs, so it make users look at the code like I did.
IMHO, 60 seconds is not a proper value for default value since it does not raise any kind of time error(in general case) when using the non-deferrable sensor with default value. It can be confusing for users when the numbers aren't consistent.
I'm even curious why 60 was chosen at first. š¤
--
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
Re: [PR] Use timeout when inntializing _timeout_sec of TaskStateTrigger [airflow]
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on PR #34715:
URL: https://github.com/apache/airflow/pull/34715#issuecomment-1783291668
> @hussein-awala Hello, I changed to use timeout parameter and set default value of 60 if timeout has value of 'conf.getfloat("sensors", "default_timeout")'.
By using timeout parameter you will have the same problem I explained before, where:
```
min(self.start_date + execution_timeout, self.trigger_timeout)
```
the `trigger_timeout` in this formula is calculated from the timeout parameter:
https://github.com/apache/airflow/blob/b75f9e880614fa0427e7d24a1817955f5de658b3/airflow/models/taskinstance.py#L2454-L2457
I suggest adding a new parameter to the trigger to override the timeout instead of using `timeout ` param.
--
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
Re: [PR] Add deferrable_timeout to inntialize _timeout_sec of TaskStateTrigger [airflow]
Posted by "Xiroo (via GitHub)" <gi...@apache.org>.
Xiroo commented on PR #34715:
URL: https://github.com/apache/airflow/pull/34715#issuecomment-2007580451
@hussein-awala
From the behavior you mentioned, it seems that whether I use execution_timeout or timeout to set _timeout_sec, they both lead to the same termination conditions. Upon further reflection, I'm questioning the need for explicitly checking if the dag_run is in a running state using _timeout_sec. This check appears to produce an outcome that overlaps with what is already achieved through the existing timeout parameter of ExeternalTaskSensor.
I want to change the async def run method internal loop logic like following .
FROM
```python
while True:
delta = utcnow() - self.trigger_start_time
if delta.total_seconds() < self._timeout_sec:
# mypy confuses typing here
if await self.count_running_dags() == 0: # type: ignore[call-arg]
self.log.info("Waiting for DAG to start execution...")
await asyncio.sleep(self.poll_interval)
else:
yield TriggerEvent({"status": "timeout"})
return
# mypy confuses typing here
if await self.count_tasks() == len(self.execution_dates): # type: ignore[call-arg]
yield TriggerEvent({"status": "success"})
return
self.log.info("Task is still running, sleeping for %s seconds...", self.poll_interval)
await asyncio.sleep(self.poll_interval)
```
TO
```python
while True:
if await self.count_running_dags() == 0: # type: ignore[call-arg]
self.log.info("Waiting for DAG to start execution...")
await asyncio.sleep(self.poll_interval)
# mypy confuses typing here
if await self.count_tasks() == len(self.execution_dates): # type: ignore[call-arg]
yield TriggerEvent({"status": "success"})
return
self.log.info("Task is still running, sleeping for %s seconds...", self.poll_interval)
await asyncio.sleep(self.poll_interval)
```
Instead of making immediate changes, I try adding a deprecation message in this PR. I believe this approach will allow for a smoother transition and provide users with adequate notice before the functionality is eventually removed.
If there's anything I've misunderstood, please let me know.
--
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
Re: [PR] Use timeout when inntializing _timeout_sec of TaskStateTrigger [airflow]
Posted by "Xiroo (via GitHub)" <gi...@apache.org>.
Xiroo commented on code in PR #34715:
URL: https://github.com/apache/airflow/pull/34715#discussion_r1352795395
##########
airflow/sensors/external_task.py:
##########
@@ -341,6 +341,7 @@ def execute(self, context: Context) -> None:
states=self.allowed_states,
trigger_start_time=utcnow(),
poll_interval=self.poll_interval,
+ timeout=self.timeout,
Review Comment:
I set the 60 sec as default value if timeout has value of 'conf.getfloat("sensors", "default_timeout")'
--
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
Re: [PR] Add deferrable_timeout to inntialize _timeout_sec of TaskStateTrigger [airflow]
Posted by "Xiroo (via GitHub)" <gi...@apache.org>.
Xiroo commented on PR #34715:
URL: https://github.com/apache/airflow/pull/34715#issuecomment-2027940307
@hussein-awala
Hello, I'm waiting for your answer.
Can you recommend how to deal with this?
Or If this issue is resolved by other PR, let me know if I can close this PR.
--
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
Re: [PR] Add deferrable_timeout to inntialize _timeout_sec of TaskStateTrigger [airflow]
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on PR #34715:
URL: https://github.com/apache/airflow/pull/34715#issuecomment-2028424604
> @hussein-awala
>
> Hello, I'm waiting for your answer.
>
> Could you recommend how to deal with this?
>
> Or If this issue is resolved by other PR, let me know if I can close this PR.
I will take a look tonight
--
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
Re: [PR] Add deferrable_timeout to inntialize _timeout_sec of TaskStateTrigger [airflow]
Posted by "Xiroo (via GitHub)" <gi...@apache.org>.
Xiroo commented on PR #34715:
URL: https://github.com/apache/airflow/pull/34715#issuecomment-2068677298
@hussein-awala
Hello,
I hope this message finds you well. I wanted to follow up regarding the pull request I submitted. It has been a few weeks since I last mentioned it, and I want to get your feedback to find the way.
Thank you for your time.
--
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
Re: [PR] Use execution timeout in TaskStateTrigger run method [airflow]
Posted by "Xiroo (via GitHub)" <gi...@apache.org>.
Xiroo commented on PR #34715:
URL: https://github.com/apache/airflow/pull/34715#issuecomment-1743158292
> Looks reasonable 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
Re: [PR] Use timeout when inntializing _timeout_sec of TaskStateTrigger [airflow]
Posted by "Xiroo (via GitHub)" <gi...@apache.org>.
Xiroo commented on code in PR #34715:
URL: https://github.com/apache/airflow/pull/34715#discussion_r1343843951
##########
airflow/sensors/external_task.py:
##########
@@ -341,6 +341,7 @@ def execute(self, context: Context) -> None:
states=self.allowed_states,
trigger_start_time=utcnow(),
poll_interval=self.poll_interval,
+ timeout=self.timeout,
Review Comment:
I think 60 seconds doesn't have any meaning because it is the number fixed at 60 seconds not user's choice. Most of the users don't think too much about it and use it because it is just a fixed value. Rather, it makes users wonder why the deferrable mode's timeout is 60 seconds and not be able to find the answer in the docs, so it makes users look at the code like I did.
IMHO, 60 seconds is not a proper value for default value since sensor does not raise any kind of time error(in general case) when using the non-deferrable sensor with default value. It can be confusing for users when the numbers aren't consistent.
I'm even curious why 60 was chosen at first.š¤
--
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
Re: [PR] Use timeout when inntializing _timeout_sec of TaskStateTrigger [airflow]
Posted by "Xiroo (via GitHub)" <gi...@apache.org>.
Xiroo commented on PR #34715:
URL: https://github.com/apache/airflow/pull/34715#issuecomment-1766343034
@hussein-awala
Hello, I changed to use timeout parameter and set default value of 60 if timeout has value of 'conf.getfloat("sensors", "default_timeout")'.
--
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
Re: [PR] Use timeout when inntializing _timeout_sec of TaskStateTrigger [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #34715:
URL: https://github.com/apache/airflow/pull/34715#discussion_r1363448598
##########
airflow/sensors/external_task.py:
##########
@@ -351,7 +352,7 @@ def execute_complete(self, context, event=None):
self.log.info("External task %s has executed successfully.", self.external_task_id)
return None
elif event["status"] == "timeout":
- raise AirflowException("Dag was not started within 1 minute, assuming fail.")
+ raise AirflowException("Dag was not started within timeout, assuming fail.")
Review Comment:
Is it possible to show the timeout used in the message? I feel this might help debugging.
--
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
Re: [PR] Use timeout when inntializing _timeout_sec of TaskStateTrigger [airflow]
Posted by "Xiroo (via GitHub)" <gi...@apache.org>.
Xiroo commented on PR #34715:
URL: https://github.com/apache/airflow/pull/34715#issuecomment-1787464996
@hussein-awala
Thank you for your feedback.
I added deferrable_timeout parameter instead of overrding timeout.
but I'm wondering if this is proper naming.
--
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
Re: [PR] Use execution timeout in TaskStateTrigger run method [airflow]
Posted by "Xiroo (via GitHub)" <gi...@apache.org>.
Xiroo commented on PR #34715:
URL: https://github.com/apache/airflow/pull/34715#issuecomment-1743171436
@uranusjr
I don't know how to reply to above comment so I wrote a new comment.
I think I don't have to make the argument optional since BaseOperator has the default value of execution_timeout and its type is timedelta or None.
You can see it in [airflow/models/baseoperator.py line763](https://github.com/apache/airflow/blob/main/airflow/models/baseoperator.py#L763)
So I added None case to cover all types of execution_timeout of BaseOperator.
--
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
Re: [PR] Use timeout when inntializing _timeout_sec of TaskStateTrigger [airflow]
Posted by "Xiroo (via GitHub)" <gi...@apache.org>.
Xiroo commented on PR #34715:
URL: https://github.com/apache/airflow/pull/34715#issuecomment-1779177551
@hussein-awala
hello, I changed to use timeout parameter
Can I get review for change?
--
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
Re: [PR] Use timeout when inntializing _timeout_sec of TaskStateTrigger [airflow]
Posted by "Xiroo (via GitHub)" <gi...@apache.org>.
Xiroo commented on code in PR #34715:
URL: https://github.com/apache/airflow/pull/34715#discussion_r1343843951
##########
airflow/sensors/external_task.py:
##########
@@ -341,6 +341,7 @@ def execute(self, context: Context) -> None:
states=self.allowed_states,
trigger_start_time=utcnow(),
poll_interval=self.poll_interval,
+ timeout=self.timeout,
Review Comment:
I think 60 seconds don't have any meaning because it is the number fixed at 60 seconds not user's choice. Most of the users don't think too much about it and use it because it is just a fixed value. Rather, it makes users wonder why the deferrable mode's timeout is 60 seconds and not be able to find the answer in the docs, so it make users look at the code like I did.
IMHO, 60 seconds is not a proper value for default value since it does not raise any kind of time error(in general case) when using the non-deferrable sensor with default value. It can be confusing for users when the numbers aren't consistent.
I'm even curious why 60 was chosen at first.š¤
--
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
Re: [PR] Use timeout when inntializing _timeout_sec of TaskStateTrigger [airflow]
Posted by "Xiroo (via GitHub)" <gi...@apache.org>.
Xiroo commented on code in PR #34715:
URL: https://github.com/apache/airflow/pull/34715#discussion_r1349613367
##########
airflow/sensors/external_task.py:
##########
@@ -341,6 +341,7 @@ def execute(self, context: Context) -> None:
states=self.allowed_states,
trigger_start_time=utcnow(),
poll_interval=self.poll_interval,
+ timeout=self.timeout,
Review Comment:
@utkarsharma2 Should I change to set default value to 60 sec?
However, I don't understand the meaning of "people 'using' the existing default value of 60 sec kind of breaking change". It's just a default and fixed value so no one could choose to use it.
Also, they even don't know there's default value of 60 if they didn't look at the source code or failure log. (Even if they saw the failure log or source code, they can't change it)
--
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
Re: [PR] Use timeout when inntializing _timeout_sec of TaskStateTrigger [airflow]
Posted by "Xiroo (via GitHub)" <gi...@apache.org>.
Xiroo commented on code in PR #34715:
URL: https://github.com/apache/airflow/pull/34715#discussion_r1343843951
##########
airflow/sensors/external_task.py:
##########
@@ -341,6 +341,7 @@ def execute(self, context: Context) -> None:
states=self.allowed_states,
trigger_start_time=utcnow(),
poll_interval=self.poll_interval,
+ timeout=self.timeout,
Review Comment:
I think 60 seconds don't have any meaning because it is the number fixed at 60 seconds not user's choice. Most of the users don't think too much about it and use it because it is just a fixed value. Rather, it makes users like me wonder why the deferrable mode's timeout is 60 seconds and not be able to find the answer in the docs, so it make users look at the code like I did.
IMHO, 60 seconds is not a proper value for default value since it does not raise any kind of time error(in general case) when using the sensor with default value. It can be confusing for users when the numbers aren't consistent.
--
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