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

CORE-308 expand concurrent requests to sam #4830

Merged
merged 3 commits into from
Feb 27, 2025
Merged

CORE-308 expand concurrent requests to sam #4830

merged 3 commits into from
Feb 27, 2025

Conversation

dvoet
Copy link
Contributor

@dvoet dvoet commented Feb 27, 2025

Jira ticket: https://broadworkbench.atlassian.net/browse/CORE-308

Summary of changes

This PR increases the number of concurrent requests a Leo instance can make to Sam from 5 (the default) to 15. This is an attempt to address socket timeout errors that appear in the logs (but are retries, presumably successfully).

What

Why

Testing these changes

What to test

Who tested and where

  • This change is covered by automated tests
    • NB: Rerun automation tests on this PR by commenting jenkins retest or jenkins multi-test.
  • I validated this change
  • Primary reviewer validated this change
  • I validated this change in the dev environment

@dvoet dvoet requested a review from a team as a code owner February 27, 2025 15:02
.dispatcher(dispatcher)
.build()
}

private def getApiClient(token: String)(implicit ev: Ask[F, AppContext]): F[ApiClient] =
for {
ctx <- ev.ask
okHttpClientBuilder = okHttpClient.newBuilder
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left the builder here because the following todo is still relevant

Comment on lines +36 to +37
dispatcher.setMaxRequests(maxConcurrentRequests)
dispatcher.setMaxRequestsPerHost(maxConcurrentRequests)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this client is only handling a single host so these are both set to the same value

Copy link

codecov bot commented Feb 27, 2025

Codecov Report

Attention: Patch coverage is 28.57143% with 5 lines in your changes missing coverage. Please review.

Project coverage is 74.58%. Comparing base (f0dd5f9) to head (39aa404).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
...kbench/leonardo/dao/sam/SamApiClientProvider.scala 0.00% 4 Missing ⚠️
...ch/leonardo/http/BaselineDependenciesBuilder.scala 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4830      +/-   ##
===========================================
- Coverage    74.59%   74.58%   -0.01%     
===========================================
  Files          166      166              
  Lines        14609    14612       +3     
  Branches      1101     1172      +71     
===========================================
+ Hits         10898    10899       +1     
- Misses        3711     3713       +2     
Files with missing lines Coverage Δ
...titute/dsde/workbench/leonardo/config/Config.scala 97.77% <100.00%> (+<0.01%) ⬆️
...itute/dsde/workbench/leonardo/dao/HttpSamDAO.scala 25.40% <ø> (ø)
...ch/leonardo/http/BaselineDependenciesBuilder.scala 0.00% <0.00%> (ø)
...kbench/leonardo/dao/sam/SamApiClientProvider.scala 0.00% <0.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0dd5f9...39aa404. Read the comment docs.

@dvoet dvoet enabled auto-merge (squash) February 27, 2025 18:01
Copy link
Collaborator

@LizBaldo LizBaldo left a comment

Choose a reason for hiding this comment

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

I did not see this as a potential cause in the ticket, but looks like a good thing worth trying 👍

@dvoet dvoet merged commit c1d60cd into develop Feb 27, 2025
22 of 23 checks passed
@dvoet dvoet deleted the use_okhttp_rite branch February 27, 2025 18:09
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.

3 participants