[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15772287#comment-15772287
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user yufeldman commented on the issue:

https://github.com/apache/zookeeper/pull/99
  
will do tomorrow. Thanks


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549-5.patch, 
> ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15772059#comment-15772059
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user hanm commented on the issue:

https://github.com/apache/zookeeper/pull/99
  
@yufeldman could you please fix some coding style issues pointed out by 
@eribeiro (4 spaces indentation, extra white space, etc)? I'll merge your patch 
once those comments are addressed. Thanks.


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549-5.patch, 
> ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15747369#comment-15747369
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

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();
+  

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15747370#comment-15747370
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

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();
+  

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15747363#comment-15747363
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

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.


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> 

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15746741#comment-15746741
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r92297258
  
--- 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();
+   

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15746739#comment-15746739
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r92282690
  
--- 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);
--- End diff --

It is a good practice (even tough I dunno we use consistently on ZK 
codebase) that we use an idiom like the one below:

```
  private String previousProperty = null;

  @Before
  public void setUp() {
 previous = System.getProperty(MY_PROPERTY_NAME);
 System.setProperty(MY_PROPERTY_NAME, "new_value");
  }

 @After
 public void tearDown() {
  System.setProperty(MY_PROPERTY_NAME, previousValue);
 }
```

This preserves the previous value of the ``System.property()``, **afaik**.


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549-5.patch, 
> ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15746751#comment-15746751
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r92297351
  
--- 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();
+   

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15746746#comment-15746746
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r92297567
  
--- 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();
+   

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15746731#comment-15746731
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r92271380
  
--- 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);
--- End diff --

1. Why we need to log the message here?

2. Why we need to serialize ``cle``? Would it be ``cle.getMessage()``?

3. Wouldn't be better to let it throw the 
``KeeperException.ConnectionLossException`` and make the test catch it with 
``expected`` as below?

```
@Test(timeout = 6, expected = KeeperException.ConnectionLossException)
```


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> 

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15746732#comment-15746732
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r92268597
  
--- 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);
--- End diff --

I didn't get why we need the lines 87-90, because ``fail()`` throws an 
``AssertionError`` that will interrupt the processing flow, so those lines are 
effectively unreachable, right? There's should be nothing more after the 
`fail()`, I **guess**.

Also, I would suggest to put a more meaningful message, something along the 
lines of ``sendResponse should have thrown IOException and failed this test.``


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue 

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15746743#comment-15746743
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r92266393
  
--- 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;
--- End diff --

``exceptionType`` is never used!


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549-5.patch, 
> ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15746740#comment-15746740
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r92266304
  
--- 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
--- End diff --

typo: ``scenarios``


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549-5.patch, 
> ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15746748#comment-15746748
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r92297538
  
--- 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();
+   

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15746738#comment-15746738
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r92274772
  
--- 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)
--- End diff --

We usually put 1 minute (6 ms) timeout, why decrease here? If **I** 
understood correctly, test timeouts define a maximum execution threshold, but 
they won't execute faster if we decrease, right?


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: 

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15746747#comment-15746747
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r92296352
  
--- 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 {
--- End diff --

nit: it's nice to have an ordering on the definition of methods: public, 
protected, private. That is, it would be nice to move the private methods to 
the end of the file.


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549-5.patch, 
> ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() 

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15746749#comment-15746749
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r92298117
  
--- Diff: src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java ---
@@ -694,7 +690,7 @@ public void sendResponse(ReplyHeader h, Record r, 
String tag) {
 }
 }
  } catch(Exception e) {
-LOG.warn("Unexpected exception. Destruction averted.", e);
+throw new IOException(e);
--- End diff --

Just a suggestion: wdyt about bubbling up a more custom message with the 
IOException instead of just encapsulate the Exception? I mean, something like:

``
throw new IOException("sendMessage exception: blah blah", e);
``


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549-5.patch, 
> ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15746750#comment-15746750
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r92296064
  
--- Diff: 
src/java/test/org/apache/zookeeper/server/MockNettyServerCnxn.java ---
@@ -0,0 +1,65 @@
+/**
+ * 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.jute.Record;
+import org.apache.zookeeper.proto.ReplyHeader;
+import org.jboss.netty.channel.Channel;
+
+import java.io.IOException;
+import java.lang.reflect.Field;
+import java.lang.reflect.Modifier;
+
+/**
+ * Helper class to test different scenarios in NettyServerCnxn
+ */
+public class MockNettyServerCnxn extends NettyServerCnxn {
--- End diff --

In this file, tab is indented with 2 spaces while the rest of the ZooKeeper 
files use 4 spaces (I only discovered this 'cause my IDE complained about it).


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549-5.patch, 
> ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15746745#comment-15746745
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r92296453
  
--- 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();
--- End diff --

Aren't we missing a ``try-catch`` around ``zk.close()`` with an explicit 
comment to ignore any error message?


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15746735#comment-15746735
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r92269683
  
--- 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 {
--- End diff --

Method name doesn't follow camel case convention (``nioSetup`` instead of 
``NIOSetup``).


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549-5.patch, 
> ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15746737#comment-15746737
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r92283709
  
--- 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();
+   

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15746744#comment-15746744
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r92297692
  
--- 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();
+   

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15746733#comment-15746733
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r92264074
  
--- 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 --

nit: I would use a modern debug format:

``
LOG.debug("Problem sending to {}", getRemoteSocketAddress(), ex);
``


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549-5.patch, 
> ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15746742#comment-15746742
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r92269582
  
--- 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 {
--- End diff --

Method name doesn't follow camel case convention (``nettySetup`` instead of 
``NettySetup``).


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549-5.patch, 
> ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15746736#comment-15746736
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r92274850
  
--- 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 --

Same as above: why decrease the timeout?


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15746734#comment-15746734
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user eribeiro commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r92296029
  
--- 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 {
--- End diff --

In this file, tab is indented with 2 spaces while the rest of the ZooKeeper 
files use 4 spaces (I only discovered this 'cause my IDE complained about it).


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549-5.patch, 
> ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15746729#comment-15746729
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user eribeiro commented on the issue:

https://github.com/apache/zookeeper/pull/99
  
I like this patch, but I think the whole reflection/mock thing is kind of 
reinventing a fault injection inside the test classes. If so, why not use a 
production ready framework as Byteman? I wrote a PR that strips the boilerplate 
stuff while leaving the feature of this PR: 
https://github.com/apache/zookeeper/pull/123 

Still a PoC, so any suggestions are welcome. :)


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549-5.patch, 
> ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15723475#comment-15723475
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

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.


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549-5.patch, 
> ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15723470#comment-15723470
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

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)**



> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549-5.patch, 
> ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15723437#comment-15723437
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r90960756
  
--- 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 --

>> It is not the case, they don't fall back to NIO - they fail.
The fallback I was referring to is 
https://github.com/apache/zookeeper/blob/master/src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java#L130.
 It gets hit when public was removed from NIOServerCnxn for Netty* tests. An 
example call stack (note that a Netty test complaining about 
NIOServerCnxnFactory):
` INFO  [main:ZKTestCase$1@70] - FAILED testNettyRunTimeException
java.io.IOException: Couldn't instantiate 
org.apache.zookeeper.server.NIOServerCnxnFactory
>---at 
org.apache.zookeeper.server.ServerCnxnFactory.createFactory(ServerCnxnFactory.java:142)
>---at 
org.apache.zookeeper.server.ServerCnxnFactory.createFactory(ServerCnxnFactory.java:158)
>---at 
org.apache.zookeeper.server.ServerCnxnFactory.createFactory(ServerCnxnFactory.java:152)`

Thanks for persisting on this, but I don't think this erroneous case need 
to be investigated further as it would not happen when real test cases were 
running.


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549-5.patch, 
> ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15723194#comment-15723194
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r90942133
  
--- 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 --

@yufeldman Yeah the tests are fine. What I mentioned that leads to 
`NettyServerCnxnx` not get instantiated only happens in erroneous cases when 
`ZOOKEEPER_SERVER_CNXN_FACTORY` is not initialized properly - this happens when 
the public specifier was removed so all tests fall back to create 
`NIOServerCnxnx` instead. That's not a real alarm.


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549-5.patch, 
> ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15723196#comment-15723196
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user hanm commented on the issue:

https://github.com/apache/zookeeper/pull/99
  
+1 with latest update.


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549-5.patch, 
> ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15723084#comment-15723084
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

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 


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549-5.patch, 
> ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15722944#comment-15722944
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r90924628
  
--- 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 --

@yufeldman Thanks for explaining, makes sense to me. It is required to run 
the tests, however I do find another problem with tests: it looks like the 
Netty tests (testNetty*) never run with current configuration. Proof: remove 
the public access specifier appertain to `NettyServerCnxn` and all tests of 
`ServerCxnExceptionsTest` still pass. We expect Netty related tests fail here 
without public access specifier, right? Now put back the public for 
`NettyServerCnxn` but remove the public access specifier appertains to 
`NIOServercCnxn`, now all tests failed while we expect only NIO tests fail but 
Netty tests pass. 

It's likely caused by the intervening of the java system properties that 
controls the Netty vs NIO server selection. One solution is to split the 
`ServerCxnExceptionsTest` into Netty and NIO specific tests. 


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549-5.patch, 
> ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15720532#comment-15720532
 ] 

Hadoop QA commented on ZOOKEEPER-2549:
--

-1 overall.  Here are the results of testing the latest attachment 
  
http://issues.apache.org/jira/secure/attachment/12841678/ZOOKEEPER-2549-5.patch
  against trunk revision 73d6bf5353586e49740f77291d1fd98b07f916cc.

+1 @author.  The patch does not contain any @author tags.

+1 tests included.  The patch appears to include 7 new or modified tests.

+1 javadoc.  The javadoc tool did not generate any warning messages.

+1 javac.  The applied patch does not increase the total number of javac 
compiler warnings.

+1 findbugs.  The patch does not introduce any new Findbugs (version 3.0.1) 
warnings.

+1 release audit.  The applied patch does not increase the total number of 
release audit warnings.

-1 core tests.  The patch failed core unit tests.

+1 contrib tests.  The patch passed contrib unit tests.

Test results: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3538//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3538//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3538//console

This message is automatically generated.

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549-5.patch, 
> ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15720507#comment-15720507
 ] 

Hadoop QA commented on ZOOKEEPER-2549:
--

-1 overall.  GitHub Pull Request  Build
  

+1 @author.  The patch does not contain any @author tags.

+1 tests included.  The patch appears to include 7 new or modified tests.

+1 javadoc.  The javadoc tool did not generate any warning messages.

+1 javac.  The applied patch does not increase the total number of javac 
compiler warnings.

+1 findbugs.  The patch does not introduce any new Findbugs (version 3.0.1) 
warnings.

+1 release audit.  The applied patch does not increase the total number of 
release audit warnings.

-1 core tests.  The patch failed core unit tests.

+1 contrib tests.  The patch passed contrib unit tests.

Test results: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/98//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/98//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/98//console

This message is automatically generated.

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549-5.patch, 
> ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Yuliya Feldman (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15720487#comment-15720487
 ] 

Yuliya Feldman commented on ZOOKEEPER-2549:
---

I have updated PR with changes to latest review comments

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549.patch, 
> ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15720381#comment-15720381
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

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


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549.patch, 
> ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15720356#comment-15720356
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

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


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549.patch, 
> ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15720345#comment-15720345
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

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



> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549.patch, 
> ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15718613#comment-15718613
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r90762715
  
--- 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 --

Agree on re-throw the exception here. We could just remove the try - catch 
block here given there is a new try - catch block added in this PR that wraps 
the entire `sendResponse.` 
We can start triaging other places where the exceptions are swallowed but 
should re-throw after this PR merging in.


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549.patch, 
> ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15718611#comment-15718611
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r90762620
  
--- 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 --

It is not obvious to me why the access specifier of `NettyServerCnxn` 
should be changed public here.


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549.patch, 
> ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15718612#comment-15718612
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r90763391
  
--- 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

Yes, I think the connection was not closing before in cases of exception 
thrown from `NIOServerCnxn.sendResponse` which swallows everything. The change 
in this PR changes the behavior by closing the connection in case of exceptions 
occur in sendResponse. I am leaning towards the old behavior of NOT closing the 
connection, because the connection looks pretty innocent - in fact 
`NIOServerCnxn.sendResponse` does not involve any socket IO I believe, it just 
queuing stuff to be send over sockets. So if something goes wrong, we just do 
our best effort by logging what's wrong - rather than trying mess up with 
sockets which seems out of responsibilities of `NIOServerCnxn.sendResponse`. 
Similarly since `NIOServerCnxn.sendResponse` does not directly involve sockets, 
there should not be any leaks in case sendResponse screw up.


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549.patch, 
> ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15718432#comment-15718432
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user rgs1 commented on the issue:

https://github.com/apache/zookeeper/pull/99
  
@fpj, @hanm could you take a look? I am +1, but another look would be nice.


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549.patch, 
> ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15713302#comment-15713302
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

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 


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549.patch, 
> ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15713266#comment-15713266
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user lvfangmin commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r90550930
  
--- 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 --

This IOException is swallowed either, should we re-throw it?


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549.patch, 
> ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15713267#comment-15713267
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user lvfangmin commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r90549964
  
--- 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 --

We're using LOG.debug, so it shouldn't be an issue on prod.


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549.patch, 
> ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Yuliya Feldman (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15648561#comment-15648561
 ] 

Yuliya Feldman commented on ZOOKEEPER-2549:
---

the test failure is irrelevant

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549-4.patch, ZOOKEEPER-2549.patch, 
> ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15645913#comment-15645913
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user yufeldman commented on the issue:

https://github.com/apache/zookeeper/pull/99
  
Addressed review comments


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, 
> zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15642098#comment-15642098
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

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


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, 
> zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15642094#comment-15642094
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user yufeldman commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r86691054
  
--- 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 --

nothing is wrong with it per say - just overhead, as we will call 
"newInstance" on the constructor for every code path - test or not.


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, 
> zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15641172#comment-15641172
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user rgs1 commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r86679205
  
--- 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 --

what's wrong with setting serverCnxnClassCtr to NettyServerCnxn by default 
(by doing all the reflection magic)?


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, 
> zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15641168#comment-15641168
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user rgs1 commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r86679164
  
--- 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 --

hmm, that would be a pretty bad leak given all what goes on in close():

```


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, 
> zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15640683#comment-15640683
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

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 


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, 
> zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15640685#comment-15640685
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

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


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, 
> zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15640684#comment-15640684
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

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?


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, 
> zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Raul Gutierrez Segales (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15640557#comment-15640557
 ] 

Raul Gutierrez Segales commented on ZOOKEEPER-2549:
---

[~yufeldman]: thanks for addressing the comments. Added a few more comments on 
GH. Thanks!

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, 
> zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15640551#comment-15640551
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user rgs1 commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r86672326
  
--- 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 --

this is an interesting change -- how/when were we closing the connection 
before?


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, 
> zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15640553#comment-15640553
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user rgs1 commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r86672419
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java ---
@@ -105,8 +115,26 @@ public void channelConnected(ChannelHandlerContext ctx,
 LOG.trace("Channel connected " + e);
 }
 
-NettyServerCnxn cnxn = new NettyServerCnxn(ctx.getChannel(),
-zkServer, NettyServerCnxnFactory.this);
+NettyServerCnxn cnxn = null;
+if (serverCnxnClassCtr != null) {
--- End diff --

same as for NIO, lets set serverCnxnClassCtr to NettyServerCnxn by default


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, 
> zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15640555#comment-15640555
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user rgs1 commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r86672337
  
--- 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 --

i have mixed feelings with concatenating strings in a hot path (IOException 
happening here is a hot path when, for instance, a network blip happens). 


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, 
> zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15640554#comment-15640554
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user rgs1 commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r86672349
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java ---
@@ -630,6 +640,22 @@ public static ByteBuffer getDirectBuffer() {
  * limits of the operating system). startup(zks) must be called 
subsequently.
  */
 public NIOServerCnxnFactory() {
+String serverCnxnClassName = 
System.getProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN);
+if ( serverCnxnClassName != null ) {
--- End diff --

nit: extra spaces


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, 
> zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15640552#comment-15640552
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

Github user rgs1 commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/99#discussion_r86672385
  
--- 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 --

why not set serverCnxnClassCtr to NIOServerCnxn by default to avoid looking 
up the right constructor every time we want to create a new connection (a bit 
of a hot path)?


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, 
> zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Raul Gutierrez Segales (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15640362#comment-15640362
 ] 

Raul Gutierrez Segales commented on ZOOKEEPER-2549:
---

we can ignore the findbugs warnings (see ZOOKEEPER-2628). 

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, 
> zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15638838#comment-15638838
 ] 

Hadoop QA commented on ZOOKEEPER-2549:
--

-1 overall.  Here are the results of testing the latest attachment 
  
http://issues.apache.org/jira/secure/attachment/12837399/ZOOKEEPER-2549-3.patch
  against trunk revision bcb07a09b06c91243ed244f04a71b8daf629e286.

+1 @author.  The patch does not contain any @author tags.

+1 tests included.  The patch appears to include 7 new or modified tests.

+1 javadoc.  The javadoc tool did not generate any warning messages.

+1 javac.  The applied patch does not increase the total number of javac 
compiler warnings.

-1 findbugs.  The patch appears to introduce 19 new Findbugs (version 
3.0.1) warnings.

+1 release audit.  The applied patch does not increase the total number of 
release audit warnings.

+1 core tests.  The patch passed core unit tests.

+1 contrib tests.  The patch passed contrib unit tests.

Test results: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3522//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3522//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3522//console

This message is automatically generated.

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549-3.patch, ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, 
> zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15638036#comment-15638036
 ] 

Hadoop QA commented on ZOOKEEPER-2549:
--

-1 overall.  Here are the results of testing the latest attachment 
  
http://issues.apache.org/jira/secure/attachment/12837316/ZOOKEEPER-2549-3.patch
  against trunk revision bcb07a09b06c91243ed244f04a71b8daf629e286.

+1 @author.  The patch does not contain any @author tags.

+1 tests included.  The patch appears to include 9 new or modified tests.

-1 patch.  The patch command could not apply the patch.

Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3521//console

This message is automatically generated.

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549-3.patch, 
> ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Yuliya Feldman (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15637895#comment-15637895
 ] 

Yuliya Feldman commented on ZOOKEEPER-2549:
---

Submitted PR that addresses review comments

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549.patch, 
> ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15637893#comment-15637893
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2549:
---

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 
Date:   2016-09-03T05:18:30Z

ZOOKEEPER-2549 Add exception handling to sendResponse




> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549.patch, 
> ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Yuliya Feldman (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=1563#comment-1563
 ] 

Yuliya Feldman commented on ZOOKEEPER-2549:
---

[~rgs]  - One comment regarding your comment on double instantiation. It will 
happen only in case of unit tests. I will certainly make a change to avoid 
double instantiation even in that case, but code-wise it will look a bit 
"uglier". Will post new patch soon 

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549.patch, 
> ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Yuliya Feldman (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15636923#comment-15636923
 ] 

Yuliya Feldman commented on ZOOKEEPER-2549:
---

Thank you very much [~rgs] for review

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549.patch, 
> ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Raul Gutierrez Segales (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15636811#comment-15636811
 ] 

Raul Gutierrez Segales commented on ZOOKEEPER-2549:
---

[~yufeldman]: a few things:

In:

{code}
  } catch(Exception e) {
 LOG.warn("Unexpected exception. Destruction averted.", e);
+throw new IOException(e);
  }
 }
{code}

can you remove the LOG.warn()? I don't think it's relevant anymore, given it 
will be handled by the caller. 


Nit in:

{code}
+if ( serverCnxnClassName != null ) {
{code}

extra spaces around the condition.

Ditto for:

{code}
+if ( serverCnxnClassCtr != null ) {
{code}


Looks like you are doing extra work (allocations) here:

{code}
+NIOServerCnxn cnxn = new NIOServerCnxn(zkServer, sock, sk, this, 
selectorThread);
+
+if ( serverCnxnClassCtr != null ) {
+try {
+cnxn = serverCnxnClassCtr.newInstance(zkServer, sock, sk, 
this, selectorThread);
+} catch (InstantiationException e1) {
+LOG.debug("Can not instantiate class for " + 
serverCnxnClassCtr.getName() + ". Using NIOServerCnxn");
+} catch (IllegalAccessException e1) {
+LOG.debug("IllegalAccessException for " + 
serverCnxnClassCtr.getName() + ". Using NIOServerCnxn");
+} catch (InvocationTargetException e1) {
+LOG.debug("InvocationTargetException for " + 
serverCnxnClassCtr.getName() + ". Using NIOServerCnxn");
+} catch (Throwable t) {
+LOG.debug("Unknown Exception while dealing with: {} . Using 
NIOServerCnxn", serverCnxnClassCtr.getName());
+}
+}
{code}

Sounds like we should try this first (if possible):

{code}
cnxn = serverCnxnClassCtr.newInstance(zkServer, sock, sk, this, 
selectorThread);
{code}

And only fallback to this:

{code}
   cnxn = new NIOServerCnxn(zkServer, sock, sk, this, selectorThread);
{code}

if that failed.


In:

{code}
+} catch (Exception e) {
+LOG.warn("Unexpected exception. Converting to IOException.", e);
+throw new IOException(e);
 }
{code}

I'd drop the warning, it's common enough...

Extra whitespaces:

{code}
+  if ( stats != null ) {
+int length = stats.getDataLength();
+  }
{code}

Other than that, I think it's looking good. Thanks [~yufeldman]!

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549.patch, 
> ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Raul Gutierrez Segales (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15636749#comment-15636749
 ] 

Raul Gutierrez Segales commented on ZOOKEEPER-2549:
---

[~yufeldman]: oops, sorry for dropping the ball. reviewing it now. 

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549.patch, 
> ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Yuliya Feldman (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15634879#comment-15634879
 ] 

Yuliya Feldman commented on ZOOKEEPER-2549:
---

Ping again. It has been about 6 weeks.

It is really an issue at least with Netty implementation.

[~phunt], [~rgs] - Could you please review the patch, as it seems you are the 
most familiar with the code in question.

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549.patch, 
> ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Yuliya Feldman (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15514862#comment-15514862
 ] 

Yuliya Feldman commented on ZOOKEEPER-2549:
---

[~phunt], [~rgs] - Could you please review the patch, as it seems you are the 
most familiar with the code in question.

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549.patch, 
> ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Michael Han (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15514323#comment-15514323
 ] 

Michael Han commented on ZOOKEEPER-2549:


You are welcome - I like doing code reviews :)

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549.patch, 
> ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Yuliya Feldman (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15514063#comment-15514063
 ] 

Yuliya Feldman commented on ZOOKEEPER-2549:
---

Thank you very much [~hanm] for the review. I thought you are the one :). 

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549.patch, 
> ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Michael Han (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15514044#comment-15514044
 ] 

Michael Han commented on ZOOKEEPER-2549:


Hi [~yufeldman], latest patch LGTM. We need +1 from at least one committer for 
this to be committed.

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549.patch, 
> ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Yuliya Feldman (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15513854#comment-15513854
 ] 

Yuliya Feldman commented on ZOOKEEPER-2549:
---

[~hanm] - just wonder if anything else we can do here, or it is good to go?

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549.patch, 
> ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15489557#comment-15489557
 ] 

Hadoop QA commented on ZOOKEEPER-2549:
--

+1 overall.  Here are the results of testing the latest attachment 
  
http://issues.apache.org/jira/secure/attachment/12828393/ZOOKEEPER-2549-2.patch
  against trunk revision b2a484cfe743116d2531fe5d1e1d78b3960c511e.

+1 @author.  The patch does not contain any @author tags.

+1 tests included.  The patch appears to include 7 new or modified tests.

+1 javadoc.  The javadoc tool did not generate any warning messages.

+1 javac.  The applied patch does not increase the total number of javac 
compiler warnings.

+1 findbugs.  The patch does not introduce any new Findbugs (version 2.0.3) 
warnings.

+1 release audit.  The applied patch does not increase the total number of 
release audit warnings.

+1 core tests.  The patch passed core unit tests.

+1 contrib tests.  The patch passed contrib unit tests.

Test results: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3428//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3428//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3428//console

This message is automatically generated.

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549.patch, 
> ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Yuliya Feldman (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15489522#comment-15489522
 ] 

Yuliya Feldman commented on ZOOKEEPER-2549:
---

Sure - removed  if (LOG.isDebugEnabled()) 
Added comments

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549-2.patch, ZOOKEEPER-2549.patch, 
> ZOOKEEPER-2549.patch, zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Michael Han (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15489075#comment-15489075
 ] 

Michael Han commented on ZOOKEEPER-2549:


bq. Sorry, which class are you referring to for debug ?
NIOServerCnxn.java (L720-727). But since you mentioned, I checked again and it 
looks like the change is copied from NettyServerCnxn which uses the pattern 
{{if (LOG.isDebugEnabled())}}. So I think we either change both by using 
{{LOG.debug}} directly and removing {{if (LOG.isDebugEnabled())}} , or leave 
both with current form. 

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, 
> zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Yuliya Feldman (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15488893#comment-15488893
 ] 

Yuliya Feldman commented on ZOOKEEPER-2549:
---

1. Sorry, which class are you referring to for debug ?

2. Yes, I will add comments regarding use of reflection to support Mock classes 
- true it was added for testability.

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, 
> zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Michael Han (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15488851#comment-15488851
 ] 

Michael Han commented on ZOOKEEPER-2549:


LGTM. 
Two nits:
* Here might just use LOG.debug (for consistency with rest of places in the 
patch). 
{code}
if (LOG.isDebugEnabled()) {
LOG.debug("Problem sending to " + getRemoteSocketAddress(), ex);
}
{code}
* IIUC, the reflection and wildcard type parameter was introduced to improve 
testability (for the Mock* classes to work), if so, might worth to comment that 
a little bit in code, for example commenting on 'serverCnxnClassCtr', which is 
not obvious to readers about what it's designed for by looking at source code 
alone.

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, 
> zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15475439#comment-15475439
 ] 

Hadoop QA commented on ZOOKEEPER-2549:
--

-1 overall.  Here are the results of testing the latest attachment 
  
http://issues.apache.org/jira/secure/attachment/12827678/zookeeper-2549-1.patch
  against trunk revision 1759917.

+1 @author.  The patch does not contain any @author tags.

+1 tests included.  The patch appears to include 7 new or modified tests.

+1 javadoc.  The javadoc tool did not generate any warning messages.

+1 javac.  The applied patch does not increase the total number of javac 
compiler warnings.

+1 findbugs.  The patch does not introduce any new Findbugs (version 2.0.3) 
warnings.

+1 release audit.  The applied patch does not increase the total number of 
release audit warnings.

-1 core tests.  The patch failed core unit tests.

+1 contrib tests.  The patch passed contrib unit tests.

Test results: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3406//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3406//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3406//console

This message is automatically generated.

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch, 
> zookeeper-2549-1.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Yuliya Feldman (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15469002#comment-15469002
 ] 

Yuliya Feldman commented on ZOOKEEPER-2549:
---

>>> Should we also do similar updates to NIOServerCnx which swallows every 
>>> exception instead of converting them to IOException, for the purpose of 
>>> consistency?

Sure

>>> Also, for tests, might be good to reset system properties in teardown via 
>>> System.clearProperty.

Yep - will do

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Michael Han (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15468997#comment-15468997
 ] 

Michael Han commented on ZOOKEEPER-2549:


Should we also do similar updates to NIOServerCnx which swallows every 
exception instead of converting them to IOException, for the purpose of 
consistency?

Also, for tests, might be good to reset system properties in teardown via 
System.clearProperty.

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15468900#comment-15468900
 ] 

Hadoop QA commented on ZOOKEEPER-2549:
--

-1 overall.  Here are the results of testing the latest attachment 
  http://issues.apache.org/jira/secure/attachment/12827265/ZOOKEEPER-2549.patch
  against trunk revision 1757584.

+1 @author.  The patch does not contain any @author tags.

+1 tests included.  The patch appears to include 4 new or modified tests.

+1 javadoc.  The javadoc tool did not generate any warning messages.

+1 javac.  The applied patch does not increase the total number of javac 
compiler warnings.

+1 findbugs.  The patch does not introduce any new Findbugs (version 2.0.3) 
warnings.

+1 release audit.  The applied patch does not increase the total number of 
release audit warnings.

-1 core tests.  The patch failed core unit tests.

+1 contrib tests.  The patch passed contrib unit tests.

Test results: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3393//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3393//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3393//console

This message is automatically generated.

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549.patch, ZOOKEEPER-2549.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Michael Han (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15461337#comment-15461337
 ] 

Michael Han commented on ZOOKEEPER-2549:


These are known flaky tests - we have a couple of them under fix 
(ZOOKEEPER-2135).


> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Yuliya Feldman (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15461233#comment-15461233
 ] 

Yuliya Feldman commented on ZOOKEEPER-2549:
---

Really very strange test failures - completely unrelated. Could it be some 
issues with the cluster during the run?

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (ZOOKEEPER-2549) As NettyServerCnxn.sendResponse() allows all the exception to bubble up it can stop main ZK requests processing thread

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15460595#comment-15460595
 ] 

Hadoop QA commented on ZOOKEEPER-2549:
--

-1 overall.  Here are the results of testing the latest attachment 
  http://issues.apache.org/jira/secure/attachment/12826962/ZOOKEEPER-2549.patch
  against trunk revision 1757584.

+1 @author.  The patch does not contain any @author tags.

+1 tests included.  The patch appears to include 4 new or modified tests.

+1 javadoc.  The javadoc tool did not generate any warning messages.

+1 javac.  The applied patch does not increase the total number of javac 
compiler warnings.

+1 findbugs.  The patch does not introduce any new Findbugs (version 2.0.3) 
warnings.

+1 release audit.  The applied patch does not increase the total number of 
release audit warnings.

-1 core tests.  The patch failed core unit tests.

+1 contrib tests.  The patch passed contrib unit tests.

Test results: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3389//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3389//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/3389//console

This message is automatically generated.

> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread
> --
>
> Key: ZOOKEEPER-2549
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2549
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: server
>Affects Versions: 3.5.1
>Reporter: Yuliya Feldman
>Assignee: Yuliya Feldman
> Attachments: ZOOKEEPER-2549.patch
>
>
> As NettyServerCnxn.sendResponse() allows all the exception to bubble up it 
> can stop main ZK requests processing thread and make Zookeeper server look 
> like it is hanging, while it just can not process any request anymore.
> Idea is to catch all the exceptions in NettyServerCnxn.sendResponse() , 
> convert them to IOException and allow it propagating up



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)