You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by bensherman <gi...@git.apache.org> on 2017/06/05 23:40:24 UTC
[GitHub] zookeeper pull request #274: Zookeeper 1748: Add option for tcp keepalive
GitHub user bensherman opened a pull request:
https://github.com/apache/zookeeper/pull/274
Zookeeper 1748: Add option for tcp keepalive
As referenced in https://issues.apache.org/jira/browse/ZOOKEEPER-1748 and https://github.com/apache/zookeeper/pull/83, add the option to use keepalived on quorum connections. These connections are often idle and long-lived, thus tend to be silently dropped by intermediate networking infrastructure (AWS Security Groups' state tables, for example).
This PR adds the option to use the system's keepalive functionality when creating the socket for quorum connections.
It does not change existing behavior.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bensherman/zookeeper ZOOKEEPER-1748
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/274.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 #274
----
commit 0f7ece14df76cba5170c0442ebedec31cf6fb4b9
Author: Ben Sherman <bs...@axon.com>
Date: 2017-06-05T21:56:14Z
Added option to use tcp keep alives.
commit 820c628ea05fbdd5477083fa77db55a4f0e8812f
Author: Ben Sherman <bs...@axon.com>
Date: 2017-06-05T22:34:59Z
document tcpKeepAlive option
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #274: Zookeeper 1748: Add option for tcp keepalive
Posted by bensherman <gi...@git.apache.org>.
Github user bensherman commented on the issue:
https://github.com/apache/zookeeper/pull/274
I'll get the docs thing fixed right now, and I'll get the 3.5 PR done soon, it may take me some time as I don't have an environment setup to test 3.5 right now - bear with me! Should this also be applied to master or is there some magic there that keeps 3.5 and master in line?
I am also concerned that jenkins isn't passing its tests, should there be anything in my change that's causing it, please let me know.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #274: Zookeeper 1748: Add option for tcp keepalive
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/274#discussion_r120463819
--- Diff: docs/zookeeperAdmin.html ---
@@ -1446,7 +1446,17 @@ <h3 class="h4">Configuration Parameters</h3>
</pre>
</dd>
-
+<dt>
+<term>tcpKeepAlive</term>
+</dt>
+<dd>
+<p>(Java system property: <strong>zookeeper.tcpKeepAlive</strong>)</p>
--- End diff --
it would be great to provide a brief explanation for users why they may want to do this. without them needing to refer to the JIRA.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #274: Zookeeper 1748: Add option for tcp keepalive
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/274#discussion_r120471591
--- Diff: docs/zookeeperAdmin.html ---
@@ -1446,7 +1446,17 @@ <h3 class="h4">Configuration Parameters</h3>
</pre>
</dd>
-
+<dt>
+<term>tcpKeepAlive</term>
+</dt>
+<dd>
+<p>(Java system property: <strong>zookeeper.tcpKeepAlive</strong>)</p>
--- End diff --
Excellent explanation. +1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #274: Zookeeper 1748: Add option for tcp keepalive
Posted by alburthoffman <gi...@git.apache.org>.
Github user alburthoffman commented on the issue:
https://github.com/apache/zookeeper/pull/274
just curios why not using heartbeat/ping methods?
Long alive socket connections might be an issue for one box. Sometimes it can cause the process could not be killed(when we want to kill it).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #274: Zookeeper 1748: Add option for tcp keepalive
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/274#discussion_r120238723
--- Diff: docs/zookeeperAdmin.html ---
@@ -1446,7 +1446,17 @@ <h3 class="h4">Configuration Parameters</h3>
</pre>
</dd>
-
+<dt>
+<term>tcpKeepAlive</term>
+</dt>
+<dd>
+<p>(No Java system property)</p>
--- End diff --
A config option by default will automatically be added as a system property with its name as the suffix. In this case, the property will be "zookeeper.tcpKeepAlive", when the zoo.cfg file is parsed. See QuorumPeerConfig.java for more details. So this doc should be updated.
Having it as a system property is actually better as we can query the property when we need it, instead of change the signatures of various classes and pass it around.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #274: Zookeeper 1748: Add option for tcp keepalive
Posted by bensherman <gi...@git.apache.org>.
Github user bensherman commented on the issue:
https://github.com/apache/zookeeper/pull/274
I don't see the error that Jenkins is reporting back to this PR - what can I do to move this forward?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #274: Zookeeper 1748: Add option for tcp keepalive
Posted by bensherman <gi...@git.apache.org>.
Github user bensherman commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/274#discussion_r120470584
--- Diff: docs/zookeeperAdmin.html ---
@@ -1446,7 +1446,17 @@ <h3 class="h4">Configuration Parameters</h3>
</pre>
</dd>
-
+<dt>
+<term>tcpKeepAlive</term>
+</dt>
+<dd>
+<p>(Java system property: <strong>zookeeper.tcpKeepAlive</strong>)</p>
--- End diff --
Without digging into TCP options too deeply, I did the best I could. Thanks for the suggestion.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #274: Zookeeper 1748: Add option for tcp keepalive
Posted by bensherman <gi...@git.apache.org>.
Github user bensherman commented on the issue:
https://github.com/apache/zookeeper/pull/274
@hanm docs are fixed, 3.5 patch is in above referenced PR. Thanks again for your help!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #274: Zookeeper 1748: Add option for tcp keepalive
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/274
lgtm.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #274: Zookeeper 1748: Add option for tcp keepalive
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/274
All set, please close this PR @bensherman
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #274: Zookeeper 1748: Add option for tcp keepalive
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/274
@bensherman
Actually there is one issue in this PR that I missed until I reviewed another PR today - the documentation change should be applied to the document *source* instead of generated document artifacts. Just to copy and paste what I commented in another PR:
>>Please don't directly modify the document artifacts (html files, etc). The way the document is updated is by modifying the source of the docs, located at src/docs/src/documentation/content/xdocs. After modifying the source, please verify the generated document is correct locally by using apache forrest https://forrest.apache.org/. After verification please submit the document source change only - the compiled document artifacts (html files, etc) don't need to be submitted, because we will regenerate document in every release. Please check the commit history of https://github.com/apache/zookeeper/tree/master/src/docs/src/documentation/content/xdocs to get a concrete idea of how to make doc changes, it should be pretty straightforward.
In addition, as I commented on the JIRA, it would be great to have this feature in branch-3.5 and master also. Would you be willing to send PRs target those branches? I am happy to review and merge them.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #274: Zookeeper 1748: Add option for tcp keepalive
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/274#discussion_r120239287
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java ---
@@ -270,6 +271,8 @@ public void parseProperties(Properties zkProp)
quorumServicePrincipal = value;
} else if (key.equals("quorum.cnxn.threads.size")) {
quorumCnxnThreadsSize = Integer.parseInt(value);
+ } else if (key.equals("tcpKeepAlive")) {
+ tcpKeepAlive = Boolean.parseBoolean(value);
--- End diff --
As previously mentioned you can just let it fall through and query the system property when we need set the sock option.
If we do need parse it, then we need handle potential exceptions on illegal values here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #274: Zookeeper 1748: Add option for tcp keepalive
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/274
The Jekins failures are "expected" - the findbugs / doc warnings are known and the failed test is a known deprecated test. I'll merge this later.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #274: Zookeeper 1748: Add option for tcp keepalive
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/274#discussion_r120239746
--- Diff: docs/zookeeperAdmin.html ---
@@ -1446,7 +1446,17 @@ <h3 class="h4">Configuration Parameters</h3>
</pre>
</dd>
-
+<dt>
+<term>tcpKeepAlive</term>
+</dt>
+<dd>
+<p>(No Java system property)</p>
--- End diff --
Updated previous comment: I haven't seen your change to QuorumPeerConfig when commented on this. Though I think having a system property might be a more flexible option with less changes required. I don't think we have firm standard on which config option should have system properties and which ones should not, so I am ok either way.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #274: Zookeeper 1748: Add option for tcp keepalive
Posted by bensherman <gi...@git.apache.org>.
Github user bensherman commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/274#discussion_r120242644
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java ---
@@ -270,6 +271,8 @@ public void parseProperties(Properties zkProp)
quorumServicePrincipal = value;
} else if (key.equals("quorum.cnxn.threads.size")) {
quorumCnxnThreadsSize = Integer.parseInt(value);
+ } else if (key.equals("tcpKeepAlive")) {
+ tcpKeepAlive = Boolean.parseBoolean(value);
--- End diff --
Got it, making changes now!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #274: Zookeeper 1748: Add option for tcp keepalive
Posted by bensherman <gi...@git.apache.org>.
Github user bensherman commented on the issue:
https://github.com/apache/zookeeper/pull/274
Sorry for all the updates, my editor and github are having a spaces/tabs fight.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #274: Zookeeper 1748: Add option for tcp keepalive
Posted by bensherman <gi...@git.apache.org>.
Github user bensherman closed the pull request at:
https://github.com/apache/zookeeper/pull/274
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #274: Zookeeper 1748: Add option for tcp keepalive
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/274
@bensherman : Merged, please close this pull request.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---