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 #578 Switch "fixed" connection pool by default for TCP/HTTP client #812

Merged
merged 1 commit into from
Aug 9, 2019

Conversation

violetagg
Copy link
Member

No description provided.

@violetagg violetagg added this to the 0.9.0.M4 milestone Aug 7, 2019
@violetagg violetagg requested a review from smaldini August 7, 2019 20:53
@violetagg
Copy link
Member Author

@Buzzardo Can you take a look at the documentation? Thanks

@smaldini Do we want idle time by default?

The max number of the channels in the pool will be 500 with 45s acquisition timeout

Copy link
Contributor

@Buzzardo Buzzardo left a comment

Choose a reason for hiding this comment

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

Thanks. :)

----
====

NOTE: Be cautious with "`elastic`" connection pool when you expect a high load. You might experience
Copy link
Contributor

Choose a reason for hiding this comment

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

Add "an" after "with" on this line, please.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -302,11 +302,15 @@ public class Application {

== Connection Pool

By default, the `TCP` client uses an "`elastic`" connection pool. This means that the implementation creates a new channel
if someone tries to acquire a channel but none is in the pool. Also, there is no limit on the maximum concurrent channels.
By default, the `TCP` client uses a "`fixed`" connection pool with `500` as a maximum number of the channels and
Copy link
Contributor

Choose a reason for hiding this comment

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

"a maximum number" should be "the maximum number".

Copy link
Member Author

Choose a reason for hiding this comment

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

done

By default, the `TCP` client uses an "`elastic`" connection pool. This means that the implementation creates a new channel
if someone tries to acquire a channel but none is in the pool. Also, there is no limit on the maximum concurrent channels.
By default, the `TCP` client uses a "`fixed`" connection pool with `500` as a maximum number of the channels and
`45s` as acquisition timeout.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please insert "the" before "acquisition" on this line.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

By default, the `TCP` client uses a "`fixed`" connection pool with `500` as a maximum number of the channels and
`45s` as acquisition timeout.
This means that the implementation creates a new channel if someone tries to acquire a channel but none is in the pool.
When the maximum number of the channels in the pool is reached, new tries to acquire a channel will be delayed
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change "will be" to "are".

Copy link
Member Author

Choose a reason for hiding this comment

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

done

The implementation uses `FIFO` order for channels in the pool regardless of whether it is "`elastic`" or "`fixed`".
By default there is no idle time specified for the channels in the pool
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comma after "default" and a period at the end.

Copy link
Member Author

Choose a reason for hiding this comment

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

done


If you need a "`fixed`" connection pool, you can apply the following configuration by using
If you need a "`elastic`" connection pool, you can apply the following configuration by using
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change "need a" to "need an".

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@codecov-io
Copy link

codecov-io commented Aug 8, 2019

Codecov Report

Merging #812 into master will increase coverage by 0.04%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #812      +/-   ##
============================================
+ Coverage     67.09%   67.13%   +0.04%     
- Complexity     1431     1433       +2     
============================================
  Files           137      137              
  Lines          6704     6704              
  Branches        862      862              
============================================
+ Hits           4498     4501       +3     
+ Misses         1745     1744       -1     
+ Partials        461      459       -2
Impacted Files Coverage Δ Complexity Δ
src/main/java/reactor/netty/tcp/TcpResources.java 57.14% <0%> (ø) 26 <0> (ø) ⬇️
...eactor/netty/http/client/HttpClientOperations.java 64.22% <0%> (+0.29%) 71% <0%> (+1%) ⬆️
...ctor/netty/resources/PooledConnectionProvider.java 82.67% <0%> (+0.78%) 26% <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 7f67e16...ded78a1. Read the comment docs.

@violetagg violetagg merged commit ded78a1 into master Aug 9, 2019
@violetagg violetagg deleted the conn-pool-fixed branch August 21, 2019 14:46
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