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

Jump to bottom

Support setting options of boss in NettyServer #6947

Merged
merged 3 commits into from Apr 21, 2020
Merged

Support setting options of boss in NettyServer #6947

merged 3 commits into from Apr 21, 2020

Conversation

asdf2014
Copy link
Contributor

No description provided.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 20, 2020 •
edited

CLA Check
The committers are authorized under a signed CLA.

creamsoup
creamsoup approved these changes Apr 20, 2020
View reviewed changes
Copy link
Contributor

@creamsoup creamsoup left a comment

Choose a reason for hiding this comment

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

just out of curiosity, which options are you planning to use?

netty/src/main/java/io/grpc/netty/NettyServerBuilder.java Outdated
@@ -189,10 +190,21 @@ public NettyServerBuilder channelFactory(ChannelFactory<? extends ServerChannel>
* Specifies a channel option. As the underlying channel as well as network implementation may
* ignore this value applications should consider it a hint.
*
* @since 1.28.1
Copy link
Contributor

Choose a reason for hiding this comment

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

1.29.0 branch cut is already done. it should 1.30.0

Suggested change
* @since 1.28.1
* @since 1.30.0

netty/src/test/java/io/grpc/netty/NettyServerTest.java Outdated
@@ -175,6 +177,7 @@ public void childChannelOptions() throws Exception {
NettyServer ns = new NettyServer(
addr,
new ReflectiveChannelFactory<>(NioServerSocketChannel.class),
new HashMap<ChannelOption<?>, Object>(),
channelOptions,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can you rename this to childChannelOptions to avoid confusion?

@creamsoup creamsoup added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Apr 21, 2020
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Apr 21, 2020
@asdf2014
Copy link
Contributor Author

Hi, @creamsoup . Thanks for your comments. All of them has been patched. Many options related to the boss ELG will be set, mainly the SO_BACKLOG parameter, because this has been hard-coded to 128, which is not enough in my case.

@creamsoup creamsoup added the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Apr 21, 2020
@grpc-kokoro grpc-kokoro removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Apr 21, 2020
@creamsoup
Copy link
Contributor

that's good to know. thanks.

@creamsoup creamsoup requested a review from voidzcy April 21, 2020 05:54
@voidzcy
Copy link
Contributor

voidzcy commented Apr 21, 2020

I don't have expertise to review this and getting its context takes time. Maybe request review from someone else? 😄

@asdf2014
Copy link
Contributor Author

Hi, @creamsoup . As @voidzcy said, would you please ask others to check it out.

@creamsoup creamsoup requested review from ejona86 and removed request for voidzcy April 21, 2020 16:37
@ejona86 ejona86 changed the title Support setting options of boss and worker in NettyServer Support setting options of boss in NettyServer Apr 21, 2020
ejona86
ejona86 approved these changes Apr 21, 2020
View reviewed changes
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

I changed the PR title to remove "worker" since worker configuration was already supported.

@creamsoup creamsoup merged commit 9411e97 into grpc:master Apr 21, 2020
@creamsoup
Copy link
Contributor

merged, thanks @asdf2014!

@ejona86
Copy link
Member

ejona86 commented Apr 21, 2020

And actually, it looks like us setting the SO_BACKLOG was just cargo-culting (that is some very old code). We should probably just remove it and use Netty's default, which is 200 on Windows and the system max on Linux (commonly 128).

https://github.com/netty/netty/blob/0cde4d9cb4d19ddc0ecafc5be7c5f7c781a1f6e9/transport/src/main/java/io/netty/channel/socket/DefaultServerSocketChannelConfig.java#L44
https://github.com/netty/netty/blob/856f1185e1c602356e489a5dae7d6d74f42d17f6/common/src/main/java/io/netty/util/NetUtil.java#L253

@creamsoup creamsoup mentioned this pull request Apr 21, 2020
Merged
@asdf2014 asdf2014 deleted the support_options_for_nettyserver branch April 22, 2020 02:19
@asdf2014
Copy link
Contributor Author

Hi, @ejona86 . Thanks for your approval. I agree with you, we shouldn't hard code the value of SO_BACKLOG option in NettyServer. This will also cause the /proc/sys/net/core/somaxconn system configuration to not work.

dfawley pushed a commit to dfawley/grpc-java that referenced this pull request Jan 15, 2021
@asdf2014 @dfawley
netty: support setting options of boss in NettyServer (grpc#6947)
1993e40
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@creamsoup creamsoup creamsoup approved these changes

@ejona86 ejona86 ejona86 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@asdf2014 @creamsoup @voidzcy @ejona86 @grpc-kokoro