ClientRegistrations#rest defines 30s connect and read timeouts

[Kehrlann]

Example PR for #11197 . Note: the RestTemplate uses a plain SimpleHttpRequestFactory which is the default through RestTemplate - extends -> InterceptingHttpAccessor - extends -> HttpAccessor.

[jgrandja]

@Kehrlann Do you still want us to consider this PR given that you closed gh-11197?

[Kehrlann]

Closing re: closing gh-11197 , forgot about this PR. Thanks Joe!

[Kehrlann]

On second thought, re-opened the PR. Those are sensible defaults, more sensible than "0" timeout which defaults to whatever the OS decides the socket timeout is. On most Linux distros, that's 75s, which is way too long for this use-case.

[sjohnr]

Hi @Kehrlann, I think we can look at possibly merging this after we add more flexibility around ClientRegistrations. See gh-11321. However, I wonder if we can discuss the timeout we would choose to default to. While probably not very scientific, I'm thinking of two questions that would drive this choice:

• Is it reasonable?
• Is it unobtrusive?

To me, 10 seconds is very reasonable as most requests should not take that long. However, it seems like it could be obtrusive when chosen as a default, because there may be many users that can or will experience events in production that suffer the effects of this short timeout, which could further compound any ongoing issues in their environment. It could be argued the opposite of course, that it helps by terminating open connections and not allowing them to compound, but this is always subjective and takes load testing and other factors to determine appropriate settings, hence we would ideally provide a way for full customization as discussed elsewhere.

While it's probably impossible to pick the "right" default, I think 30 seconds also seems reasonable while being less obtrusive. What do you think? Are there other factors that are worth considering?

[Kehrlann]

Hi @sjohnr , 30 seconds sounds very un-obtrusive, while still being below the "1 minute healthcheck" threshold I've seen in the wild in some k8s scenarios.

Updated the PR accordingly.

[sjohnr]

Merged into 5.8.x and main as bea7761.