Skip to content
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

feat(artifacts): update to okhttp3 and add connection timeout config for github artifacts. #5093

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

gsapkal
Copy link

@gsapkal gsapkal commented Nov 10, 2020

  • The artifact provider is updated to use okHttp3 provided by kork-web.
  • For artifact configuration added attributes connectTimeoutMs, readTimeoutMs and retryOnConnectionFailure. These will be used to configure okhttp3 used by http based artifact providers.
  • For github artifact provider connectTimeoutMs, readTimeoutMs and retryOnConnectionFailure attributes are added. If defined the okhttp3 client used by github provider will be configured using these.
  • For gitrepo artifact provider timeout attribute is added account configuration which will be used to configure jgit timeout.

Ganesh Sapkal added 3 commits November 6, 2020 20:37
… configuration .

##Clouddriver artifacts downloader settings.
artifacts:
  connectTimeoutMs: 1200000
  readTimeoutMs: 1200000
  retryOnConnectionFailure: true
@gsapkal gsapkal changed the title feat(artifacts): add connection timeout config for artifacts. WIP:feat(artifacts): add connection timeout config for artifacts. Nov 10, 2020
@gsapkal gsapkal marked this pull request as draft November 10, 2020 23:10
@gsapkal gsapkal changed the title WIP:feat(artifacts): add connection timeout config for artifacts. feat(artifacts): add connection timeout config for artifacts. Nov 10, 2020
@zachsmith1
Copy link
Contributor

zachsmith1 commented Nov 11, 2020

Let's chat before this gets merged. We should actually be using https://github.com/spinnaker/kork/blob/master/kork-web/src/main/groovy/com/netflix/spinnaker/config/OkHttp3ClientConfiguration.groovy

We have recently been talking about switching to the single okhttp instance model because it's recommended by the library.

Are you in the OSS slack?

@gsapkal
Copy link
Author

gsapkal commented Nov 11, 2020

@zachsmith1 I explored the option of using the config from kork but it seems to be specific to spinnaker micro-services communication specific. With external systems for artifacts download the values might be different based on endpoints and network we are hitting. we can discuss this more on OSS slack.

Ganesh Sapkal added 3 commits November 12, 2020 16:57
@gsapkal gsapkal marked this pull request as ready for review November 13, 2020 23:34
@gsapkal gsapkal changed the title feat(artifacts): add connection timeout config for artifacts. feat(artifacts): add connection timeout config for github artifacts. Nov 13, 2020
@gsapkal gsapkal changed the title feat(artifacts): add connection timeout config for github artifacts. feat(artifacts): update to okhttp3 and add connection timeout config for github artifacts. Nov 13, 2020
@Bean
OkHttpClient okHttpClient() {
return new OkHttpClient();
@JsonIgnore private final OkHttpClient okHttpClient;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we trust that the correct bean is getting wired up here or should we be explicitly calling this and then making subsequence changes to the builder as necessary?
@dreynaud

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zachsmith1 @dreynaud I have tested updated this code to keep the okHttpClient bean. Also updated PR to support the timeout as part of gitrepo account configuration.
Please review and let me know if there are any changes needed.

@dreynaud
Copy link
Contributor

dreynaud commented Dec 2, 2020

CC'ing @srekapalli and @cfieber who probably have more recent experience with this

@Bean
OkHttpClient okHttpClient() {
return new OkHttpClient();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Base client is already configured here kork-web and we should be able to inject this and customize further for timeout settings etc using the newBuilder api.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants