Thomas Koch commented on ZOOKEEPER-823:

Hi Patrick,

please excuse me, but when it comes to clean code I'm a pain in the ass.

ClientCnxnFactory.createConnection (in both variants) looks up the name of the 
factory from System.getProperty(ZOOKEEPER_CLIENT_CNXN_FACTORY). I think this is 
ugly. It should not be the concern of createConnection where the name comes 
from (System property, config file, ...). It should receive the name as a 
parameter. You can then still have a helper method to look up the name from the 
system property.

Are the classes NIOClientCnxnFactory and NettyClientCnxnFactory really 
necessary? I'm not fluent enough in java, but this seems very verbose and 
unnecessarily confusing to me.

The two methods ClientCnxnFactory.createConnection are mostly cut&paste code. 
One should call the other with null values for long sessionId, byte[] 

In NIOClientCnxn.cleanup() you check for LOG.isDebugEnabled(). This would be 
warranted only for log messages that are expensive to build and inside of code 
that's called often. Both conditions are not true in these cases.

Now that you've put all the socket related code in the two XXXClientCnxn 
packages it becomes obvious, that ClientCnxn actually has at least two 
responsibilities that could be separated in different classes. ClientCnxn could 
be split up in one class working with Packets, handling sockets and another 
class preparing and handling finished Packets. The first may be called abstract 
ClientCnxnSocket and the second remains ClientCnxn.
ClientCnxn could work with two types of ClientCnxnSocket: NioClientCnxnSocket 
and NettyClientCnxnSocket. The ClientCnxnSocket classes would probably not even 
be aware of that they are working for ZooKeeper. They just happily send and 
receive Packets and handle a Thread for that issue. I don't know, how much work 
it would mean for you to refactor your patch in this way, but I think it would 
clean up the code a lot. (And make all of us happy in the long run.)

> update ZooKeeper java client to optionally use Netty for connections
> --------------------------------------------------------------------
>                 Key: ZOOKEEPER-823
>                 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-823
>             Project: Zookeeper
>          Issue Type: New Feature
>          Components: java client
>            Reporter: Patrick Hunt
>            Assignee: Patrick Hunt
>             Fix For: 3.4.0
>         Attachments: ZOOKEEPER-823.patch, ZOOKEEPER-823.patch
> This jira will port the client side connection code to use netty rather than 
> direct nio.

This message is automatically generated by JIRA.
You can reply to this email to add a comment to the issue online.

Reply via email to