[GitHub] [airflow] jedcunningham commented on a change in pull request #16659: Refactor: Remove processor_factory from DAG processing

[GitBox <gi...@apache.org>]

jedcunningham commented on a change in pull request #16659:
URL: https://github.com/apache/airflow/pull/16659#discussion_r663214773



##########
File path: airflow/dag_processing/manager.py
##########
@@ -1022,8 +1012,8 @@ def start_new_processes(self):
                 continue
 
             callback_to_execute_for_file = self._callback_to_execute[file_path]
-            processor = self._processor_factory(
-                file_path, callback_to_execute_for_file, self._dag_ids, self._pickle_dags
+            processor = type(self)._create_process(

Review comment:
       ```suggestion
               processor = self._create_process(
   ```

##########
File path: airflow/dag_processing/manager.py
##########
@@ -1013,6 +994,15 @@ def collect_results(self) -> None:
 
         self.log.debug("%s file paths queued for processing", len(self._file_path_queue))
 
+    @staticmethod
+    def _create_process(file_path, pickle_dags, dag_ids, callback_requests):
+        """Creates DagFileProcessorProcess instance."""
+        from airflow.dag_processing.processor import DagFileProcessorProcess

Review comment:
       I assume this was put here to avoid a circular import, but did you profile at all to see if there is a performance impact?

##########
File path: airflow/dag_processing/processor.py
##########
@@ -536,6 +536,7 @@ def execute_callbacks(
         :type callback_requests: List[airflow.utils.callback_requests.CallbackRequest]
         :param session: DB session.
         """
+        self.log.error('ESSADAFD %s', callback_requests)

Review comment:
       ```suggestion
   ```
   
   Leftovers I assume?

##########
File path: tests/dag_processing/test_manager.py
##########
@@ -92,7 +92,7 @@ def result(self):
         return self._result
 
     @staticmethod
-    def _fake_dag_processor_factory(file_path, callbacks, dag_ids, pickle_dags):
+    def _create_process(file_path, callbacks, dag_ids, pickle_dags):

Review comment:
       I don't think this is being used anymore, nor is `FakeDagFileProcessorRunner` itself. Might need to patch `DagFileProcessorManager` 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