You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by hanm <gi...@git.apache.org> on 2017/01/25 04:36:40 UTC
[GitHub] zookeeper pull request #156: ZOOKEEPER-2044:CancelledKeyException in zookeep...
GitHub user hanm opened a pull request:
https://github.com/apache/zookeeper/pull/156
ZOOKEEPER-2044:CancelledKeyException in zookeeper 3.4.5.
Patch from Germ�n Blanco, test case from Flavio.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/hanm/zookeeper ZOOKEEPER-2044
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/156.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 #156
----
commit a220d95f2aefd0bf6f72d2757a87dc5480f9891e
Author: Michael Han <ha...@apache.org>
Date: 2017-01-25T04:35:05Z
ZOOKEEPER-2044:CancelledKeyException in zookeeper 3.4.5.
Patch from Germ�n Blanco, test case from Flavio.
----
---
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 #156: ZOOKEEPER-2044:CancelledKeyException in zookeep...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/156#discussion_r97716727
--- Diff: src/java/test/org/apache/zookeeper/server/NIOServerCnxnTest.java ---
@@ -18,6 +18,12 @@
package org.apache.zookeeper.server;
import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.channels.CancelledKeyException;
+import java.nio.channels.SelectionKey;
--- End diff --
Please remove unused imports.
---
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 #156: ZOOKEEPER-2044:CancelledKeyException in zookeep...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/156#discussion_r97717262
--- Diff: src/java/test/org/apache/zookeeper/server/NIOServerCnxnTest.java ---
@@ -68,4 +74,38 @@ public void testOperationsAfterCnxnClose() throws IOException,
zk.close();
}
}
+
+ /**
+ * Mock extension of NIOServerCnxn to test for
+ * CancelledKeyException (ZOOKEEPER-2044).
+ */
+ class MockNIOServerCnxn extends NIOServerCnxn {
+ public MockNIOServerCnxn( NIOServerCnxn cnxn )
+ throws IOException {
+ super( cnxn.zkServer, cnxn.sock, cnxn.sk, cnxn.factory );
--- End diff --
This line has extra spaces and I could see formatting issues in many places. Please remove unwanted spaces and correct 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 issue #156: ZOOKEEPER-2044:CancelledKeyException in zookeeper 3.4....
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/156
@rakeshadr thanks for review, patch updated.
---
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 #156: ZOOKEEPER-2044:CancelledKeyException in zookeep...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/156#discussion_r98146418
--- Diff: src/java/test/org/apache/zookeeper/server/NIOServerCnxnTest.java ---
@@ -68,4 +69,41 @@ public void testOperationsAfterCnxnClose() throws IOException,
zk.close();
}
}
+
+ /**
+ * Mock extension of NIOServerCnxn to test for
+ * CancelledKeyException (ZOOKEEPER-2044).
+ */
+ private static class MockNIOServerCnxn extends NIOServerCnxn {
+ public MockNIOServerCnxn(NIOServerCnxn cnxn)
+ throws IOException {
+ super(cnxn.zkServer, cnxn.sock, cnxn.sk, cnxn.factory);
+ }
+
+ public void mockSendBuffer(ByteBuffer bb) throws Exception {
+ super.internalSendBuffer(bb);
+ }
+ }
+
+ @Test(timeout = 30000)
+ public void testValidSelectionKey() throws Exception {
+ int oldTimeout = ClientBase.CONNECTION_TIMEOUT;
+ ClientBase.CONNECTION_TIMEOUT = 3000;
+ final ZooKeeper zk = createClient();
--- End diff --
Thanks @hanm for the analysis and fixing it. Instead of directly changing the static value, how about simplifying the ZooKeeper client creation like below,
``final ZooKeeper zk = createZKClient(hostPort, 3000);``
---
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 #156: ZOOKEEPER-2044:CancelledKeyException in zookeeper 3.4....
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/156
Closing pull request now it's merged.
---
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 #156: ZOOKEEPER-2044:CancelledKeyException in zookeep...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/156#discussion_r97716640
--- Diff: src/java/test/org/apache/zookeeper/server/NIOServerCnxnTest.java ---
@@ -68,4 +74,38 @@ public void testOperationsAfterCnxnClose() throws IOException,
zk.close();
}
}
+
+ /**
+ * Mock extension of NIOServerCnxn to test for
+ * CancelledKeyException (ZOOKEEPER-2044).
+ */
+ class MockNIOServerCnxn extends NIOServerCnxn {
+ public MockNIOServerCnxn( NIOServerCnxn cnxn )
+ throws IOException {
+ super( cnxn.zkServer, cnxn.sock, cnxn.sk, cnxn.factory );
+ }
+
+ public void mockSendBuffer(ByteBuffer bb) throws Exception {
+ super.internalSendBuffer( bb );
+ }
+ }
+
+ @Test(timeout = 30000)
+ public void testValidSelectionKey() throws Exception {
+ final ZooKeeper zk = createClient();
+ try {
+ Iterable<ServerCnxn> connections = serverFactory.getConnections();
+ for (ServerCnxn serverCnxn : connections) {
+ MockNIOServerCnxn mock = new MockNIOServerCnxn((NIOServerCnxn) serverCnxn);
+ // Cancel key
+ ((NIOServerCnxn) serverCnxn).sock.keyFor(((NIOServerCnxnFactory) serverFactory).selector).cancel();;
+ mock.mockSendBuffer( ByteBuffer.allocate(8) );
+ }
+ } catch (CancelledKeyException e) {
+ e.printStackTrace();
--- End diff --
Instead of printStackTrace, can we use LOG.error("Exception while sending bytes!", e);
---
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 #156: ZOOKEEPER-2044:CancelledKeyException in zookeeper 3.4....
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on the issue:
https://github.com/apache/zookeeper/pull/156
Thanks @hanm , I've committed the changes to branch-3.4
---
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 #156: ZOOKEEPER-2044:CancelledKeyException in zookeep...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/156#discussion_r98120457
--- Diff: src/java/test/org/apache/zookeeper/server/NIOServerCnxnTest.java ---
@@ -68,4 +69,38 @@ public void testOperationsAfterCnxnClose() throws IOException,
zk.close();
}
}
+
+ /**
+ * Mock extension of NIOServerCnxn to test for
+ * CancelledKeyException (ZOOKEEPER-2044).
+ */
+ private static class MockNIOServerCnxn extends NIOServerCnxn {
+ public MockNIOServerCnxn(NIOServerCnxn cnxn)
+ throws IOException {
+ super(cnxn.zkServer, cnxn.sock, cnxn.sk, cnxn.factory);
+ }
+
+ public void mockSendBuffer(ByteBuffer bb) throws Exception {
+ super.internalSendBuffer(bb);
+ }
+ }
+
+ @Test(timeout = 30000)
+ public void testValidSelectionKey() throws Exception {
+ final ZooKeeper zk = createClient();
+ try {
+ Iterable<ServerCnxn> connections = serverFactory.getConnections();
+ for (ServerCnxn serverCnxn : connections) {
+ MockNIOServerCnxn mock = new MockNIOServerCnxn((NIOServerCnxn) serverCnxn);
+ // Cancel key
+ ((NIOServerCnxn) serverCnxn).sock.keyFor(((NIOServerCnxnFactory) serverFactory).selector).cancel();;
+ mock.mockSendBuffer(ByteBuffer.allocate(8));
+ }
+ } catch (CancelledKeyException e) {
+ LOG.error("Exception while sending bytes!", e);
+ Assert.fail(e.toString());
+ } finally {
+ zk.close();
--- End diff --
@rakeshadr Good observation on the long running of the test. This is definitely something we should fix. The actual delay indeed happens at client close and the root cause is session timeout: when a client closing itself it sends a request to server, and this request packet will stuck forever in our case because server has canceled the selector; so client session will expire eventually and by default, the timeout value between client / server is set as 30 sec and 2/3 about it - which is 20 sec is exactly what it would cost for a heart beat to fail. I fixed this by adjusting the timeout value to 3 sec instead just for this single test. PTAL.
---
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 #156: ZOOKEEPER-2044:CancelledKeyException in zookeep...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/156#discussion_r97935895
--- Diff: src/java/test/org/apache/zookeeper/server/NIOServerCnxnTest.java ---
@@ -68,4 +69,38 @@ public void testOperationsAfterCnxnClose() throws IOException,
zk.close();
}
}
+
+ /**
+ * Mock extension of NIOServerCnxn to test for
+ * CancelledKeyException (ZOOKEEPER-2044).
+ */
+ private static class MockNIOServerCnxn extends NIOServerCnxn {
+ public MockNIOServerCnxn(NIOServerCnxn cnxn)
+ throws IOException {
+ super(cnxn.zkServer, cnxn.sock, cnxn.sk, cnxn.factory);
+ }
+
+ public void mockSendBuffer(ByteBuffer bb) throws Exception {
+ super.internalSendBuffer(bb);
+ }
+ }
+
+ @Test(timeout = 30000)
+ public void testValidSelectionKey() throws Exception {
+ final ZooKeeper zk = createClient();
+ try {
+ Iterable<ServerCnxn> connections = serverFactory.getConnections();
+ for (ServerCnxn serverCnxn : connections) {
+ MockNIOServerCnxn mock = new MockNIOServerCnxn((NIOServerCnxn) serverCnxn);
+ // Cancel key
+ ((NIOServerCnxn) serverCnxn).sock.keyFor(((NIOServerCnxnFactory) serverFactory).selector).cancel();;
+ mock.mockSendBuffer(ByteBuffer.allocate(8));
+ }
+ } catch (CancelledKeyException e) {
+ LOG.error("Exception while sending bytes!", e);
+ Assert.fail(e.toString());
+ } finally {
+ zk.close();
--- End diff --
Thanks @hanm , the latest changes looks good to me. I will leave this pull request for one more day and if there is no more comments then will commit this tomorrow.
I've an observation while running the unit test case, "zk.close()" call is taking 15-20secs to complete. I think, this delay is due to the previously submitted mocked buffer. Could you please run test in your env and let me know your observation.
---
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 #156: ZOOKEEPER-2044:CancelledKeyException in zookeep...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/156#discussion_r97716986
--- Diff: src/java/test/org/apache/zookeeper/server/NIOServerCnxnTest.java ---
@@ -68,4 +74,38 @@ public void testOperationsAfterCnxnClose() throws IOException,
zk.close();
}
}
+
+ /**
+ * Mock extension of NIOServerCnxn to test for
+ * CancelledKeyException (ZOOKEEPER-2044).
+ */
+ class MockNIOServerCnxn extends NIOServerCnxn {
--- End diff --
Please make MockNIOServerCnxn class ''private static''
---
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 #156: ZOOKEEPER-2044:CancelledKeyException in zookeep...
Posted by hanm <gi...@git.apache.org>.
Github user hanm closed the pull request at:
https://github.com/apache/zookeeper/pull/156
---
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 #156: ZOOKEEPER-2044:CancelledKeyException in zookeeper 3.4....
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on the issue:
https://github.com/apache/zookeeper/pull/156
@rakeshadr @hanm For whatever reason this PR was not closed. Could you close it Michael?
---
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 #156: ZOOKEEPER-2044:CancelledKeyException in zookeep...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/156#discussion_r98244348
--- Diff: src/java/test/org/apache/zookeeper/server/NIOServerCnxnTest.java ---
@@ -68,4 +69,41 @@ public void testOperationsAfterCnxnClose() throws IOException,
zk.close();
}
}
+
+ /**
+ * Mock extension of NIOServerCnxn to test for
+ * CancelledKeyException (ZOOKEEPER-2044).
+ */
+ private static class MockNIOServerCnxn extends NIOServerCnxn {
+ public MockNIOServerCnxn(NIOServerCnxn cnxn)
+ throws IOException {
+ super(cnxn.zkServer, cnxn.sock, cnxn.sk, cnxn.factory);
+ }
+
+ public void mockSendBuffer(ByteBuffer bb) throws Exception {
+ super.internalSendBuffer(bb);
+ }
+ }
+
+ @Test(timeout = 30000)
+ public void testValidSelectionKey() throws Exception {
+ int oldTimeout = ClientBase.CONNECTION_TIMEOUT;
+ ClientBase.CONNECTION_TIMEOUT = 3000;
+ final ZooKeeper zk = createClient();
--- End diff --
@rakeshadr done. I was looking for something similar in createClient - did not find it so ended up using the old approach.
---
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.
---