Add production checklist about maxNumEventLoopsPerEndpoint
#5297
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
0x1306e6d
requested review from
ikhoon,
jrhee17,
minwoox and
trustin
as code owners
ikhoon
reviewed
Nov 14, 2023
| @@ -96,3 +96,20 @@ See [Using CircuitBreaker with non-Armeria client](/docs/client-circuit-breaker# | |||
| ClientBuilder cb = Clients.builder(...); | |||
| cb.factory(cf); | |||
| ``` | |||
|
|
|||
| - Increase `maxNumEventLoopsPerEndpoint` and `maxNumEventLoopsPerHttp1Endpoint` when your clients send requests | |||
There was a problem hiding this comment.
How about explaining the effect of increasing the number of event loops?
Suggested change
| - Increase `maxNumEventLoopsPerEndpoint` and `maxNumEventLoopsPerHttp1Endpoint` when your clients send requests | |
| - Increase `<type://ClientFactoryBuilder.maxNumEventLoopsPerEndpoint(int)>` and | |
| `<type://ClientFactoryBuilder.maxNumEventLoopsPerHttp1Endpoint(int)>` when a client | |
| needs to send a large number of requests to a specific endpoint. The client will | |
| assign more CPU resources and create more connections by increasing the number of | |
| event loops. |
jrhee17
approved these changes
Nov 14, 2023
| import com.linecorp.armeria.client.ClientFactory; | ||
| import com.linecorp.armeria.client.ClientFactoryBuilder; | ||
|
|
||
| ClientFactoryBuilder cfb = new ClientFactoryBuilder(); |
There was a problem hiding this comment.
The default constructor isn't public (optionally it would be great if the other examples in this page are updated)
Suggested change
| ClientFactoryBuilder cfb = new ClientFactoryBuilder(); | |
| ClientFactoryBuilder cfb = ClientFactory.builder(); |
Comment on lines
100
to
101
| - Increase `maxNumEventLoopsPerEndpoint` and `maxNumEventLoopsPerHttp1Endpoint` when your clients send requests | ||
| to a small number of servers. As an Endpoint uses a single event loop, there may be a bottleneck. |
There was a problem hiding this comment.
- What do you think of adding
Considersince performance may not always be better. - What do you think of conveying why there is a bottleneck?
Suggested change
| - Increase `maxNumEventLoopsPerEndpoint` and `maxNumEventLoopsPerHttp1Endpoint` when your clients send requests | |
| to a small number of servers. As an Endpoint uses a single event loop, there may be a bottleneck. | |
| - Consider increasing `maxNumEventLoopsPerEndpoint` and `maxNumEventLoopsPerHttp1Endpoint` for better request throughput. Doing so may increase performance at the cost of opening more connections. |
There was a problem hiding this comment.
I saw this comment late #5297 (comment) .
I think using this comment is fine, but still I think adding "Consider" is a good idea
Co-authored-by: minux <songmw725@gmail.com>
Co-authored-by: minux <songmw725@gmail.com>
|
Thank you @minwoox! |
minwoox
reviewed
Nov 23, 2023
eottabom
pushed a commit
to eottabom/armeria
that referenced
this pull request
Jan 18, 2024
Motivation: Using `maxNumEventLoopsPerEndpoint` and `maxNumEventLoopsPerHttp1Endpoint` with the default value (1) may be a bottleneck when a client sends lots of requests to a small number of servers because an `Endpoint` uses a single event loop. Modifications: - Add a production checklist about `maxNumEventLoopsPerEndpoint` and `maxNumEventLoopsPerHttp1Endpoint`. Result: - Users would recognize the bottleneck problem earlier.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
Using
maxNumEventLoopsPerEndpointandmaxNumEventLoopsPerHttp1Endpointwith the default value (1) may be a bottleneck when a client sends lots of requests to a small number of servers because anEndpointuses a single event loop.Modifications:
maxNumEventLoopsPerEndpointandmaxNumEventLoopsPerHttp1Endpoint.Result: