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/04/05 16:20:22 UTC
[GitHub] [airflow] andrewgodwin opened a new pull request #15207: Fixed #14270: Add error message in OOM situations
andrewgodwin opened a new pull request #15207:
URL: https://github.com/apache/airflow/pull/15207
In the case where a child process is reaped early (before we get to it) the presumption in the code is that it is due to an OOM error and we set the return code -9. This adds an error message alongside that return code to make it more obvious.
It also adds a test that checks this overall functionality, and in the process of doing that switches the StandardTaskRunner tests to use `pytest`-style tests rather than a TestCase, as otherwise we can't use the `caplog` fixture. It also removes the apparently unnecessary logging propagation removal in this file.
closes: #14270
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] andrewgodwin commented on a change in pull request #15207: Fixed #14270: Add error message in OOM situations
Posted by GitBox <gi...@apache.org>.
andrewgodwin commented on a change in pull request #15207:
URL: https://github.com/apache/airflow/pull/15207#discussion_r607220353
##########
File path: tests/task/task_runner/test_standard_task_runner.py
##########
@@ -48,22 +48,24 @@
'class': 'logging.StreamHandler',
'formatter': 'airflow.task',
'stream': 'ext://sys.stdout',
- }
+ },
},
- 'loggers': {'airflow': {'handlers': ['console'], 'level': 'INFO', 'propagate': False}},
+ 'loggers': {'airflow': {'handlers': ['console'], 'level': 'INFO', 'propagate': True}},
}
-class TestStandardTaskRunner(unittest.TestCase):
- @classmethod
- def setUpClass(cls):
+class TestStandardTaskRunner:
+ @pytest.fixture(autouse=True, scope="class")
+ def logging_and_db(self):
+ """
+ This fixture sets up logging to have a different setup on the way in
+ (as the test environment does not have enough context for the normal
+ way to run) and ensures they reset back to normal on the way out.
+ """
dictConfig(LOGGING_CONFIG)
-
- @classmethod
- def tearDownClass(cls):
+ yield
Review comment:
When you write pytest fixtures, `yield` signifies where the test should run. In this case, it is sitting in between the old setUp and tearDown portions to signify the test should run there.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman merged pull request #15207: Fixed #14270: Add error message in OOM situations
Posted by GitBox <gi...@apache.org>.
dimberman merged pull request #15207:
URL: https://github.com/apache/airflow/pull/15207
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman commented on a change in pull request #15207: Fixed #14270: Add error message in OOM situations
Posted by GitBox <gi...@apache.org>.
dimberman commented on a change in pull request #15207:
URL: https://github.com/apache/airflow/pull/15207#discussion_r607230420
##########
File path: tests/task/task_runner/test_standard_task_runner.py
##########
@@ -48,22 +48,24 @@
'class': 'logging.StreamHandler',
'formatter': 'airflow.task',
'stream': 'ext://sys.stdout',
- }
+ },
},
- 'loggers': {'airflow': {'handlers': ['console'], 'level': 'INFO', 'propagate': False}},
+ 'loggers': {'airflow': {'handlers': ['console'], 'level': 'INFO', 'propagate': True}},
}
-class TestStandardTaskRunner(unittest.TestCase):
- @classmethod
- def setUpClass(cls):
+class TestStandardTaskRunner:
+ @pytest.fixture(autouse=True, scope="class")
+ def logging_and_db(self):
+ """
+ This fixture sets up logging to have a different setup on the way in
+ (as the test environment does not have enough context for the normal
+ way to run) and ensures they reset back to normal on the way out.
+ """
dictConfig(LOGGING_CONFIG)
-
- @classmethod
- def tearDownClass(cls):
+ yield
Review comment:
Oh! Nifty!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #15207: Fixed #14270: Add error message in OOM situations
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15207:
URL: https://github.com/apache/airflow/pull/15207#issuecomment-813611256
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 master 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman commented on a change in pull request #15207: Fixed #14270: Add error message in OOM situations
Posted by GitBox <gi...@apache.org>.
dimberman commented on a change in pull request #15207:
URL: https://github.com/apache/airflow/pull/15207#discussion_r607208485
##########
File path: tests/task/task_runner/test_standard_task_runner.py
##########
@@ -48,22 +48,24 @@
'class': 'logging.StreamHandler',
'formatter': 'airflow.task',
'stream': 'ext://sys.stdout',
- }
+ },
},
- 'loggers': {'airflow': {'handlers': ['console'], 'level': 'INFO', 'propagate': False}},
+ 'loggers': {'airflow': {'handlers': ['console'], 'level': 'INFO', 'propagate': True}},
}
-class TestStandardTaskRunner(unittest.TestCase):
- @classmethod
- def setUpClass(cls):
+class TestStandardTaskRunner:
+ @pytest.fixture(autouse=True, scope="class")
+ def logging_and_db(self):
+ """
+ This fixture sets up logging to have a different setup on the way in
+ (as the test environment does not have enough context for the normal
+ way to run) and ensures they reset back to normal on the way out.
+ """
dictConfig(LOGGING_CONFIG)
-
- @classmethod
- def tearDownClass(cls):
+ yield
Review comment:
@andrewgodwin what does this yield accomplish?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org