-
Notifications
You must be signed in to change notification settings - Fork 140
adding breaking change information for orca #504
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@dbyron-sf, do you mind reviewing this again? |
readTimeoutMs: 10000 # Hardcoded default: 10 seconds | ||
writeTimeoutMs: 10000 # Hardcoded default: 10 seconds | ||
``` | ||
Spinnaker installations that set the old config properties (`ok-http-client`) see a change in behavior in version 1.36.0. The old properties are ignored, and the new hardcoded defaults (10 seconds) are used instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even users that didn't set any ok-http-client properties see a change in behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, since the new default is shorter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah need to tweak the text to allow for this. Probably rephrased to
- in 1.36, there's a new set of defaults that replace the default timeouts on internal calls to front50 with drastically lower defaults. Further these defaults now ignore any overridden okhttp settings for inter-service communications. This can break certain operations that might have previously worked due to the lower timeout defaults. The most common case is when service accounts are created as part of saving a pipeline. As such, you should adjust your front50 timeout calls explicitly for your use case using these different properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasonmcintosh @dbyron-sf please review! If it is still problematic, feel free to edit this. I think you should have the option, this will reduce the time :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 9f451c3. I'd like to reference spinnaker/orca#4851 and be able to say when we've at least restored the timeout behavior, but let's not do that until it's actually merged. It still needs some work from my perspective.
- put it at the beginning so it stands out more - add info about maxRequests/maxRequestsPerHost and the connection pool properties - simplify the advice
No description provided.