[GitHub] [airflow] o-nikolas commented on a diff in pull request #33084: Don't just default to scheduler heartbeat in jobs

["o-nikolas (via GitHub)" <gi...@apache.org>]

o-nikolas commented on code in PR #33084:
URL: https://github.com/apache/airflow/pull/33084#discussion_r1283778503


##########
airflow/jobs/job.py:
##########
@@ -117,6 +115,15 @@ def __init__(self, executor=None, heartrate=None, **kwargs):
     def executor(self):
         return ExecutorLoader.get_default_executor()
 
+    @cached_property
+    def heartrate(self):
+        if self.job_type == "TriggererJob":
+            return conf.getfloat("triggerer", "JOB_HEARTBEAT_SEC")
+        else:
+            # Heartrate used to be hardcoded to scheduler, so in all other
+            # cases continue to use that value for back compat
+            return conf.getfloat("scheduler", "JOB_HEARTBEAT_SEC")
+

Review Comment:
   > I wonder if we can avoid implementing this logic in the parent class (job class), where doing so, we will have to add a case for each future job runner.
   
   Yeah and the interaction between jobs and their runners are very blurry (a job doesn't set it's own type even, it's the runner that does it!).
   
   My aim with this PR was just to keep it scoped to the bug fix and not rearchitect too much.
   
   > Why do we need to override the default value too?
   
   I'm not sure precisely which default value you're talking about. But the heartrate needs to be set in the Job class because the heartrate is not stored in the DB for Jobs, so it needs to be fetched from cconfig when sqlalchemy builds the Python object from a query result.



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