Skip to content
This repository was archived by the owner on Dec 20, 2025. It is now read-only.

fix(okhttpconfig): increase the defaul timeout due to low number#4849

Closed
shlomodaari wants to merge 1 commit intospinnaker:masterfrom
shlomodaari:patch-1
Closed

fix(okhttpconfig): increase the defaul timeout due to low number#4849
shlomodaari wants to merge 1 commit intospinnaker:masterfrom
shlomodaari:patch-1

Conversation

@shlomodaari
Copy link
Copy Markdown

No description provided.

This is a low number and can have an affect on complex production
@dbyron-sf
Copy link
Copy Markdown
Contributor

There's enough stuff going on with retrofit2 upgrades that I'd love to leave this code alone. Isn't changing your local config sufficient?

@jasonmcintosh
Copy link
Copy Markdown
Member

Sooo what we hit recently:

  • OKHTTP timeouts are normally set via kork's common okhttp configurations.
  • The bulk upload stopped using the service locator lookups which USES those okhttp for it's OWN configuration by service
  • This then caused failures where the okhttp global timeouts which were high... suddenly no longer applied and these front50 operations timed out with lower thresholds.

SO the bulk PR ... broke some things rather badly b/c of this. I'm not sure changing these defaults high is the answer... vs. adding a bit more logic. A few ideas:

  • "OK if these are SET, use the set values, otherwise default to okhttp".
  • I could also see "use whichever is higher... MAYBE.
  • Alternative would be use Integer, no defaults set, and check if the okhttps are set, and if not, use the 10 seconds instead via a spel expression to allow an override for a specific service.

@dbyron-sf
Copy link
Copy Markdown
Contributor

Which bean is bulk upload? Can you provide a link to the code that creates it? From there perhaps @kirangodishala can take a look and see if there's some way to restore the ability to configure it independently?

@shlomodaari
Copy link
Copy Markdown
Author

I will close this one the go ahead with Jason idea!
"OK if these are SET, use the set values, otherwise default to okhttp".
If you both okay with it

@jasonmcintosh
Copy link
Copy Markdown
Member

#4773 was what broke things this way as it overloaded the okhttp settings for front50 calls WITHOUT considering okhttp global defaults.

@jasonmcintosh
Copy link
Copy Markdown
Member

jasonmcintosh commented Apr 2, 2025

NOTE right now kinda thinking of a "int to Integer" then doing
@Value("${front50.okhttp.readTimeoutMs != null ? front50.okhttp.readTimeoutMs : (ok-http-client.readTimeoutMs != null ? ok-http-client.readTimeoutMs : 10000)}") or similar kinda thing

@shlomodaari
Copy link
Copy Markdown
Author

#4851
I created this new one based on Jason input

@dbyron-sf dbyron-sf closed this Oct 2, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants