netty server sockets shall be configurable #3755

xiaoshuang-lu · 2017-11-16T09:34:59Z

No description provided.

thelinuxfoundation · 2017-11-16T09:35:00Z

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

User @xiaoshuang-lu isn't covered by a CLA. They will need to complete the form at https://identity.linuxfoundation.org/projects/cncf

Regards,
The Linux Foundation CLA GitHub bot

thelinuxfoundation · 2017-11-16T09:48:16Z

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

User @xiaoshuang-lu isn't covered by a CLA. They will need to complete the form at https://identity.linuxfoundation.org/projects/cncf

Regards,
The Linux Foundation CLA GitHub bot

carl-mastrangelo · 2017-11-16T18:40:01Z

netty/src/main/java/io/grpc/netty/NettyServerBuilder.java

+   * Specifies a channel option. As the underlying channel as well as network implementation may
+   * ignore this value applications should consider it a hint.
+   */
+  public <T> NettyServerBuilder withOption(ChannelOption<T> option, T value) {


withChildOption? These won't affect the listening socket itself.

Thanks, Carl. I will change the method name to withChildOption later.

carl-mastrangelo · 2017-11-17T19:46:50Z

netty/src/main/java/io/grpc/netty/NettyServerBuilder.java

@@ -403,7 +417,7 @@ protected NettyServer buildTransportServer(
    }

    return new NettyServer(
-        address, channelType, bossEventLoopGroup, workerEventLoopGroup,
+        address, channelType, channelOptions, bossEventLoopGroup, workerEventLoopGroup,


This needs to be a copy of the map. If someone modifies the map later, existing servers would accidentally see the changes.

Okay. How about coping the map in NettyServer constructor?

This seems fine to me.

thelinuxfoundation · 2017-11-20T03:38:06Z

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

User @xiaoshuang-lu isn't covered by a CLA. They will need to complete the form at https://identity.linuxfoundation.org/projects/cncf

Regards,
The Linux Foundation CLA GitHub bot

carl-mastrangelo

Would you midn adding a test (something like starting a server with some options, and then checking from the handler the channel option is properly set?)

@ejona86 wanna take a look?

carl-mastrangelo · 2017-11-20T21:12:33Z

netty/src/main/java/io/grpc/netty/NettyServerBuilder.java

@@ -403,7 +417,7 @@ protected NettyServer buildTransportServer(
    }

    return new NettyServer(
-        address, channelType, bossEventLoopGroup, workerEventLoopGroup,
+        address, channelType, channelOptions, bossEventLoopGroup, workerEventLoopGroup,


This seems fine to me.

ejona86

+1 to having a simple test. Otherwise, LGTM

ejona86 · 2017-11-20T21:17:36Z

netty/src/main/java/io/grpc/netty/NettyServer.java

@@ -122,6 +129,7 @@ public int getPort() {
    return ((InetSocketAddress) localAddr).getPort();
  }

+  @SuppressWarnings("unchecked")


Instead of disabling the checking for the entire (large) method, make a temporary variable to apply the annotation to:

@SuppressWarnings("unchecked") ChannelOption<Object> key = (ChannelOption<Object>) entry.getKey(); b.childOption(key, entry.getValue());

dapengzhang0 · 2017-11-21T01:42:44Z

netty/src/main/java/io/grpc/netty/NettyServerBuilder.java

+   * Specifies a channel option. As the underlying channel as well as network implementation may
+   * ignore this value applications should consider it a hint.
+   */
+  public <T> NettyServerBuilder withChildOption(ChannelOption<T> option, T value) {


Add @since 1.9.0 in the javadoc.

carl-mastrangelo · 2017-11-21T02:17:58Z

@xiaoshuang-lu You'll need to sign the CLA before we an accept this PR.

thelinuxfoundation · 2017-11-21T11:19:20Z

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

User @xiaoshuang-lu isn't covered by a CLA. They will need to complete the form at https://identity.linuxfoundation.org/projects/cncf

Regards,
The Linux Foundation CLA GitHub bot

thelinuxfoundation · 2017-11-21T11:34:09Z

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

User @xiaoshuang-lu isn't covered by a CLA. They will need to complete the form at https://identity.linuxfoundation.org/projects/cncf

Regards,
The Linux Foundation CLA GitHub bot

thelinuxfoundation · 2017-11-21T11:44:39Z

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

User @xiaoshuang-lu isn't covered by a CLA. They will need to complete the form at https://identity.linuxfoundation.org/projects/cncf

Regards,
The Linux Foundation CLA GitHub bot

xiaoshuang-lu · 2017-11-21T12:00:27Z

@carl-mastrangelo A testing case was appended to NettyServerTest.

carl-mastrangelo · 2017-11-21T19:06:15Z

netty/src/test/java/io/grpc/netty/NettyServerTest.java

+        try {
+          socket.connect(new InetSocketAddress("localhost", 9999), 8000);
+        } catch (Exception e) {
+          Assert.assertTrue("" + e, false);


just let the exception propagate. Make the test signature a throws

carl-mastrangelo · 2017-11-21T19:07:32Z

netty/src/test/java/io/grpc/netty/NettyServerTest.java

+            .isEqualTo(sendBufferSize);
+
+        countDownLatch.countDown();
+


nit, if an error happens here (like an assertion fails), the test thread wont find out about it. On receiving these values you should set them in an atomic integer or something, and then check for the test thread.

travis-ci node will overwrite receiveBufferSize and sendBufferSize?

carl-mastrangelo · 2017-11-22T19:59:51Z

netty/src/test/java/io/grpc/netty/NettyServerTest.java

@@ -91,4 +102,65 @@ public void getPort_notStarted() throws Exception {

    assertThat(ns.getPort()).isEqualTo(-1);
  }
+
+  @Test(timeout = 60000)


We should not use timeout here, and instead use a JUnit4 timeout rule:

@Rule public final Timeout globalTimeout = Timeout.seconds(30);

The reason is that the timeout rule works correctly when debugging / stepping through code.

carl-mastrangelo

LGTM. @ejona86 API looks fine?

ejona86 · 2017-11-22T23:10:11Z

@carl-mastrangelo, I don't see any changes to the API since my last approval. The NettyServerBuilder.withChildOption seems fine to me.

carl-mastrangelo · 2017-11-23T00:00:44Z

@xiaoshuang-lu merged, Thanks!

carl-mastrangelo reviewed Nov 16, 2017

View reviewed changes

carl-mastrangelo reviewed Nov 17, 2017

View reviewed changes

carl-mastrangelo reviewed Nov 20, 2017

View reviewed changes

ejona86 approved these changes Nov 20, 2017

View reviewed changes

dapengzhang0 reviewed Nov 21, 2017

View reviewed changes

carl-mastrangelo reviewed Nov 21, 2017

View reviewed changes

netty server sockets shall be configurable

05a2085

carl-mastrangelo reviewed Nov 22, 2017

View reviewed changes

carl-mastrangelo approved these changes Nov 22, 2017

View reviewed changes

dapengzhang0 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Nov 22, 2017

kokoro-team removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Nov 22, 2017

carl-mastrangelo merged commit cdb1f54 into grpc:master Nov 23, 2017

carl-mastrangelo added the enhancement label Nov 23, 2017

carl-mastrangelo added this to the 1.9 milestone Nov 30, 2017

carl-mastrangelo added the Type: Feature label Nov 30, 2017

lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

netty server sockets shall be configurable #3755

netty server sockets shall be configurable #3755

xiaoshuang-lu commented Nov 16, 2017

thelinuxfoundation commented Nov 16, 2017

thelinuxfoundation commented Nov 16, 2017

carl-mastrangelo Nov 16, 2017

xiaoshuang-lu Nov 17, 2017

carl-mastrangelo Nov 17, 2017

xiaoshuang-lu Nov 20, 2017

carl-mastrangelo Nov 20, 2017

thelinuxfoundation commented Nov 20, 2017

carl-mastrangelo left a comment

carl-mastrangelo Nov 20, 2017

ejona86 left a comment

ejona86 Nov 20, 2017

xiaoshuang-lu Nov 21, 2017

dapengzhang0 Nov 21, 2017

xiaoshuang-lu Nov 21, 2017

carl-mastrangelo commented Nov 21, 2017

thelinuxfoundation commented Nov 21, 2017

thelinuxfoundation commented Nov 21, 2017

thelinuxfoundation commented Nov 21, 2017

xiaoshuang-lu commented Nov 21, 2017

carl-mastrangelo Nov 21, 2017

xiaoshuang-lu Nov 22, 2017

carl-mastrangelo Nov 21, 2017

xiaoshuang-lu Nov 22, 2017

carl-mastrangelo Nov 22, 2017

carl-mastrangelo left a comment

ejona86 commented Nov 22, 2017

carl-mastrangelo commented Nov 23, 2017

netty server sockets shall be configurable #3755

netty server sockets shall be configurable #3755

Conversation

xiaoshuang-lu commented Nov 16, 2017

thelinuxfoundation commented Nov 16, 2017

thelinuxfoundation commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thelinuxfoundation commented Nov 20, 2017

carl-mastrangelo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejona86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carl-mastrangelo commented Nov 21, 2017

thelinuxfoundation commented Nov 21, 2017

thelinuxfoundation commented Nov 21, 2017

thelinuxfoundation commented Nov 21, 2017

xiaoshuang-lu commented Nov 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carl-mastrangelo left a comment

Choose a reason for hiding this comment

ejona86 commented Nov 22, 2017

carl-mastrangelo commented Nov 23, 2017