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

fix: scale number of threads to scale with number of cpus #878

Merged

Conversation

igorbernstein2
Copy link
Collaborator

@igorbernstein2 igorbernstein2 commented Feb 20, 2020

This is temporary solution until something better can be worked out.
Ideally we would separate the executor for gax and the transport provider. But there are some backwards compatibility concerns to workout first.

This is temporary solution until something better can be worked out.
Ideally we would separate the executor for gax and the transport provider. But there are some backwards compatibility concerns to workout first.
@googlebot googlebot added the cla: yes label Feb 20, 2020
@codecov
Copy link

@codecov codecov bot commented Feb 20, 2020

Codecov Report

Merging #878 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #878      +/-   ##
============================================
+ Coverage      78.6%   78.63%   +0.02%     
- Complexity     1162     1163       +1     
============================================
  Files           203      203              
  Lines          5141     5143       +2     
  Branches        413      413              
============================================
+ Hits           4041     4044       +3     
  Misses          925      925              
+ Partials        175      174       -1
Impacted Files Coverage Δ Complexity Δ
...le/api/gax/core/InstantiatingExecutorProvider.java 62.5% <100%> (+5.35%) 3 <1> (ø) ⬇️
.../java/com/google/api/gax/batching/BatcherImpl.java 98% <0%> (+0.66%) 16% <0%> (+1%) ⬆️

Continue to review full report at Codecov.

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

Copy link
Contributor

@vam-google vam-google left a comment

LGTM

(with fingers crossed)

@igorbernstein2
Copy link
Collaborator Author

@igorbernstein2 igorbernstein2 commented Feb 21, 2020

It seems the presubmits are broken and are preventing merging prs

@igorbernstein2 igorbernstein2 merged commit 0a5b940 into googleapis:master Feb 21, 2020
6 of 8 checks passed
@igorbernstein2 igorbernstein2 deleted the increase-default-thread-pool branch Feb 21, 2020
@igorbernstein2
Copy link
Collaborator Author

@igorbernstein2 igorbernstein2 commented Feb 24, 2020

@vam-google would you mind cutting a release with this fix?

@vam-google
Copy link
Contributor

@vam-google vam-google commented Feb 24, 2020

I believe DPE team now handles this. @chingor13 can you cut a release?

@codyoss
Copy link
Member

@codyoss codyoss commented Feb 24, 2020

@igorbernstein2 Does Runtime.getRuntime().availableProcessors() work as expected in kubernetes container environments? I personally don't know, but a quick search yielded some people having issues where it returns 1 for them.

@igorbernstein2
Copy link
Collaborator Author

@igorbernstein2 igorbernstein2 commented Feb 24, 2020

Not really. If there is no cpu config, then it will default to 1, which is why the PR sets a minimum to 4 as before. This is just a bandaid, the real fix would be more along the lines of #869. However that approach was a bit controversial, so this change was put in place until @vam-google could figure out how to incorporate #869 without too much breakage

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

Successfully merging this pull request may close these issues.

None yet

4 participants