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

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

@asdf2014 asdf2014 commented Apr 20, 2020

No description provided.

@linux-foundation-easycla
Copy link

@linux-foundation-easycla linux-foundation-easycla bot commented Apr 20, 2020

CLA Check
The committers are authorized under a signed CLA.

Copy link
Contributor

@creamsoup creamsoup left a comment

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

@@ -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

@creamsoup creamsoup Apr 20, 2020

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

@@ -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

@creamsoup creamsoup Apr 20, 2020

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 label Apr 21, 2020
@grpc-kokoro grpc-kokoro removed the kokoro:run label Apr 21, 2020
@asdf2014
Copy link
Contributor Author

@asdf2014 asdf2014 commented Apr 21, 2020

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 label Apr 21, 2020
@grpc-kokoro grpc-kokoro removed the kokoro:force-run label Apr 21, 2020
@creamsoup
Copy link
Contributor

@creamsoup creamsoup commented Apr 21, 2020

that's good to know. thanks.

@creamsoup creamsoup requested a review from voidzcy Apr 21, 2020
@voidzcy
Copy link
Contributor

@voidzcy 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

@asdf2014 asdf2014 commented Apr 21, 2020

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 Apr 21, 2020
@ejona86 ejona86 changed the title Support setting options of boss and worker in NettyServer Support setting options of boss in NettyServer Apr 21, 2020
Copy link
Member

@ejona86 ejona86 left a comment

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
12 of 13 checks passed
@creamsoup
Copy link
Contributor

@creamsoup creamsoup commented Apr 21, 2020

merged, thanks @asdf2014!

@ejona86
Copy link
Member

@ejona86 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

@asdf2014 asdf2014 deleted the support_options_for_nettyserver branch Apr 22, 2020
@asdf2014
Copy link
Contributor Author

@asdf2014 asdf2014 commented Apr 22, 2020

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 issue Jan 15, 2021
@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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants