[GitHub] [airflow] dstandish commented on a change in pull request #14085: Ensure executors end method is called

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

dstandish commented on a change in pull request #14085:
URL: https://github.com/apache/airflow/pull/14085#discussion_r574227085



##########
File path: airflow/jobs/scheduler_job.py
##########
@@ -1292,13 +1290,12 @@ def _execute(self) -> None:
                 )
                 models.DAG.deactivate_stale_dags(execute_start_time)
 
-            self.executor.end()
-
             settings.Session.remove()  # type: ignore
         except Exception:  # pylint: disable=broad-except
             self.log.exception("Exception when executing SchedulerJob._run_scheduler_loop")
         finally:
             self.processor_agent.end()

Review comment:
       what if `self.processor_agent.end()` fails? will `self.executor.end()` be reached?  maybe you need something like an exit stack

##########
File path: airflow/executors/local_executor.py
##########
@@ -383,6 +383,10 @@ def end(self) -> None:
             raise AirflowException(NOT_STARTED_MESSAGE)
         if not self.manager:
             raise AirflowException(NOT_STARTED_MESSAGE)
+        self.log.info(
+            "Shutting down LocalExecutor"
+            " - this will wait for running tasks to finish, or signal again if you don't want to wait."
+        )

Review comment:
       ```suggestion
           self.log.info(
               "Attempting to shut down LocalExecutor"
               "; waiting for running tasks to finish.  Signal again if you don't want to wait."
           )
   ```
   
   main point here is to clarify grammatically that it's not `_this_ will wait or signal` but `_this_ will wait and _you_ must signal`




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