Avoid timing out when no timeout is set #4653
Conversation
rauchy
changed the title
rauchy
changed the title
redash/tasks/worker.py
Outdated
| @@ -50,8 +50,11 @@ def stop_executing_job(self, job): | |||
| self.log.warning("Job %s has been cancelled.", job.id) | |||
|
|
|||
| def soft_limit_exceeded(self, job): | |||
| seconds_under_monitor = (utcnow() - self.monitor_started).seconds | |||
| return seconds_under_monitor > job.timeout + self.grace_period | |||
| if job.timeout < 0: | |||
There was a problem hiding this comment.
Shouldn't it be <= ? Or zero timeout has some special meaning?
There was a problem hiding this comment.
Maybe we should use explicit -1 here? So it's clear what is it about? I would also consider defining a constant for it, so it's clear where this magic number coming from.
There was a problem hiding this comment.
@kravets-levko only -1 means "run infinitely". While I agree that a 0 doesn't make sense, someone might want to choose to set their Redash instance to time out immediately (🤷🏻♂️), so soft_limit_exceeded should include those. I agree with @arikfr's approach regarding explicitness.
There was a problem hiding this comment.
I would also consider defining a constant for it, so it's clear where this magic number coming from.
Decided to avoid constant since it wouldn't be re-used in settings. Went for a descriptive variable name instead.
What type of PR is this? (check all applicable)
Description
Following #4626, query execution jobs were set to never time out by default, but our
Workerimplementation would enforce a hard limit if soft limits + grace period (15s) are not handled. In this case, if a time limit is not set, jobs would time out after-1 + 15 = 14seconds.