This is an automatically generated e-mail. To reply, visit:
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 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
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.
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
* 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
* 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?
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.
- Michael Han
On Sept. 15, 2016, 4:48 p.m., Rakesh R wrote:
> This is an automatically generated e-mail. To reply, visit:
> (Updated Sept. 15, 2016, 4:48 p.m.)
> Review request for zookeeper, fpj, Ivan Kelly, Patrick Hunt, and Raul
> Gutierrez Segales.
> Bugs: ZOOKEEPER-1045
> Repository: zookeeper-git
> Quorum mutual authentication using SASL mechanism - Digest/Kerberos
> build.xml ee6834c
> ivy.xml 95b0e5a
> src/java/main/org/apache/zookeeper/Login.java aaa220c
> 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/quorum/Leader.java c83d352
> src/java/main/org/apache/zookeeper/server/quorum/Learner.java 647b8a2
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 2f0f21b
> 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/LearnerTest.java 2ae57ce
> src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java ab8ce42
> src/java/test/org/apache/zookeeper/test/FLEPredicateTest.java 8088505
> src/zookeeper.jute 6521e54
> Diff: https://reviews.apache.org/r/47354/diff/
> Added unit test cases to verify the changes.
> Rakesh R