----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31277/#review74038 -----------------------------------------------------------
src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java <https://reviews.apache.org/r/31277/#comment120503> just use Boolean.getBoolean(), no need for System.getProperty. src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java <https://reviews.apache.org/r/31277/#comment120505> so if any of this fails, we just give up and throw? why not catch it, report that SSL setup failed and go on without SSL? src/java/main/org/apache/zookeeper/ZooKeeper.java <https://reviews.apache.org/r/31277/#comment120507> nit: if you grep around for other properties, the convention seems to be: zookeeper.<component>.<property>. so maybe: zookeeper.client.enableSecure ? src/java/main/org/apache/zookeeper/common/X509Util.java <https://reviews.apache.org/r/31277/#comment120508> nit: no need to say it's complicated :-) maybe just: /* * Utility code for X509 handling */ src/java/main/org/apache/zookeeper/common/X509Util.java <https://reviews.apache.org/r/31277/#comment120511> why the line wrap, this is < 80 cols. src/java/main/org/apache/zookeeper/common/X509Util.java <https://reviews.apache.org/r/31277/#comment120512> why throw all of these? i think we should handle them and just throw one (probably custom) exception (i.e.: FailedKeyManager or such). No need to leak all these details. src/java/main/org/apache/zookeeper/common/X509Util.java <https://reviews.apache.org/r/31277/#comment120514> ditto about bubbling up specific exceptions src/java/main/org/apache/zookeeper/common/X509Util.java <https://reviews.apache.org/r/31277/#comment120515> ditto src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java <https://reviews.apache.org/r/31277/#comment120517> nit: initSSL() also, throws Exception is to broad. we should catch the specific ssl exceptions and just throw a custom one (i.e.: InitFailedSSL) (to make this code future-proof when we change the underlying ssl libs, etc). src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java <https://reviews.apache.org/r/31277/#comment120519> lets be explicit about public/private for readability src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java <https://reviews.apache.org/r/31277/#comment120520> why the line wrap? it's a short line src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java <https://reviews.apache.org/r/31277/#comment120522> should it not be the sum of secure + non-secure? src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java <https://reviews.apache.org/r/31277/#comment120538> when should this be done? why is this needed? i fear this is even more coupling between QuorumPeer, ZooKeeperServer(s) and the ServerCnxnFactory... src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java <https://reviews.apache.org/r/31277/#comment120540> (a bit of nit): simplify this by assigning the intermediate strings, otherwise it's a bit confusing with the two ternaries. src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java <https://reviews.apache.org/r/31277/#comment120542> tis could be LOG.info src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java <https://reviews.apache.org/r/31277/#comment120543> just TIMEOUT? src/java/test/org/apache/zookeeper/test/SslTest.java <https://reviews.apache.org/r/31277/#comment120544> nit: SSLTest src/java/test/org/apache/zookeeper/test/SslTest.java <https://reviews.apache.org/r/31277/#comment120545> why the line wrap? the following lines are even longer src/java/test/org/apache/zookeeper/test/SslTest.java <https://reviews.apache.org/r/31277/#comment120546> nit: use String.format(), concatenation is harder to read/maintain - Raul Gutierrez Segales On Feb. 25, 2015, 5:35 p.m., Hongchao Deng wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31277/ > ----------------------------------------------------------- > > (Updated Feb. 25, 2015, 5:35 p.m.) > > > Review request for zookeeper. > > > Repository: zookeeper-git > > > Description > ------- > > ZOOKEEPER-2125: SSL on Netty client-server communication > > > Diffs > ----- > > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java PRE-CREATION > src/java/main/org/apache/zookeeper/ZooKeeper.java > dd13cc9ba5096312b06999a03ae0057cd3677623 > src/java/main/org/apache/zookeeper/common/X509Util.java PRE-CREATION > src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java > acabb33f6c7a000706763ccba94cbaf5aaaca08e > src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java > 41268805fe16244aeea4db3f35f13a6987b30187 > src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java > 14037722c569d560acef56de0b5a7ae13464128c > src/java/main/org/apache/zookeeper/server/ServerConfig.java > f2b8463e871739319bdf40be1f014d5ad0af5602 > src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java > 30a0ed390bb7473ddb36757da97bc7d5f4281887 > src/java/main/org/apache/zookeeper/server/ZooKeeperServerBean.java > 0eb5c7f979199f2f7dcb9e5cfa011a9b20113713 > src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java > b756d349abeb1fc69534100c3633db4c1c18e031 > src/java/main/org/apache/zookeeper/server/quorum/Leader.java > 20589045752a7ba4ae9c9090055a4fcbe86a8eda > src/java/main/org/apache/zookeeper/server/quorum/Learner.java > 4dd1e947357080f3e055f3e7e2a78c979daa6ea7 > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java > 388ceeb45bd18c7cb8f0766a96ebd4a54a9e76de > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java > badc8df1f05dea4be337bc8312d7ac22f6c77dc3 > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java > d17c58d59e0131a78adde1becb5c23ce8c7a16a7 > > src/java/main/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java > 2aab6d09f9bd980ed76f886fb8168aae2ac8f99f > src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java > 6ab19b1eb137c8b13b8ad031d474e213267da1ea > src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java > 6ce058e48d17410d89d8348ee659dd7752bfd578 > src/java/test/org/apache/zookeeper/test/ReconfigTest.java > 8b238ee7463508122010208ebc3e786caa2cf1b1 > src/java/test/org/apache/zookeeper/test/SslTest.java PRE-CREATION > > Diff: https://reviews.apache.org/r/31277/diff/ > > > Testing > ------- > > > Thanks, > > Hongchao Deng > >
