potiuk commented on PR #34457: URL: https://github.com/apache/airflow/pull/34457#issuecomment-1986905542
this looks good @Bowrna (except the static check failing) - but I have one small proposal. It took me a bit of time to see why grace_multiplier is not passed to to the method in heartbeat - and I found out that even if it is specified in `is_alive` method, it's never set to anything diffferent than 2.1. And I think it's placed wrrongly. it should not be parameter of is_alive, but it should be - similarly as `heartrate` for example) added as Job's field in Job's constructor. ``` self.grace_multiplier = 2.1 ``` And then - it should be taken from job whenever needed (similarly as we take job.job_type). It would be great to change it here - because we are anyhow touching the same code, but If you think it's too much, I am fine to approve it even without it (and you could do it as a follow-up for example). -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
