You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/07/23 09:28:43 UTC
[GitHub] [airflow] ephraimbuddy opened a new pull request #17187: Do not seek error file when it is closed
ephraimbuddy opened a new pull request #17187:
URL: https://github.com/apache/airflow/pull/17187
We do not check if the error file is closed before we seek it, which causes exceptions.
Sometimes, this error file does not exist e.g when the task state is changed externally.
This change fixes it by returning None when the file is closed so that custom text can be used for error.
```
...
error = self.task_runner.deserialize_run_error()
File "/home/ephraimbuddy/Documents/astronomer/airflow/airflow/task/task_runner/base_task_runner.py", line 105, in deserialize_run_error
return load_error_file(self._error_file)
File "/home/ephraimbuddy/Documents/astronomer/airflow/airflow/models/taskinstance.py", line 115, in load_error_file
fd.seek(0, os.SEEK_SET)
File "/usr/lib/python3.8/tempfile.py", line 612, in func_wrapper
return func(*args, **kwargs)
ValueError: seek of closed file
....
File "/usr/lib/python3.8/tempfile.py", line 579, in __del__
self.close()
File "/usr/lib/python3.8/tempfile.py", line 575, in close
unlink(self.name)
FileNotFoundError: [Errno 2] No such file or directory: '/tmp/tmpmr_i6r_2'
```
---
**^ 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 change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+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 [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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 merged pull request #17187: Do not seek error file when it is closed
Posted by GitBox <gi...@apache.org>.
ephraimbuddy merged pull request #17187:
URL: https://github.com/apache/airflow/pull/17187
--
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 a change in pull request #17187: Do not seek error file when it is closed
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #17187:
URL: https://github.com/apache/airflow/pull/17187#discussion_r675488718
##########
File path: tests/models/test_taskinstance.py
##########
@@ -119,6 +125,18 @@ def setUp(self):
def tearDown(self):
self.clean_db()
+ def test_load_error_file_returns_None_for_closed_file(self):
+ error_fd = create_tempfile()
Review comment:
In my opinion, using a temp file will help us use the method `set_error_file` line136, thereby mimicking the actual run. What do 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] jedcunningham commented on a change in pull request #17187: Do not seek error file when it is closed
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #17187:
URL: https://github.com/apache/airflow/pull/17187#discussion_r675887518
##########
File path: tests/models/test_taskinstance.py
##########
@@ -119,6 +121,17 @@ def setUp(self):
def tearDown(self):
self.clean_db()
+ def test_load_error_file_returns_None_for_closed_file(self):
+ error_fd = NamedTemporaryFile()
+ error_fd.close()
+ assert not load_error_file(error_fd)
Review comment:
```suggestion
assert load_error_file(error_fd) is None
```
Might be better to explicitly assert `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] github-actions[bot] commented on pull request #17187: Do not seek error file when it is closed
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #17187:
URL: https://github.com/apache/airflow/pull/17187#issuecomment-885936972
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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 #17187: Do not seek error file when it is closed
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17187:
URL: https://github.com/apache/airflow/pull/17187#discussion_r675437369
##########
File path: tests/models/test_taskinstance.py
##########
@@ -119,6 +125,18 @@ def setUp(self):
def tearDown(self):
self.clean_db()
+ def test_load_error_file_returns_None_for_closed_file(self):
+ error_fd = create_tempfile()
Review comment:
It’s probably easier (and slightly more efficient) to do `io.BytesIO()` instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #17187: Do not seek error file when it is closed
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17187:
URL: https://github.com/apache/airflow/pull/17187#discussion_r675437369
##########
File path: tests/models/test_taskinstance.py
##########
@@ -119,6 +125,18 @@ def setUp(self):
def tearDown(self):
self.clean_db()
+ def test_load_error_file_returns_None_for_closed_file(self):
+ error_fd = create_tempfile()
Review comment:
It’s probably easier (and slightly more efficient) to do `io.TextIO()` instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a change in pull request #17187: Do not seek error file when it is closed
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #17187:
URL: https://github.com/apache/airflow/pull/17187#discussion_r675662038
##########
File path: tests/models/test_taskinstance.py
##########
@@ -119,6 +125,18 @@ def setUp(self):
def tearDown(self):
self.clean_db()
+ def test_load_error_file_returns_None_for_closed_file(self):
+ error_fd = create_tempfile()
Review comment:
Yeah, not sure `open()` will work on `BytesIO()`?
##########
File path: tests/models/test_taskinstance.py
##########
@@ -119,6 +125,18 @@ def setUp(self):
def tearDown(self):
self.clean_db()
+ def test_load_error_file_returns_None_for_closed_file(self):
+ error_fd = create_tempfile()
+ error_fd.close()
+ assert not load_error_file(error_fd)
+
+ def test_load_error_file_loads_correctly(self):
+ error_message = "some random error message"
+ error_fd = create_tempfile()
+ set_error_file(error_fd.name, error=error_message)
+ assert load_error_file(error_fd) == error_message
+ error_fd.close()
Review comment:
```suggestion
with NamedTemporaryFile() as error_fd:
set_error_file(error_fd.name, error=error_message)
assert load_error_file(error_fd) == error_message
```
Probably better?
##########
File path: tests/models/test_taskinstance.py
##########
@@ -72,6 +74,10 @@
from tests.test_utils.db import clear_db_connections
+def create_tempfile():
+ return NamedTemporaryFile(delete=True)
Review comment:
`delete=True` is the default. Not sure this warrants a separate function 🤷♂️?
--
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