> On Sept. 19, 2016, 11:52 p.m., Michael Han wrote: > > src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java, > > line 166 > > <https://reviews.apache.org/r/47354/diff/9/?file=1498960#file1498960line166> > > > > I have a mixed feeling about this change - it makes QuorumCnxManager's > > constructor more complicated comparing to the previous constructor > > QuorumCnxManager(QuorumPeer), where we just pass a QuorumPeer object. That > > said though, with the new QuorumPeer.createCnxManager method, this is not a > > big problem, as a QuorumCnxManager can be conveniently created given a > > QuorumPeer object. > > > > Alternatively, we can keep QuorumCnxManager's constructor as it is, so > > it still accepts a QuorumPeer object and inside QuorumCnxManager's > > constructor, we initialize various fields required here (such as > > authServer/Learner, and so on.). This requires QuorumCnxManager be able to > > access QuorumPeer's protected / private fields, so it requires adding some > > getter methods on QuorumPeer to expose those properties. > > > > Overall I don't have a strong opinion one way or the other, wondering > > what other reviewers think about this. > > Rakesh R wrote: > Thanks Michael for the imprv point. Initially I've implemented the same > way you proposed. Later based on the discussions, one point has come up to > reduce the dependency with QuorumPeer, so that we could unit test esaily the > QuorumCnxManager class. Please refer QuorumCnxManagerTest.java test class. > How about will keep this point open and see others response about this change > and based on that will take this ahead. Does this makes sense?
Agreed. > On Sept. 19, 2016, 11:52 p.m., Michael Han wrote: > > src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java, > > line 328 > > <https://reviews.apache.org/r/47354/diff/9/?file=1498960#file1498960line328> > > > > Here we create new threads to handle both incoming connection request > > handling and out going connection initiation and set up. It makes sense to > > me as SASL auth evaluation might take a while - I am curious, Rakesh, do > > you have any rough idea / data on how long the auth would take? Would it > > take at the magnitude of million seconds, or seconds, or even minutes? My > > impression is that SASL evaluation between client and server should be done > > at the scale of seconds at most, as it purely involves two servers which > > sending small data between each other, and does not involve KDC (as > > Kerberos login should have already been finished when we reach here.). > > > > I am trying to justify the usage of threads here because we did not > > create threads to handle such connection requests, so this change is pretty > > big in terms of impact to existing codebase. Because we don't create > > threads, the old code base was executed purely sequentially. With threads, > > the semantic is slightly changed as the code after receiveConnection might > > be executing without receiveConnection actually finished. So far I have not > > spotted anywhere in code base with such sequential dependency, but want to > > raise the awareness of this and see what others think about this. > > > > Also, note that this thread code path is by default (and the only) code > > path for QuorumCnxManager connection handling, regardless if users enable > > server to server auth or not. I am wondering should we have another code > > path so users who don't enable server to server auth will get exactly the > > old behavior. In other words, when server to server auth is not enabled, we > > will be very confident that our change will not introduce any regressions. > > Rakesh R wrote: > Yes, the SASL evaluation path may take at the magnitude of seconds. > Again, this depends on the I/O resources(network speed etc). > Its good point to maintain the existing code as it is and add the new > code separately. I also have a feeling that folks are having concerns about > the complexity of the code changes, may be this this approach will help to > gain more confidence from all. Let me think way to seggregate this out. Chat with Rakesh more about this, and we think use threading here is justified and neccessary. The key reason we need use threading is because the QuormCnxManager handing connections for a specific peer in a sequential way - so if a server is trying to connect to multiple peers, the connection will block until previous request is fininshed, unless we use threading to handle each connection in parallel. We also agreed that it would be good to keep the previous single threaded connection handling code path, and users who don't want enable server to server auth will get exact same old behavior with the previous single thread connection code path. This is good for stability and compatibility, and keep a different code path is not too complicated to do. In future releases we can decide if we need to deprecated the single thread code path or not, depending on feedback from production and more testing involved in the new code path. - Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47354/#review149566 ----------------------------------------------------------- On Sept. 15, 2016, 4:48 p.m., Rakesh R wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47354/ > ----------------------------------------------------------- > > (Updated Sept. 15, 2016, 4:48 p.m.) > > > Review request for zookeeper, fpj, Ivan Kelly, Patrick Hunt, and Raul > Gutierrez Segales. > > > Bugs: ZOOKEEPER-1045 > https://issues.apache.org/jira/browse/ZOOKEEPER-1045 > > > Repository: zookeeper-git > > > Description > ------- > > Quorum mutual authentication using SASL mechanism - Digest/Kerberos > > > Diffs > ----- > > build.xml ee6834c > ivy.xml 95b0e5a > src/java/main/org/apache/zookeeper/Login.java aaa220c > src/java/main/org/apache/zookeeper/SaslClientCallbackHandler.java > PRE-CREATION > src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 21ef0fa > src/java/main/org/apache/zookeeper/server/ZooKeeperSaslServer.java 71870ce > > src/java/main/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java > 2fbd6ed > src/java/main/org/apache/zookeeper/server/quorum/Leader.java c83d352 > src/java/main/org/apache/zookeeper/server/quorum/Learner.java 647b8a2 > src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java > 8a748c7 > src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java > 20e5f16 > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 2f0f21b > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java > 0924ef6 > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java > e9c8007 > > src/java/main/org/apache/zookeeper/server/quorum/auth/NullQuorumAuthLearner.java > PRE-CREATION > > src/java/main/org/apache/zookeeper/server/quorum/auth/NullQuorumAuthServer.java > PRE-CREATION > src/java/main/org/apache/zookeeper/server/quorum/auth/QuorumAuth.java > PRE-CREATION > > src/java/main/org/apache/zookeeper/server/quorum/auth/QuorumAuthLearner.java > PRE-CREATION > src/java/main/org/apache/zookeeper/server/quorum/auth/QuorumAuthServer.java > PRE-CREATION > src/java/main/org/apache/zookeeper/server/quorum/auth/README.md > PRE-CREATION > > src/java/main/org/apache/zookeeper/server/quorum/auth/SaslQuorumAuthLearner.java > PRE-CREATION > > src/java/main/org/apache/zookeeper/server/quorum/auth/SaslQuorumAuthServer.java > PRE-CREATION > > src/java/main/org/apache/zookeeper/server/quorum/auth/SaslQuorumServerCallbackHandler.java > PRE-CREATION > src/java/main/org/apache/zookeeper/util/SecurityUtils.java PRE-CREATION > src/java/test/data/kerberos/minikdc-krb5.conf PRE-CREATION > src/java/test/data/kerberos/minikdc.ldiff PRE-CREATION > src/java/test/org/apache/zookeeper/server/quorum/CnxManagerTest.java > 8db7fa8 > > src/java/test/org/apache/zookeeper/server/quorum/FLEBackwardElectionRoundTest.java > c1259d1 > src/java/test/org/apache/zookeeper/server/quorum/FLECompatibilityTest.java > 72e4fc9 > src/java/test/org/apache/zookeeper/server/quorum/FLEDontCareTest.java > a4c0cb0 > src/java/test/org/apache/zookeeper/server/quorum/FLELostMessageTest.java > 39a53ca > src/java/test/org/apache/zookeeper/server/quorum/LearnerTest.java 2ae57ce > src/java/test/org/apache/zookeeper/server/quorum/QuorumCnxManagerTest.java > PRE-CREATION > src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java > 85817b2 > src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java ab8ce42 > > src/java/test/org/apache/zookeeper/server/quorum/auth/KerberosSecurityTestcase.java > PRE-CREATION > > src/java/test/org/apache/zookeeper/server/quorum/auth/KerberosTestUtils.java > PRE-CREATION > src/java/test/org/apache/zookeeper/server/quorum/auth/MiniKdc.java > PRE-CREATION > src/java/test/org/apache/zookeeper/server/quorum/auth/MiniKdcTest.java > PRE-CREATION > > src/java/test/org/apache/zookeeper/server/quorum/auth/QuorumAuthTestBase.java > PRE-CREATION > > src/java/test/org/apache/zookeeper/server/quorum/auth/QuorumAuthUpgradeTest.java > PRE-CREATION > > src/java/test/org/apache/zookeeper/server/quorum/auth/QuorumDigestAuthTest.java > PRE-CREATION > > src/java/test/org/apache/zookeeper/server/quorum/auth/QuorumKerberosAuthTest.java > PRE-CREATION > > src/java/test/org/apache/zookeeper/server/quorum/auth/QuorumKerberosHostBasedAuthTest.java > PRE-CREATION > src/java/test/org/apache/zookeeper/test/FLEPredicateTest.java 8088505 > src/zookeeper.jute 6521e54 > > Diff: https://reviews.apache.org/r/47354/diff/ > > > Testing > ------- > > Added unit test cases to verify the changes. > > > Thanks, > > Rakesh R > >
