[
https://issues.apache.org/jira/browse/ZOOKEEPER-2549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=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)
