> On Sept. 19, 2016, 11:52 p.m., Michael Han wrote: > > src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java, > > line 117 > > <https://reviews.apache.org/r/47354/diff/9/?file=1498960#file1498960line117> > > > > This inprogressConnections is used to make sure we don't have > > duplicated auth session for outgoing connections, so it make sense to me. > > > > However, this set only applies to outgoing connections. For incoming > > connections, there is no corresponding structure to deduplicate. Do we need > > to also do similar book keeping on the receiver side (i.e. in > > handleConnections)?
I've added the 'inprogressConnections' datastructure to control the number of connection init attempts. There are two places it will initiate the connection, QuorumCnxManager#connectAll() and QuorumCnxManager#handleConnection(). But for the outgoing connections, they are channeled via Listener#run, I think there won't be any duplication, any thoughts? > 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. 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? > On Sept. 19, 2016, 11:52 p.m., Michael Han wrote: > > src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java, > > line 219 > > <https://reviews.apache.org/r/47354/diff/9/?file=1498960#file1498960line219> > > > > Should we put the check of in progress sid here, or up on the caller > > site (connectOne) who calls initateConnection? I am asking because I > > spotted there could be a potential socket leak if we put check here. > > Consider this case: > > > > * In connectOne(long sid), we first open a new socket and then call > > initiateConnection(sock, sid). > > * In side initiateConnection(sock, sid), we are doing this sid check > > and will return if we find out there is an ongoing outgoing connections to > > the same sid. > > * Now, the opened socket in connectOne is a strayed socket, and will > > not get used, or closed. > > > > If we put same check in connectOne, this problem can be fixed. Does > > this make sense for the case I am describing above? Thanks Michael for pointing out this. Yes, its a connection leak, I will do the necessary changes. > 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. 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. - Rakesh ----------------------------------------------------------- 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 > >
