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

Add shortcut methods that create event loops and blocking task executor thread pool #3602

Merged
merged 2 commits into from Jun 21, 2021

Conversation

kezhenxu94
Copy link
Contributor

@kezhenxu94 kezhenxu94 commented Jun 3, 2021

Motivation:

It may be useful if a user can create a new event loop or blocking task executor that has the same life cycle with a ClientFactory or Server:

ClientFactory
  .builder()
  .workerGroup(3)
  .build()

Server
  .builder()
  .workerGroup(3)
  .blockingTaskExecutor(10)
  ...
  .build()

Modifications:

  • Added ClientFactoryBuilder.workerGroup(int)
  • Added ServerBuilder.workerGroup(int)
  • Added ServerBuilder.blockingTaskExecutor(int)

Result:

@codecov
Copy link

codecov bot commented Jun 3, 2021

Codecov Report

Merging #3602 (d028a26) into master (001a1f7) will increase coverage by 0.79%.
The diff coverage is 74.86%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3602      +/-   ##
============================================
+ Coverage     73.00%   73.80%   +0.79%     
- Complexity    10880    14387    +3507     
============================================
  Files           947     1263     +316     
  Lines         41936    54938   +13002     
  Branches       5207     7026    +1819     
============================================
+ Hits          30616    40546    +9930     
- Misses         8662    10804    +2142     
- Partials       2658     3588     +930     
Impacted Files Coverage Δ
...ommon/brave/RequestContextCurrentTraceContext.java 98.48% <ø> (ø)
...inecorp/armeria/client/AbstractEventLoopEntry.java 100.00% <ø> (ø)
...inecorp/armeria/client/AbstractEventLoopState.java 100.00% <ø> (ø)
.../main/java/com/linecorp/armeria/client/Client.java 66.66% <0.00%> (-33.34%) ⬇️
...ava/com/linecorp/armeria/client/ClientFactory.java 65.33% <0.00%> (-1.34%) ⬇️
...m/linecorp/armeria/client/ClientFactoryOption.java 71.42% <ø> (+1.11%) ⬆️
...java/com/linecorp/armeria/client/ClientOption.java 71.42% <ø> (-5.93%) ⬇️
...rp/armeria/client/ClientRequestContextWrapper.java 0.00% <0.00%> (ø)
.../armeria/client/ConnectionPoolListenerWrapper.java 0.00% <0.00%> (ø)
...m/linecorp/armeria/client/DecodedHttpResponse.java 85.00% <ø> (ø)
... and 1383 more

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 49594e9...d028a26. Read the comment docs.

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Thanks! @kezhenxu94

Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

Just some documentation comments 👍

Comment on lines 136 to 138
* Sets the worker {@link EventLoopGroup} which is responsible for performing socket I/O and running
* {@link Client#execute(ClientRequestContext, Request)}.
* If not set, {@linkplain CommonPools#workerGroup() the common worker group} is used.
Copy link
Member

Choose a reason for hiding this comment

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

How about this:

Uses a newly created {@link EventLoopGroup} with the specified number of threads for
performing socket I/O and running ...

If not set... could be removed.

Comment on lines 422 to 426
* Sets the worker {@link EventLoopGroup} which is responsible for performing socket I/O and running
* {@link Service#serve(ServiceRequestContext, Request)}.
* If not set, {@linkplain CommonPools#workerGroup() the common worker group} is used.
* The worker {@link EventLoopGroup} will be shut down when the {@link Server} stops.
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

@@ -94,7 +93,7 @@
@BeforeAll
static void beforeAll() {
// use different eventLoop from server's so that clients don't hang when the eventLoop in server hangs
clientFactory = ClientFactory.builder().workerGroup(EventLoopGroups.newEventLoopGroup(2), true).build();
clientFactory = ClientFactory.builder().workerGroup(2).build();
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for cleaning this up! 🙇

Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

Thanks a lot for quick turn-around, @kezhenxu94

@trustin trustin added this to the 1.9.0 milestone Jun 5, 2021
@trustin
Copy link
Member

trustin commented Jun 14, 2021

@minwoox Gentle ping

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Thanks!

@trustin
Copy link
Member

trustin commented Jun 14, 2021

@kezhenxu94 I've just realized you didn't add ServerBuilder.blockingTaskExecutor(int). Could you add one? You could use BlockingTaskExecutor.builder() to build one.

@kezhenxu94
Copy link
Contributor Author

@kezhenxu94 I've just realized you didn't add ServerBuilder.blockingTaskExecutor(int). Could you add one? You could use BlockingTaskExecutor.builder() to build one.

Added

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

LGTM once @ikhoon's comment is addressed. 😄

@kezhenxu94
Copy link
Contributor Author

Please squash when merging 🙇

Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

@trustin trustin merged commit 0fbd1cc into line:master Jun 21, 2021
heowc pushed a commit to heowc/armeria that referenced this pull request Jun 24, 2021
…or thread pool (line#3602)

Motivation:

It may be useful if a user can create a new event loop or blocking task executor that has the same life cycle with a `ClientFactory` or `Server`:

```java
ClientFactory
  .builder()
  .workerGroup(3)
  .build()

Server
  .builder()
  .workerGroup(3)
  .blockingTaskExecutor(10)
  ...
  .build()
```

Modifications:

- Added `ClientFactoryBuilder.workerGroup(int)`
- Added `ServerBuilder.workerGroup(int)`
- Added `ServerBuilder.blockingTaskExecutor(int)`

Result:

- Closes line#3597
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the shortcut methods that creates event loops and blocking task executor thread pool
4 participants