jedcunningham commented on a change in pull request #21501:
URL: https://github.com/apache/airflow/pull/21501#discussion_r804344322
##########
File path: airflow/settings.py
##########
@@ -209,6 +209,18 @@ def get_airflow_context_vars(context):
return {}
+def get_dagbag_import_timeout(dag_file_path: str) -> float:
+ """
+ This setting allows to dynamically control the dag file parsing timeout.
+
+ It is useful when there are a few dag files, requiring long parsing time
while others not
Review comment:
```suggestion
It is useful when there are a few DAG files requiring longer parsing
times, while others do not.
```
##########
File path: tests/models/test_dagbag.py
##########
@@ -206,6 +207,49 @@ def test_zip(self):
assert dagbag.get_dag("test_zip_dag")
assert sys.path == syspath_before # sys.path doesn't change
+ @patch("airflow.models.dagbag.timeout")
+ @patch("airflow.models.dagbag.settings.get_dagbag_import_timeout")
+ def test_process_dag_file_without_timeout(self,
mocked_get_dagbag_import_timeout, mocked_timeout):
+ """
+ Test dag file parsing without timeout
+ """
+ mocked_get_dagbag_import_timeout.return_value = 0
Review comment:
We also check with a negative, no?
##########
File path: tests/models/test_dagbag.py
##########
@@ -206,6 +207,49 @@ def test_zip(self):
assert dagbag.get_dag("test_zip_dag")
assert sys.path == syspath_before # sys.path doesn't change
+ @patch("airflow.models.dagbag.timeout")
+ @patch("airflow.models.dagbag.settings.get_dagbag_import_timeout")
+ def test_process_dag_file_without_timeout(self,
mocked_get_dagbag_import_timeout, mocked_timeout):
+ """
+ Test dag file parsing without timeout
+ """
+ mocked_get_dagbag_import_timeout.return_value = 0
+
+ dagbag = models.DagBag(dag_folder=self.empty_dir,
include_examples=False)
+ dagbag.process_file(os.path.join(TEST_DAGS_FOLDER,
'test_default_views.py'))
+ mocked_timeout.assert_not_called()
+
+ @patch("airflow.models.dagbag.timeout")
+ @patch("airflow.models.dagbag.settings.get_dagbag_import_timeout")
+ def test_process_dag_file_with_non_default_timeout(
+ self, mocked_get_dagbag_import_timeout, mocked_timeout
+ ):
+ """
+ Test customized dag file parsing timeout
+ """
+ timeout_value = 100
+ mocked_get_dagbag_import_timeout.return_value = timeout_value
+
+ # ensure the test value is not equal to the default value
+ assert timeout_value != settings.conf.getfloat('core',
'DAGBAG_IMPORT_TIMEOUT')
+
+ dagbag = models.DagBag(dag_folder=self.empty_dir,
include_examples=False)
+ dagbag.process_file(os.path.join(TEST_DAGS_FOLDER,
'test_default_views.py'))
+
+ mocked_timeout.assert_called_once_with(timeout_value,
error_message=mock.ANY)
+
+ @patch("airflow.models.dagbag.settings.get_dagbag_import_timeout")
+ def test_check_value_type_from_get_dagbag_import_timeout(self,
mocked_get_dagbag_import_timeout):
+ """
+ Test correctness of value from get_dagbag_import_timeout
+ """
+ mocked_get_dagbag_import_timeout.return_value = '1'
Review comment:
```suggestion
mocked_get_dagbag_import_timeout.return_value = 0.1
```
Faster, and the right type?
##########
File path: airflow/settings.py
##########
@@ -209,6 +209,18 @@ def get_airflow_context_vars(context):
return {}
+def get_dagbag_import_timeout(dag_file_path: str) -> float:
+ """
+ This setting allows to dynamically control the dag file parsing timeout.
+
+ It is useful when there are a few dag files, requiring long parsing time
while others not
+ You can control them separately instead of having one value for all dag
files.
Review comment:
```suggestion
You can control them separately instead of having one value for all DAG
files.
```
##########
File path: airflow/settings.py
##########
@@ -209,6 +209,18 @@ def get_airflow_context_vars(context):
return {}
+def get_dagbag_import_timeout(dag_file_path: str) -> float:
+ """
+ This setting allows to dynamically control the dag file parsing timeout.
+
+ It is useful when there are a few dag files, requiring long parsing time
while others not
+ You can control them separately instead of having one value for all dag
files.
+
+ If the return value is less than 0, it means no timeout during the dag
parsing.
Review comment:
```suggestion
If the return value is less than or equal to 0, it means no timeout
while parsing.
```
##########
File path: airflow/settings.py
##########
@@ -209,6 +209,18 @@ def get_airflow_context_vars(context):
return {}
+def get_dagbag_import_timeout(dag_file_path: str) -> float:
Review comment:
```suggestion
def get_dagbag_import_timeout(dag_file_path: str) -> Union[int, float]:
```
##########
File path: airflow/settings.py
##########
@@ -209,6 +209,18 @@ def get_airflow_context_vars(context):
return {}
+def get_dagbag_import_timeout(dag_file_path: str) -> float:
+ """
+ This setting allows to dynamically control the dag file parsing timeout.
Review comment:
```suggestion
This setting allows for dynamically control of the dag file parsing
timeout based on the DAG file path.
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
