Skip to content

set maxRequestsPerHost of okhttp to support slow response requests and high TPS#708

Merged
artursouza merged 4 commits into
dapr:masterfrom
skyao:fix-okjava-limitation
Mar 21, 2022
Merged

set maxRequestsPerHost of okhttp to support slow response requests and high TPS#708
artursouza merged 4 commits into
dapr:masterfrom
skyao:fix-okjava-limitation

Conversation

@skyao

@skyao skyao commented Mar 18, 2022

Copy link
Copy Markdown
Member

Description

In dapr java sdk, there are some default settings in okhttp which we don't set and use the default value:

  • maxRequests: default to 64
  • maxRequestsPerHost: default to 5!
  • maxIdleConnections

The default value 5 of maxRequestsPerHost is totally unacceptable, it will lead to very serious problems of very low throughput as discribed in issue #709.

And the default value 64 of maxRequests is not enough for the user case of high TPS. For example, if the response time is 100 ms, 64 connections leads to at most 640 TPS.

Issue reference

This PR is to fix this issue.

#709

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

@skyao skyao requested review from a team as code owners March 18, 2022 03:19

@mukundansundar mukundansundar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@skyao Keep in mind that in future we will be moving away from OkHttp and move to native Java Client. But I believe these parameters will still add value there.

Comment thread sdk/src/main/java/io/dapr/client/DaprHttpBuilder.java Outdated
Comment thread sdk/src/main/java/io/dapr/client/DaprHttpBuilder.java
@skyao skyao changed the title set maxRequestsPerHost of okjava to support slow response requests and high TPS set maxRequestsPerHost of okhttp to support slow response requests and high TPS Mar 18, 2022
@skyao

skyao commented Mar 18, 2022

Copy link
Copy Markdown
Member Author

@skyao Keep in mind that in future we will be moving away from OkHttp and move to native Java Client. But I believe these parameters will still add value there.

OK.

Before we changed to native java client, we still need OkHttp. The fix for default maxRequestPerHost is very important.

artursouza
artursouza previously approved these changes Mar 18, 2022
@artursouza

Copy link
Copy Markdown
Contributor

@skyao Please, sign off the commits. This is a new requirement in our repos since the CNCF donation.

mukundansundar
mukundansundar previously approved these changes Mar 18, 2022
skyao added 2 commits March 19, 2022 08:27
…d high TPS

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>
Signed-off-by: Sky Ao <aoxiaojian@gmail.com>
@skyao skyao dismissed stale reviews from mukundansundar and artursouza via 26a1c70 March 19, 2022 00:28
Signed-off-by: Sky Ao <aoxiaojian@gmail.com>
Comment thread sdk/src/main/java/io/dapr/config/Properties.java Outdated
Signed-off-by: Sky Ao <aoxiaojian@gmail.com>
@codecov

codecov Bot commented Mar 20, 2022

Copy link
Copy Markdown

Codecov Report

Merging #708 (b13a2fc) into master (b7e41a0) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #708      +/-   ##
============================================
+ Coverage     80.66%   80.72%   +0.06%     
  Complexity      993      993              
============================================
  Files            81       81              
  Lines          3010     3020      +10     
  Branches        340      340              
============================================
+ Hits           2428     2438      +10     
  Misses          416      416              
  Partials        166      166              
Impacted Files Coverage Δ
.../src/main/java/io/dapr/client/DaprHttpBuilder.java 94.44% <100.00%> (+2.77%) ⬆️
sdk/src/main/java/io/dapr/config/Properties.java 81.81% <100.00%> (+4.04%) ⬆️

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 b7e41a0...b13a2fc. Read the comment docs.

@mukundansundar mukundansundar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

@artursouza artursouza merged commit e3fa24f into dapr:master Mar 21, 2022
@skyao skyao deleted the fix-okjava-limitation branch March 21, 2022 07:05
fackee pushed a commit to fackee/java-sdk that referenced this pull request Mar 28, 2022
…d high TPS (dapr#708)

* set maxRequestsPerHost of okjava to support slow response requests and high TPS

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

* fix typo in comments

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

* update comments

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

* update javadoc

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>
Signed-off-by: zjx244729 <zjx244729@alibaba-inc.com>
fackee pushed a commit to fackee/java-sdk that referenced this pull request Mar 28, 2022
…d high TPS (dapr#708)

* set maxRequestsPerHost of okjava to support slow response requests and high TPS

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

* fix typo in comments

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

* update comments

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

* update javadoc

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>
Signed-off-by: zjx244729 <zjx244729@alibaba-inc.com>
fackee pushed a commit to fackee/java-sdk that referenced this pull request Mar 28, 2022
…d high TPS (dapr#708)

* set maxRequestsPerHost of okjava to support slow response requests and high TPS

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

* fix typo in comments

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

* update comments

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

* update javadoc

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>
Signed-off-by: zjx244729 <zjx244729@alibaba-inc.com>
fackee pushed a commit to fackee/java-sdk that referenced this pull request Mar 28, 2022
…d high TPS (dapr#708)

* set maxRequestsPerHost of okjava to support slow response requests and high TPS

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

* fix typo in comments

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

* update comments

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

* update javadoc

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>
Signed-off-by: zjx244729 <zjx244729@alibaba-inc.com>
fackee pushed a commit to fackee/java-sdk that referenced this pull request Mar 28, 2022
…d high TPS (dapr#708)

* set maxRequestsPerHost of okjava to support slow response requests and high TPS

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

* fix typo in comments

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

* update comments

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

* update javadoc

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>
Signed-off-by: zjx244729 <zjx244729@alibaba-inc.com>
artursouza pushed a commit to pkedy/java-sdk that referenced this pull request Jun 22, 2022
…d high TPS (dapr#708)

* set maxRequestsPerHost of okjava to support slow response requests and high TPS

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

* fix typo in comments

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

* update comments

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

* update javadoc

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>
Signed-off-by: Artur Souza <artursouza.ms@outlook.com>
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