You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by lvfangmin <gi...@git.apache.org> on 2018/08/07 00:54:14 UTC
[GitHub] zookeeper pull request #593: [ZOOKEEPER-3111] Add socket buffer size option ...
GitHub user lvfangmin opened a pull request:
https://github.com/apache/zookeeper/pull/593
[ZOOKEEPER-3111] Add socket buffer size option to tune the TCP throughput between leader and learner
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/lvfangmin/zookeeper ZOOKEEPER-3111
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/593.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #593
----
commit 3c009a4b7e294479edade58d012df818a72edf19
Author: Fangmin Lyu <al...@...>
Date: 2018-08-07T00:53:13Z
Add socket buffer size option to tune the TCP throughput between leader and learner
----
---
[GitHub] zookeeper issue #593: [ZOOKEEPER-3111] Add socket buffer size option to tune...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/593
Thanks @anmolnar for reviewing, the SocketUtil class is pretty straightforward, but I'll add a simple test case for it.
---
[GitHub] zookeeper issue #593: [ZOOKEEPER-3111] Add socket buffer size option to tune...
Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on the issue:
https://github.com/apache/zookeeper/pull/593
> What do you mean by 'do regression' exactly?
@anmolnar like this(grin):
`int index = 0;
while (true) {
try {
testSetSocketBufferSize();
} catch (Exception e) {
e.printStackTrace();
}
try {
Thread.sleep(200);
} catch (InterruptedException e) {
}
System.out.println("-----------" + (index++)+" passed!!!");
}`
---
[GitHub] zookeeper pull request #593: [ZOOKEEPER-3111] Add socket buffer size option ...
Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/593#discussion_r211097882
--- Diff: src/java/test/org/apache/zookeeper/server/SocketUtilTest.java ---
@@ -0,0 +1,46 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server;
+
+import java.net.Socket;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+public class SocketUtilTest {
+
+ @Test
+ public void testSetSocketBufferSize() throws Exception {
+ Socket s = new Socket();
+ int initSendBufferSize = s.getSendBufferSize();
+ int initReceiveBufferSize = s.getReceiveBufferSize();
+
+ // check buffer size won't be changed without set NETWORK_BUFFER_SIZE
+ SocketUtil.setSocketBufferSize("test", s);
+ Assert.assertEquals(initSendBufferSize, s.getSendBufferSize());
+ Assert.assertEquals(initReceiveBufferSize, s.getReceiveBufferSize());
+
+ // set NETWORK_BUFFER_SIZE
+ int networkBufferSize = 1024 * 1024;
+ SocketUtil.setNetworkBufferSize(networkBufferSize);
+ SocketUtil.setSocketBufferSize("test", s);
+ Assert.assertEquals(networkBufferSize, s.getSendBufferSize());
+ Assert.assertEquals(networkBufferSize, s.getReceiveBufferSize());
--- End diff --
`testSetSocketBufferSize` has failed due to Line 43,I guess it will also fail due to Line 36(no test)
---
[GitHub] zookeeper pull request #593: [ZOOKEEPER-3111] Add socket buffer size option ...
Posted by nkalmar <gi...@git.apache.org>.
Github user nkalmar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/593#discussion_r211551473
--- Diff: src/java/main/org/apache/zookeeper/server/SocketUtil.java ---
@@ -0,0 +1,63 @@
+package org.apache.zookeeper.server;
--- End diff --
Please add Apache License Header - the Jenkins build will also fail if missing.
---
[GitHub] zookeeper pull request #593: [ZOOKEEPER-3111] Add socket buffer size option ...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/593#discussion_r212069968
--- Diff: src/java/main/org/apache/zookeeper/server/SocketUtil.java ---
@@ -0,0 +1,63 @@
+package org.apache.zookeeper.server;
+
+import java.net.ServerSocket;
+import java.net.Socket;
+import java.net.SocketAddress;
+import java.net.SocketException;
+import java.io.IOException;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class SocketUtil {
+
+ private static final Logger LOG = LoggerFactory.getLogger(SocketUtil.class);
+
+ public static final String NETWORK_BUFFER_SIZE = "zookeeper.NetworkBufferSize";
+ protected static int networkBufferSize;
--- End diff --
Yes, I realized that, our internal code is using uppercase, but I agree we should change it to keep consistent.
---
[GitHub] zookeeper issue #593: [ZOOKEEPER-3111] Add socket buffer size option to tune...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/593
@maoling, sorry for the lately reply, I forgot to reply this PR.
We've done some benchmark when we added this code a year ago, I was trying to find some benchmark data at that point, but I cannot find it yet.
I've done some benchmark internally today, and found the performance of different buffer size is highly depending on the traffic pattern, and may actually introduce higher latency, so it's hard to give a suggested buffer size. But I don't think the extra negotiation time during connecting is a problem here, since we only set the buffer size between quorum servers, which is very few connections and it's stable.
Internally, we're not customizing the buffer size with this option anymore, instead we're using the system TCP Autotuning options net.ipv4.tcp_wmem and net.ipv4.tcp_rmem, which are more flexible and will automatically adjusts socket buffer sizes as needed to optimally balance TCP performance and memory usage, so we're not customizing the buffer size anymore.
**As for the unit test**, those SO_SENDBUF and SO_RCVBUF options are used by the platform's networking code as a hint for the size to set the underlying network I/O buffers, so it doesn't always guarantee it will use what you told it to. So I think this test is not that useful from this point of view.
Overall, I think we probably don't need this custom option anymore because it's hard to configure right.
---
[GitHub] zookeeper pull request #593: [ZOOKEEPER-3111] Add socket buffer size option ...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin closed the pull request at:
https://github.com/apache/zookeeper/pull/593
---
[GitHub] zookeeper pull request #593: [ZOOKEEPER-3111] Add socket buffer size option ...
Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/593#discussion_r214507553
--- Diff: src/java/main/org/apache/zookeeper/server/SocketUtil.java ---
@@ -13,18 +31,18 @@
--- End diff --
some useless import?
---
[GitHub] zookeeper issue #593: [ZOOKEEPER-3111] Add socket buffer size option to tune...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/593
Very good point @maoling
Docs should be part of this PR.
---
[GitHub] zookeeper issue #593: [ZOOKEEPER-3111] Add socket buffer size option to tune...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/593
@maoling I've run the test a few times on Mac and CentOs, but it doesn't seem to be flaky for me.
What do you mean by 'do regression' exactly?
---
[GitHub] zookeeper issue #593: [ZOOKEEPER-3111] Add socket buffer size option to tune...
Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on the issue:
https://github.com/apache/zookeeper/pull/593
document `zookeeper.NetworkBufferSize`?
---
[GitHub] zookeeper issue #593: [ZOOKEEPER-3111] Add socket buffer size option to tune...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/593
@lvfangmin The patch looks good to me, but you need to tweak the testing side to get a green build.
---
[GitHub] zookeeper issue #593: [ZOOKEEPER-3111] Add socket buffer size option to tune...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/593
@maoling thanks for providing the reference to KAFKA-200, it's very useful.
---
[GitHub] zookeeper pull request #593: [ZOOKEEPER-3111] Add socket buffer size option ...
Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/593#discussion_r211097828
--- Diff: src/java/main/org/apache/zookeeper/server/SocketUtil.java ---
@@ -0,0 +1,63 @@
+package org.apache.zookeeper.server;
+
+import java.net.ServerSocket;
+import java.net.Socket;
+import java.net.SocketAddress;
+import java.net.SocketException;
+import java.io.IOException;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class SocketUtil {
+
+ private static final Logger LOG = LoggerFactory.getLogger(SocketUtil.class);
+
+ public static final String NETWORK_BUFFER_SIZE = "zookeeper.NetworkBufferSize";
+ protected static int networkBufferSize;
--- End diff --
all the Java system property starts with lowercase,so `zookeeper.networkBufferSize`(same with the document) is better?
---
[GitHub] zookeeper issue #593: [ZOOKEEPER-3111] Add socket buffer size option to tune...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/593
Updated the doc based on @breed's comment, this should be ready to get in now.
---
[GitHub] zookeeper pull request #593: [ZOOKEEPER-3111] Add socket buffer size option ...
Posted by breed <gi...@git.apache.org>.
Github user breed commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/593#discussion_r212782182
--- Diff: zookeeper-docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -1641,6 +1641,24 @@ server.3=zoo3:2888:3888</programlisting>
Default is "10000".</para>
</listitem>
</varlistentry>
+
+ <varlistentry>
+ <term>networkBufferSize</term>
+
+ <listitem>
+ <para>(Java system property: zookeeper.<emphasis
+ role="bold">networkBufferSize</emphasis>)</para>
+
+ <para><emphasis role="bold">New in 3.6.0:</emphasis>
+ Sets the socket send and receive buffer size between leader
--- End diff --
nit: buffer size in bytes
it probably should be obvious, but it's worth being extra clear.
---
[GitHub] zookeeper pull request #593: [ZOOKEEPER-3111] Add socket buffer size option ...
Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/593#discussion_r214507592
--- Diff: src/java/test/org/apache/zookeeper/server/SocketUtilTest.java ---
@@ -37,7 +37,7 @@ public void testSetSocketBufferSize() throws Exception {
Assert.assertEquals(initReceiveBufferSize, s.getReceiveBufferSize());
// set NETWORK_BUFFER_SIZE
- int networkBufferSize = 1024 * 1024;
+ int networkBufferSize = 32 * 1024;
SocketUtil.setNetworkBufferSize(networkBufferSize);
SocketUtil.setSocketBufferSize("test", s);
--- End diff --
Although it passed the test,but When do regression,it will also be a flaky(you can see it by hitting it again and again).
could you plz look at it?
---
[GitHub] zookeeper issue #593: [ZOOKEEPER-3111] Add socket buffer size option to tune...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/593
@lvfangmin Cool, thanks for the good addition.
Please write some unit tests for the `SocketUtil` class.
---
[GitHub] zookeeper pull request #593: [ZOOKEEPER-3111] Add socket buffer size option ...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/593#discussion_r212070271
--- Diff: src/java/main/org/apache/zookeeper/server/SocketUtil.java ---
@@ -0,0 +1,63 @@
+package org.apache.zookeeper.server;
--- End diff --
Will do.
---
[GitHub] zookeeper pull request #593: [ZOOKEEPER-3111] Add socket buffer size option ...
Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/593#discussion_r211098029
--- Diff: src/java/main/org/apache/zookeeper/server/SocketUtil.java ---
@@ -0,0 +1,63 @@
+package org.apache.zookeeper.server;
+
+import java.net.ServerSocket;
+import java.net.Socket;
+import java.net.SocketAddress;
+import java.net.SocketException;
+import java.io.IOException;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class SocketUtil {
+
+ private static final Logger LOG = LoggerFactory.getLogger(SocketUtil.class);
+
+ public static final String NETWORK_BUFFER_SIZE = "zookeeper.NetworkBufferSize";
+ protected static int networkBufferSize;
+
+
+ static {
+ networkBufferSize = Integer.getInteger(NETWORK_BUFFER_SIZE, -1);
+ LOG.info("{} = {}", NETWORK_BUFFER_SIZE, networkBufferSize);
+ }
--- End diff --
add a unit: `byte` in the log is better?
---
[GitHub] zookeeper pull request #593: [ZOOKEEPER-3111] Add socket buffer size option ...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/593#discussion_r212070585
--- Diff: src/java/test/org/apache/zookeeper/server/SocketUtilTest.java ---
@@ -0,0 +1,46 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server;
+
+import java.net.Socket;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+public class SocketUtilTest {
+
+ @Test
+ public void testSetSocketBufferSize() throws Exception {
+ Socket s = new Socket();
+ int initSendBufferSize = s.getSendBufferSize();
+ int initReceiveBufferSize = s.getReceiveBufferSize();
+
+ // check buffer size won't be changed without set NETWORK_BUFFER_SIZE
+ SocketUtil.setSocketBufferSize("test", s);
+ Assert.assertEquals(initSendBufferSize, s.getSendBufferSize());
+ Assert.assertEquals(initReceiveBufferSize, s.getReceiveBufferSize());
+
+ // set NETWORK_BUFFER_SIZE
+ int networkBufferSize = 1024 * 1024;
+ SocketUtil.setNetworkBufferSize(networkBufferSize);
+ SocketUtil.setSocketBufferSize("test", s);
+ Assert.assertEquals(networkBufferSize, s.getSendBufferSize());
+ Assert.assertEquals(networkBufferSize, s.getReceiveBufferSize());
--- End diff --
It passed on my local test, I'll double check and fix if it's failing.
---
[GitHub] zookeeper issue #593: [ZOOKEEPER-3111] Add socket buffer size option to tune...
Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on the issue:
https://github.com/apache/zookeeper/pull/593
@lvfangmin
it will be a very useful option for tunning the network perfermance between leader and follower.
but you should give us some test evidence(smirk).
can we get the inclusion:
> Increasing the receive buffer size can increase the performance of network I/O for high-volume connection, while decreasing it can help reduce the backlog of incoming data.
or something else?what is the reasonable value you suggest?
---