> On Feb. 25, 2015, 6:28 p.m., Raul Gutierrez Segales wrote: > > src/java/main/org/apache/zookeeper/common/X509Util.java, line 57 > > <https://reviews.apache.org/r/31277/diff/5/?file=875853#file875853line57> > > > > 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.
Sounds good to me. Will fix it soon. Can you give more details what it should look like? > On Feb. 25, 2015, 6:28 p.m., Raul Gutierrez Segales wrote: > > src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java, line > > 279 > > <https://reviews.apache.org/r/31277/diff/5/?file=875855#file875855line279> > > > > 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). Sure. Will fix it. > On Feb. 25, 2015, 6:28 p.m., Raul Gutierrez Segales wrote: > > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java, line 353 > > <https://reviews.apache.org/r/31277/diff/5/?file=875851#file875851line353> > > > > 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? I have similar concerns too when SSL parameters are given wrong. I have in this JIRA intentionally kept this simple and not included failure cases in testing. Let's discuss more and fix it in another JIRA. > On Feb. 25, 2015, 6:28 p.m., Raul Gutierrez Segales wrote: > > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java, line 1463 > > <https://reviews.apache.org/r/31277/diff/5/?file=875863#file875863line1463> > > > > when should this be done? why is this needed? i fear this is even more > > coupling between QuorumPeer, ZooKeeperServer(s) and the ServerCnxnFactory... That's a really good question. The reason for setZooKeeperServer, closeAllConnections is that Leader, Learner, ROZKServer all do the duplicate work. {code} if (cnxnFactory != null) { ... } if (secureCnxnFactory != null) { ... } {code} Regarding coupling, those three classes are already calling each others. I tried to refactor it but then I realized it's too many unrelated changes... What's your suggestion here? - Hongchao ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31277/#review74038 ----------------------------------------------------------- On Feb. 25, 2015, 7:18 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, 7:18 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 > >
