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/10/21 20:13:49 UTC
[GitHub] [airflow] eitanme opened a new pull request, #27190: External task sensor fail fix
eitanme opened a new pull request, #27190:
URL: https://github.com/apache/airflow/pull/27190
At @o-nikolas request, I'm creating a new PR to attempt to fix #16204 where the ExternalTaskSensor would hang indefinitely when an execution_date_fn is used, failed_states/allowed_states are set, and external DAGs have mixed states upstream.
--
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] eitanme commented on pull request #27190: External task sensor fail fix
Posted by GitBox <gi...@apache.org>.
eitanme commented on PR #27190:
URL: https://github.com/apache/airflow/pull/27190#issuecomment-1304705302
@eladkal that seems to have done the trick, thanks for the help!
Anything else you need on my end to get this merged?
--
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 #27190: External task sensor fail fix
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #27190:
URL: https://github.com/apache/airflow/pull/27190#issuecomment-1309993643
Awesome work, congrats on your first merged pull request!
--
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 pull request #27190: External task sensor fail fix
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #27190:
URL: https://github.com/apache/airflow/pull/27190#issuecomment-1293159251
It's fine but I think we need a newsfragment describing this change in the behaviours. This is borderline breaking change/bugfix and while I would lean more on the bugfix side, but people could have relied on it and it should be described in more detail in changelog.
--
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 pull request #27190: External task sensor fail fix
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #27190:
URL: https://github.com/apache/airflow/pull/27190#issuecomment-1296557844
Can you please rebase/solve conflicts?
--
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] eitanme commented on pull request #27190: External task sensor fail fix
Posted by GitBox <gi...@apache.org>.
eitanme commented on PR #27190:
URL: https://github.com/apache/airflow/pull/27190#issuecomment-1293802461
@potiuk because of this bug, to use the `ExternalTaskSensor` currently you must explicitly set a timeout on the sensor or your DAG will hang forever. To your point on reliance on old behavior, to workaround the bug, folks may have set that timeout to avoid an infinite hang.
In those cases, fixing this bug will cause a change in the exception they receive from `AirflowSensorTimeout` to the generic `AirflowException`. If they are relying on catching the `AirflowSensorTimeout` exception subclass they may have issues though if they catch the base class they'd still be OK.
Does that sound about right? What would you propose we do?
I'm happy to update a changelog if I'm pointed in the right direction?
Also, there are some failing checks on this PR that I don't understand. Specifically, in the Sqlite Py3.7: API Always CLI Core Integration Other Providers WWW check a test fails that I'm pretty sure I don't go anywhere near:
```
FAILED tests/jobs/test_local_task_job.py::TestLocalTaskJob::test_heartbeat_failed_fast
```
Any ideas on that front? The logs are long and I didn't see much useful in them while looking through so I wanted to ask before trying to dig deeper as I'm not super familiar with this code-base and the checks on 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
[GitHub] [airflow] eitanme commented on pull request #27190: External task sensor fail fix
Posted by GitBox <gi...@apache.org>.
eitanme commented on PR #27190:
URL: https://github.com/apache/airflow/pull/27190#issuecomment-1297386565
@potiuk just merged `apache:main` into the branch.
--
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] ephraimbuddy commented on pull request #27190: External task sensor fail fix
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on PR #27190:
URL: https://github.com/apache/airflow/pull/27190#issuecomment-1308970226
Are we getting this into 2.4.3? @eladkal @potiuk @uranusjr
--
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] o-nikolas commented on pull request #27190: External task sensor fail fix
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on PR #27190:
URL: https://github.com/apache/airflow/pull/27190#issuecomment-1289463439
> Good point @o-nikolas I was being lazy and didn't want to figure out how to run the test suite. You've pushed me to do the right thing and I think I've added a commit/test that addresses it and fails with the old code while passing with the new. Let me know what you think.
Thanks for adding a test case! A committer will be required to run the workflow for a first time committer (and ultimately merge if everything passes and they are happy with the code). Unfortunately I am not a committer, I'll CC a few who may have time to have a look: @eladkal @potiuk @kaxil
--
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 merged pull request #27190: External task sensor fail fix
Posted by GitBox <gi...@apache.org>.
uranusjr merged PR #27190:
URL: https://github.com/apache/airflow/pull/27190
--
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] o-nikolas commented on pull request #27190: External task sensor fail fix
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on PR #27190:
URL: https://github.com/apache/airflow/pull/27190#issuecomment-1290942370
There was a change merged recently to standardize quoting in Airflow, you'll need to rebase this PR and run static checks locally to patch those up.
You can enable pre-commit to easily run static checks on your code ([readme)](https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#installing-pre-commit-hooks).
--
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] eitanme commented on pull request #27190: External task sensor fail fix
Posted by GitBox <gi...@apache.org>.
eitanme commented on PR #27190:
URL: https://github.com/apache/airflow/pull/27190#issuecomment-1295718907
@eladkal I've added a newsfragment as suggested.
For what it's worth, I think this should break very few if any people because most folks would want their DAGs to fail in the case of an upstream external task failing whether by timeout or because a task in a chain fails. I doubt there are many people who are trying to catch an exception and act on it in this case, but it's at least documented now that you might have to change the exception type in case anyone falls into that bucket.
--
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] eitanme commented on pull request #27190: External task sensor fail fix
Posted by GitBox <gi...@apache.org>.
eitanme commented on PR #27190:
URL: https://github.com/apache/airflow/pull/27190#issuecomment-1297402843
@potiuk sorry about that, I had been merging in `main` instead of rebasing on it, but I've changed that.
Let me know if you need anything else on my end.
--
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] eitanme commented on pull request #27190: External task sensor fail fix
Posted by GitBox <gi...@apache.org>.
eitanme commented on PR #27190:
URL: https://github.com/apache/airflow/pull/27190#issuecomment-1304137163
@eladkal I noticed that the following test failed for the "Tests / Sqlite Py3.7: API Always CLI Core Integration Other Providers WWW (pull_request)" pre-submit job but I'm pretty sure this test has nothing to do with my code. Does that seem right? Is there anything I should be doing? Or do these tests fail intermittently or something?
Thanks for the help!
```
=================================== FAILURES ===================================
_________________ TestLocalTaskJob.test_heartbeat_failed_fast __________________
self = <tests.jobs.test_local_task_job.TestLocalTaskJob object at 0x7f1378e24390>
def test_heartbeat_failed_fast(self):
"""
Test that task heartbeat will sleep when it fails fast
"""
self.mock_base_job_sleep.side_effect = time.sleep
dag_id = "test_heartbeat_failed_fast"
task_id = "test_heartbeat_failed_fast_op"
with create_session() as session:
dag_id = "test_heartbeat_failed_fast"
task_id = "test_heartbeat_failed_fast_op"
dag = self.dagbag.get_dag(dag_id)
task = dag.get_task(task_id)
dr = dag.create_dagrun(
run_id="test_heartbeat_failed_fast_run",
state=State.RUNNING,
execution_date=DEFAULT_DATE,
start_date=DEFAULT_DATE,
session=session,
)
ti = dr.task_instances[0]
ti.refresh_from_task(task)
ti.state = State.QUEUED
ti.hostname = get_hostname()
ti.pid = 1
session.commit()
job = LocalTaskJob(task_instance=ti, executor=MockExecutor(do_update=False))
job.heartrate = 2
heartbeat_records = []
job.heartbeat_callback = lambda session: heartbeat_records.append(job.latest_heartbeat)
job._execute()
assert len(heartbeat_records) > 2
for i in range(1, len(heartbeat_records)):
time1 = heartbeat_records[i - 1]
time2 = heartbeat_records[i]
# Assert that difference small enough
delta = (time2 - time1).total_seconds()
> assert abs(delta - job.heartrate) < 0.8
E assert 1.0317020000000001 < 0.8
E + where 1.0317020000000001 = abs((3.031702 - 2))
E + where 2 = <airflow.jobs.local_task_job.LocalTaskJob object at 0x7f1378e35cd0>.heartrate
tests/jobs/test_local_task_job.py:312: AssertionError
```
--
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] eitanme commented on pull request #27190: External task sensor fail fix
Posted by GitBox <gi...@apache.org>.
eitanme commented on PR #27190:
URL: https://github.com/apache/airflow/pull/27190#issuecomment-1287515841
Good point @o-nikolas I was being lazy and didn't want to figure out how to run the test suite. You've pushed me to do the right thing and I think I've added a commit/test that addresses it and fails with the old cold while passing with the new. Let me know what you think.
--
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 pull request #27190: External task sensor fail fix
Posted by GitBox <gi...@apache.org>.
eladkal commented on PR #27190:
URL: https://github.com/apache/airflow/pull/27190#issuecomment-1294806097
> I'm happy to update a changelog if I'm pointed in the right direction?
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
[GitHub] [airflow] eitanme commented on pull request #27190: External task sensor fail fix
Posted by GitBox <gi...@apache.org>.
eitanme commented on PR #27190:
URL: https://github.com/apache/airflow/pull/27190#issuecomment-1292237510
@o-nikolas I merged in main enabled pre-commit and updated my quoting style so hopefully that's the last of the linting though we'll see for sure after PR checks run. Thanks for the help and heads up that it had changed.
--
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 pull request #27190: External task sensor fail fix
Posted by GitBox <gi...@apache.org>.
eladkal commented on PR #27190:
URL: https://github.com/apache/airflow/pull/27190#issuecomment-1304611479
I reran the test. lets see what happens
--
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