[GitHub] zookeeper issue #99: ZOOKEEPER-2549 Add exception handling to sendResponse
Github user yufeldman commented on the issue: https://github.com/apache/zookeeper/pull/99 will do tomorrow. Thanks --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #123: ZOOKEEPER-1364: Add orthogonal fault injection mechani...
Github user yufeldman commented on the issue: https://github.com/apache/zookeeper/pull/123 @eribeiro I am not sure that critical bug is the place to introduce injection framework to Zookeeper. Can't it be done as a separate 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper pull request #99: ZOOKEEPER-2549 Add exception handling to sendRes...
Github user yufeldman commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/99#discussion_r92326091
--- Diff:
src/java/test/org/apache/zookeeper/server/ServerCxnExceptionsTest.java ---
@@ -0,0 +1,170 @@
+/**
+ * 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 org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.ZooKeeper;
+import org.apache.zookeeper.data.Stat;
+import org.apache.zookeeper.test.ClientBase;
+import org.junit.AfterClass;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.junit.Assert.fail;
+
+/**
+ * Unit tests to test different exceptions scenarious in sendResponse
+ */
+public class ServerCxnExceptionsTest extends ClientBase {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(ServerCxnExceptionsTest.class);
+
+ private String exceptionType;
+
+ private void NettySetup() throws Exception {
+System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY,
+ "org.apache.zookeeper.server.NettyServerCnxnFactory");
+System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN,
"org.apache.zookeeper.server.MockNettyServerCnxn");
+System.setProperty("exception.type", "NoException");
+ }
+
+ private void NIOSetup() throws Exception {
+System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY,
+ "org.apache.zookeeper.server.NIOServerCnxnFactory");
+System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN,
"org.apache.zookeeper.server.MockNIOServerCnxn");
+System.setProperty("exception.type", "NoException");
+ }
+
+ @AfterClass
+ public static void tearDownAfterClass() throws Exception {
+System.clearProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY);
+System.clearProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN);
+System.clearProperty("exception.type");
+ }
+
+ @Test (timeout = 6)
+ public void testNettyIOException() throws Exception {
+tearDown();
+NettySetup();
+testIOExceptionHelper();
+ }
+
+ @Test (timeout = 6)
+ public void testNIOIOException() throws Exception {
+tearDown();
+NIOSetup();
+testIOExceptionHelper();
+ }
+
+ private void testIOExceptionHelper() throws Exception {
+System.setProperty("exception.type", "IOException");
+super.setUp();
+final ZooKeeper zk = createClient();
+final String path = "/a";
+try {
+ // make sure zkclient works
+ zk.create(path, "test".getBytes(), ZooDefs.Ids.OPEN_ACL_UNSAFE,
+CreateMode.EPHEMERAL);
+ fail("Should not come here");
+ Stat stats = zk.exists(path, false);
+ if (stats != null) {
+int length = stats.getDataLength();
+ }
+} catch (KeeperException.ConnectionLossException cle) {
+ LOG.info("ConnectionLossException: {}", cle);
+} finally {
+ zk.close();
+}
+ }
+
+ @Test (timeout = 1)
+ public void testNettyNoException() throws Exception {
+tearDown();
+NettySetup();
+testZKNoExceptionHelper();
+ }
+
+ @Test (timeout = 1)
+ public void testNIONoException() throws Exception {
+tearDown();
+NIOSetup();
+testZKNoExceptionHelper();
+ }
+
+ private void testZKNoExceptionHelper() throws Exception {
+System.setProperty("exception.type", "NoException");
+super.setUp();
+final ZooKeeper zk = createClient();
+final String path = "/a";
+t
[GitHub] zookeeper pull request #99: ZOOKEEPER-2549 Add exception handling to sendRes...
Github user yufeldman commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/99#discussion_r92326078
--- Diff:
src/java/test/org/apache/zookeeper/server/ServerCxnExceptionsTest.java ---
@@ -0,0 +1,170 @@
+/**
+ * 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 org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.ZooKeeper;
+import org.apache.zookeeper.data.Stat;
+import org.apache.zookeeper.test.ClientBase;
+import org.junit.AfterClass;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.junit.Assert.fail;
+
+/**
+ * Unit tests to test different exceptions scenarious in sendResponse
+ */
+public class ServerCxnExceptionsTest extends ClientBase {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(ServerCxnExceptionsTest.class);
+
+ private String exceptionType;
+
+ private void NettySetup() throws Exception {
+System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY,
+ "org.apache.zookeeper.server.NettyServerCnxnFactory");
+System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN,
"org.apache.zookeeper.server.MockNettyServerCnxn");
+System.setProperty("exception.type", "NoException");
+ }
+
+ private void NIOSetup() throws Exception {
+System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY,
+ "org.apache.zookeeper.server.NIOServerCnxnFactory");
+System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN,
"org.apache.zookeeper.server.MockNIOServerCnxn");
+System.setProperty("exception.type", "NoException");
+ }
+
+ @AfterClass
+ public static void tearDownAfterClass() throws Exception {
+System.clearProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY);
+System.clearProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN);
+System.clearProperty("exception.type");
+ }
+
+ @Test (timeout = 6)
+ public void testNettyIOException() throws Exception {
+tearDown();
+NettySetup();
+testIOExceptionHelper();
+ }
+
+ @Test (timeout = 6)
+ public void testNIOIOException() throws Exception {
+tearDown();
+NIOSetup();
+testIOExceptionHelper();
+ }
+
+ private void testIOExceptionHelper() throws Exception {
+System.setProperty("exception.type", "IOException");
+super.setUp();
+final ZooKeeper zk = createClient();
+final String path = "/a";
+try {
+ // make sure zkclient works
+ zk.create(path, "test".getBytes(), ZooDefs.Ids.OPEN_ACL_UNSAFE,
+CreateMode.EPHEMERAL);
+ fail("Should not come here");
+ Stat stats = zk.exists(path, false);
+ if (stats != null) {
+int length = stats.getDataLength();
+ }
+} catch (KeeperException.ConnectionLossException cle) {
+ LOG.info("ConnectionLossException: {}", cle);
+} finally {
+ zk.close();
+}
+ }
+
+ @Test (timeout = 1)
+ public void testNettyNoException() throws Exception {
+tearDown();
+NettySetup();
+testZKNoExceptionHelper();
+ }
+
+ @Test (timeout = 1)
+ public void testNIONoException() throws Exception {
+tearDown();
+NIOSetup();
+testZKNoExceptionHelper();
+ }
+
+ private void testZKNoExceptionHelper() throws Exception {
+System.setProperty("exception.type", "NoException");
+super.setUp();
+final ZooKeeper zk = createClient();
+final String path = "/a";
+t
[GitHub] zookeeper pull request #99: ZOOKEEPER-2549 Add exception handling to sendRes...
Github user yufeldman commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/99#discussion_r92326026
--- Diff:
src/java/test/org/apache/zookeeper/server/ServerCxnExceptionsTest.java ---
@@ -0,0 +1,170 @@
+/**
+ * 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 org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.ZooKeeper;
+import org.apache.zookeeper.data.Stat;
+import org.apache.zookeeper.test.ClientBase;
+import org.junit.AfterClass;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.junit.Assert.fail;
+
+/**
+ * Unit tests to test different exceptions scenarious in sendResponse
+ */
+public class ServerCxnExceptionsTest extends ClientBase {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(ServerCxnExceptionsTest.class);
+
+ private String exceptionType;
+
+ private void NettySetup() throws Exception {
+System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY,
+ "org.apache.zookeeper.server.NettyServerCnxnFactory");
+System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN,
"org.apache.zookeeper.server.MockNettyServerCnxn");
+System.setProperty("exception.type", "NoException");
+ }
+
+ private void NIOSetup() throws Exception {
+System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY,
+ "org.apache.zookeeper.server.NIOServerCnxnFactory");
+System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN,
"org.apache.zookeeper.server.MockNIOServerCnxn");
+System.setProperty("exception.type", "NoException");
+ }
+
+ @AfterClass
+ public static void tearDownAfterClass() throws Exception {
+System.clearProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY);
+System.clearProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN);
+System.clearProperty("exception.type");
+ }
+
+ @Test (timeout = 6)
+ public void testNettyIOException() throws Exception {
+tearDown();
+NettySetup();
+testIOExceptionHelper();
+ }
+
+ @Test (timeout = 6)
+ public void testNIOIOException() throws Exception {
+tearDown();
+NIOSetup();
+testIOExceptionHelper();
+ }
+
+ private void testIOExceptionHelper() throws Exception {
+System.setProperty("exception.type", "IOException");
+super.setUp();
+final ZooKeeper zk = createClient();
+final String path = "/a";
+try {
+ // make sure zkclient works
+ zk.create(path, "test".getBytes(), ZooDefs.Ids.OPEN_ACL_UNSAFE,
+CreateMode.EPHEMERAL);
+ fail("Should not come here");
+ Stat stats = zk.exists(path, false);
+ if (stats != null) {
+int length = stats.getDataLength();
+ }
+} catch (KeeperException.ConnectionLossException cle) {
+ LOG.info("ConnectionLossException: {}", cle);
+} finally {
+ zk.close();
+}
+ }
+
+ @Test (timeout = 1)
+ public void testNettyNoException() throws Exception {
+tearDown();
+NettySetup();
+testZKNoExceptionHelper();
+ }
+
+ @Test (timeout = 1)
--- End diff --
why to keep it more than needed? 30 sec. is standard for ZK timeout, so 1
min would be overkill.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #99: ZOOKEEPER-2549 Add exception handling to sendRes...
Github user yufeldman commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/99#discussion_r90963807
--- Diff: src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java ---
@@ -71,7 +71,7 @@
NettyServerCnxnFactory factory;
boolean initialized;
-NettyServerCnxn(Channel channel, ZooKeeperServer zks,
NettyServerCnxnFactory factory) {
+public NettyServerCnxn(Channel channel, ZooKeeperServer zks,
NettyServerCnxnFactory factory) {
--- End diff --
@hanm - thank you for all the reviews and feedback. Really appreciate 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #99: ZOOKEEPER-2549 Add exception handling to sendRes...
Github user yufeldman commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/99#discussion_r90963562
--- Diff: src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java ---
@@ -71,7 +71,7 @@
NettyServerCnxnFactory factory;
boolean initialized;
-NettyServerCnxn(Channel channel, ZooKeeperServer zks,
NettyServerCnxnFactory factory) {
+public NettyServerCnxn(Channel channel, ZooKeeperServer zks,
NettyServerCnxnFactory factory) {
--- End diff --
Interesting. For me it actually is:
2016-12-05 11:11:02,523 [myid:] - INFO [Time-limited
test:JUnit4ZKTestRunner$LoggedInvokeMethod@98] - TEST METHOD FAILED
testNettyRunTimeException
**java.io.IOException: Couldn't instantiate
org.apache.zookeeper.server.NettyServerCnxnFactory
at
org.apache.zookeeper.server.ServerCnxnFactory.createFactory(ServerCnxnFactory.java:141)
at
org.apache.zookeeper.server.ServerCnxnFactory.createFactory(ServerCnxnFactory.java:157)
at
org.apache.zookeeper.server.ServerCnxnFactory.createFactory(ServerCnxnFactory.java:151)**
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #99: ZOOKEEPER-2549 Add exception handling to sendRes...
Github user yufeldman commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/99#discussion_r90953900
--- Diff: src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java ---
@@ -71,7 +71,7 @@
NettyServerCnxnFactory factory;
boolean initialized;
-NettyServerCnxn(Channel channel, ZooKeeperServer zks,
NettyServerCnxnFactory factory) {
+public NettyServerCnxn(Channel channel, ZooKeeperServer zks,
NettyServerCnxnFactory factory) {
--- End diff --
@hanm - >>> so all tests fall back to create NIOServerCnxnx instead
It is not the case, they don't fall back to NIO - they fail.
Here is printout form running tests with both Netty and MockNettyServerCnxn
not having public ctor:
[junit] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed:
0.159 sec
** [junit] Test org.apache.zookeeper.server.NettyServerCnxnTest FAILED **
[junit] Running org.apache.zookeeper.server.PrepRequestProcessorTest
[junit] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed:
0.279 sec
[junit] Running org.apache.zookeeper.server.PurgeTxnTest
[junit] Tests run: 8, Failures: 0, Errors: 0, Skipped: 0, Time elapsed:
2.431 sec
[junit] Running org.apache.zookeeper.server.ReferenceCountedACLCacheTest
[junit] Tests run: 10, Failures: 0, Errors: 0, Skipped: 0, Time
elapsed: 0.089 sec
[junit] Running org.apache.zookeeper.server.SerializationPerfTest
[junit] Tests run: 8, Failures: 0, Errors: 0, Skipped: 0, Time elapsed:
1.114 sec
[junit] Running org.apache.zookeeper.server.ServerCxnExceptionsTest
[junit] Tests run: 6, Failures: 0, Errors: 6, Skipped: 0, Time elapsed:
0.284 sec
**[junit] Test org.apache.zookeeper.server.ServerCxnExceptionsTest
FAILED **
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #99: ZOOKEEPER-2549 Add exception handling to sendRes...
Github user yufeldman commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/99#discussion_r90935413
--- Diff: src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java ---
@@ -71,7 +71,7 @@
NettyServerCnxnFactory factory;
boolean initialized;
-NettyServerCnxn(Channel channel, ZooKeeperServer zks,
NettyServerCnxnFactory factory) {
+public NettyServerCnxn(Channel channel, ZooKeeperServer zks,
NettyServerCnxnFactory factory) {
--- End diff --
@hanm - there is nothing wrong with the tests, they do run fine. They do
not use ctor from NettyServerCnxn, but from MockNettyServerCnxn - that has
public ctor.
You can make MockNettyServerCnxn not public and you will have the same
issue. And BTW
org.apache.zookeeper.server.NettyServerCnxnTest fails with not public ctor
in NettyServerCnxn
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #99: ZOOKEEPER-2549 Add exception handling to sendRes...
Github user yufeldman commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/99#discussion_r90783210
--- Diff: src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java ---
@@ -71,7 +71,7 @@
NettyServerCnxnFactory factory;
boolean initialized;
-NettyServerCnxn(Channel channel, ZooKeeperServer zks,
NettyServerCnxnFactory factory) {
+public NettyServerCnxn(Channel channel, ZooKeeperServer zks,
NettyServerCnxnFactory factory) {
--- End diff --
Take my words back. I need it to be public as I use reflection to create it
in NettyServerCnxnFactory and if it is not I would have to do couple of more
steps during init to set access to public which is unnecessary
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #99: ZOOKEEPER-2549 Add exception handling to sendRes...
Github user yufeldman commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/99#discussion_r90782724
--- Diff: src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java ---
@@ -716,7 +716,12 @@ public void process(WatchedEvent event) {
// Convert WatchedEvent to a type that can be sent over the wire
WatcherEvent e = event.getWrapper();
-sendResponse(h, e, "notification");
+try {
+sendResponse(h, e, "notification");
+} catch (IOException ex) {
+LOG.debug("Problem sending to " + getRemoteSocketAddress(),
ex);
+close();
--- End diff --
Will remove close() from catch block
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #99: ZOOKEEPER-2549 Add exception handling to sendRes...
Github user yufeldman commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/99#discussion_r90782565
--- Diff: src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java ---
@@ -71,7 +71,7 @@
NettyServerCnxnFactory factory;
boolean initialized;
-NettyServerCnxn(Channel channel, ZooKeeperServer zks,
NettyServerCnxnFactory factory) {
+public NettyServerCnxn(Channel channel, ZooKeeperServer zks,
NettyServerCnxnFactory factory) {
--- End diff --
I think I did this to match NIOServerCnxn constructor. It can be kept
package level, as my test class is in the same package namespace. I can change
it back, but it will be inconsistent with NIO
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #99: ZOOKEEPER-2549 Add exception handling to sendRes...
Github user yufeldman commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/99#discussion_r90553827
--- Diff: src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java ---
@@ -165,31 +163,35 @@ public void process(WatchedEvent event) {
@Override
public void sendResponse(ReplyHeader h, Record r, String tag)
throws IOException {
-if (!channel.isOpen()) {
-return;
-}
-ByteArrayOutputStream baos = new ByteArrayOutputStream();
-// Make space for length
-BinaryOutputArchive bos = BinaryOutputArchive.getArchive(baos);
try {
-baos.write(fourBytes);
-bos.writeRecord(h, "header");
-if (r != null) {
-bos.writeRecord(r, tag);
+if (!channel.isOpen()) {
+return;
}
-baos.close();
-} catch (IOException e) {
-LOG.error("Error serializing response");
-}
-byte b[] = baos.toByteArray();
-ByteBuffer bb = ByteBuffer.wrap(b);
-bb.putInt(b.length - 4).rewind();
-sendBuffer(bb);
-if (h.getXid() > 0) {
-// zks cannot be null otherwise we would not have gotten here!
-if
(!zkServer.shouldThrottle(outstandingCount.decrementAndGet())) {
-enableRecv();
+ByteArrayOutputStream baos = new ByteArrayOutputStream();
+// Make space for length
+BinaryOutputArchive bos = BinaryOutputArchive.getArchive(baos);
+try {
+baos.write(fourBytes);
+bos.writeRecord(h, "header");
+if (r != null) {
+bos.writeRecord(r, tag);
+}
+baos.close();
+} catch (IOException e) {
--- End diff --
I did not modify this code - it was like that before, but potentially - yes
it makes sense to rethrow
I would say there are multiple places I came across where exceptions are
swallowed
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #99: ZOOKEEPER-2549 Add exception handling to sendResponse
Github user yufeldman commented on the issue: https://github.com/apache/zookeeper/pull/99 Addressed review comments --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper pull request #99: ZOOKEEPER-2549 Add exception handling to sendRes...
Github user yufeldman commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/99#discussion_r86691087
--- Diff: src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java ---
@@ -716,7 +716,12 @@ public void process(WatchedEvent event) {
// Convert WatchedEvent to a type that can be sent over the wire
WatcherEvent e = event.getWrapper();
-sendResponse(h, e, "notification");
+try {
+sendResponse(h, e, "notification");
+} catch (IOException ex) {
+LOG.debug("Problem sending to " + getRemoteSocketAddress(),
ex);
+close();
--- End diff --
I agree that it must be closing somewhere - just need to trace where
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #99: ZOOKEEPER-2549 Add exception handling to sendRes...
Github user yufeldman commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/99#discussion_r86673997
--- Diff: src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java ---
@@ -716,7 +716,12 @@ public void process(WatchedEvent event) {
// Convert WatchedEvent to a type that can be sent over the wire
WatcherEvent e = event.getWrapper();
-sendResponse(h, e, "notification");
+try {
+sendResponse(h, e, "notification");
+} catch (IOException ex) {
+LOG.debug("Problem sending to " + getRemoteSocketAddress(),
ex);
+close();
--- End diff --
It was not closing (I think) before as exception was swallowed since
sendResponse in NIOServerCnxn was not throwing IOException
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #99: ZOOKEEPER-2549 Add exception handling to sendRes...
Github user yufeldman commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/99#discussion_r86674430
--- Diff: src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java ---
@@ -716,7 +716,12 @@ public void process(WatchedEvent event) {
// Convert WatchedEvent to a type that can be sent over the wire
WatcherEvent e = event.getWrapper();
-sendResponse(h, e, "notification");
+try {
+sendResponse(h, e, "notification");
+} catch (IOException ex) {
+LOG.debug("Problem sending to " + getRemoteSocketAddress(),
ex);
--- End diff --
Any suggestion here? Not to getRemoteSocketAddress() at all?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #99: ZOOKEEPER-2549 Add exception handling to sendRes...
Github user yufeldman commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/99#discussion_r86674332
--- Diff:
src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java ---
@@ -842,7 +868,26 @@ private void addCnxn(NIOServerCnxn cnxn) {
protected NIOServerCnxn createConnection(SocketChannel sock,
SelectionKey sk, SelectorThread selectorThread) throws
IOException {
-return new NIOServerCnxn(zkServer, sock, sk, this, selectorThread);
+
+NIOServerCnxn cnxn = null;
+
+if (serverCnxnClassCtr != null) {
--- End diff --
Actually I tend to come back to double instantiation - as it will be used
only in UnitTests (serverCnxnClassCtr != null), while manipulation with
constructor (invocation of newInstance) instead of directly creating instance
of NIOServerCnxn will be really a hot path
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #99: ZOOKEEPER-2549 Add exception handling to sendRes...
GitHub user yufeldman opened a pull request: https://github.com/apache/zookeeper/pull/99 ZOOKEEPER-2549 Add exception handling to sendResponse Fix for: As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread Same changes done for NIOServerCnxn You can merge this pull request into a Git repository by running: $ git pull https://github.com/yufeldman/zookeeper master2549 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/99.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 #99 commit 18270e28f3e60a03422641dd0ee2ccaad6a3b096 Author: Yuliya Feldman <yfeld...@maprtech.com> Date: 2016-09-03T05:18:30Z ZOOKEEPER-2549 Add exception handling to sendResponse --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
