[GitHub] [airflow] mhenc commented on a diff in pull request #28502: Migrate DagFileProcessor.manage_slas to Internal API

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

mhenc commented on code in PR #28502:
URL: https://github.com/apache/airflow/pull/28502#discussion_r1065687034


##########
airflow/dag_processing/processor.py:
##########
@@ -365,8 +365,10 @@ def __init__(self, dag_ids: list[str] | None, dag_directory: str, log: logging.L
         self._dag_directory = dag_directory
         self.dag_warnings: set[tuple[str, str]] = set()
 
+    @staticmethod
+    @internal_api_call
     @provide_session
-    def manage_slas(self, dag: DAG, session: Session = None) -> None:
+    def manage_slas(dag_folder, dag_id: str, log: logging.Logger, session: Session = NEW_SESSION) -> None:

Review Comment:
   I don't think we could make it work for non-static methods - passing the whole state of the object would be tricky (we want to avoid pickling) and would affect the performance even more.
   
   If the only problem is passing the logger, then I can see few options:
   1. Instead of passing the logger, just send the logger name and create the logger inside the method
   2. Make logger static (as Vinc suggested in the first place) - we can wrap it into some method to keep `self.log` available for other methods.
   3. I think internal_api_call should also work for class methods, so we could do something like this:
   https://stackoverflow.com/a/8438818
   
   cc: @potiuk 



-- 
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