Add idle_timeout to routes API

[denismakogon]

Adding inactivity_timeout to routes API.
This change will allow users to define idle timeout for hot functions via API instead of having it hardcoded in worker.go.

Closes: #554

[jmank88]

LGTM

[treeder]

Generally looks good, but can we call it IdleTimeout instead of InactivityTimeout ?

[denismakogon]

@treeder Sure, why not.

[ucirello]

I am 👎 this because I really think has not been thought through.

Now, instead of one timeouts, there are two. And one private constant, that most likely had a very good reason to be private, is now converted into a part of a public interface.

Also, lacking documentation about the use of this attribute and its impact in the behavior of the machinery - before, it has a predictable behavior, now, no one will actually know how it will behave.

[denismakogon]

@ccirello So, yes, there are two timeouts, and it doesn't look like something bad or confusing. One timeout is for function call, and the idle timeout defines for how long to keep hot function around without serving requests for it. You're saying that it was not thought through, i tend to disagree on this, because it's not clear why hardcoded 30s timeout is exact timeout that every hot function should wait without processing requests before becoming "cold".
From user perspective, if i have a workload that comes each 1m, and i don't want to spend time to wait until function will be started (+ overhead of function itself, like pulling out data from 3rd party services, etc.) rather than setting idle timeout for 2m, so with such workload my hot function will be more effective in terms of faster requests serving.
Does it make sense?
cc @treeder @seiflotfy @pedronasser

[ucirello]

@denismakogon - makes sense to a point.

So, yes, there are two timeouts, and it doesn't look like something bad or confusing.

Without context, I doubt people will know the difference between timeout and idle_timeout. This could be easily addressed with documentation though - as of now, however, docs are missing.

You're saying that it was not thought through, i tend to disagree on this, because it's not clear why hardcoded 30s timeout is exact timeout that every hot function should wait without processing requests before becoming "cold".

Indeed, it is not clear the reason behind 30s. However, whenever we change a constant to a variable, we are changing the behavior of the system. So what you are saying is that the user needs it - which I am fine with - and that this reason alone is enough to justify this change as it is right now.

I did not see anywhere any consideration of the consequences of this change: it does not define min and max boundaries, does not explain the consequence of too short or too long idle timeouts, it seems to address the zero case implicitly (for instance, it is not self-evident for me what happens if this idle_timeout is zero); also I do not see in the source code the interaction between this field being updated in the DB while the hot functions are still running.

Again, I am not against the goal of this change - but this implementation of this change is lacking in many aspects.

A middle-way here would be to merge this as it is, and open a batch of tickets addresses its deficiencies.

[denismakogon]

@ccirello I agree on documentation as well as with edge cases. I'll make next PR with docs that explicitly describes all possible cases for idle time and its combinations with regular timeout.

[treeder]

The docs should be part of this PR, agree with @ccirello . Not complete without docs.

[denismakogon]

@treeder doc is already here. Check last 2 commits.

[treeder]

@ccirello ok now with docs?

[ucirello]

@denismakogon @treeder LGTM