potiuk commented on a change in pull request #6760: [AIRFLOW-6157] Separate out common protocol for executors. URL: https://github.com/apache/airflow/pull/6760#discussion_r355408225
########## File path: airflow/executors/base_executor.py ########## @@ -42,7 +43,124 @@ QueuedTaskInstanceType = Tuple[CommandType, int, Optional[str], SimpleTaskInstance] -class BaseExecutor(LoggingMixin): +class BaseExecutorProtocol(LoggingMixin): + """ + Base Protocol implemented by all executors including multiple executors. + """ + + def __init__(self): + super().__init__() + + def start(self): # pragma: no cover + """ + Executors may need to get things started. + """ + raise NotImplementedError() + + def has_task(self, task_instance: TaskInstance) -> bool: + """ + Checks if a task is either queued or running in this executor. + + :param task_instance: TaskInstance + :return: True if the task is known to this executor + """ + raise NotImplementedError() + + def sync(self) -> None: + """ + Sync will get called periodically by the heartbeat method. + Executors should override this to perform gather statuses. + """ + raise NotImplementedError() Review comment: Yes. you have to implement everything. Sync is not only called from the heartbeat. It is also called in the "end" method. And we might implement it differently - for example skipping heartbeat and doing nothing there - especially for simple protocols like InProcessExecutor. There are two different things those two methods do. heartbeat just checks if the worker is alive. Sync makes sure that all of the locally accumulated data is sent to the remote workers (or actually to queue). It might be that in some executors sync will be doing nothing as the data will already be synced. I think we might want to redesign it later. For now I just wanted to capture a clean protocol that all Executors are implementing without revealing the internal implementation of BaseExecutor (which is not a protocol - it's a Mixin in fact implementing some common behaviours that might be useful for a number of executors - but then all other entities should simply see and use the protocol to communicate with executor. ---------------------------------------------------------------- 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: [email protected] With regards, Apache Git Services
