[GitHub] [zookeeper] symat commented on a change in pull request #1289: ZOOKEEPER-3756: Members slow to rejoin quorum using Kubernetes

[GitBox <gi...@apache.org>]

symat commented on a change in pull request #1289: ZOOKEEPER-3756: Members slow to rejoin quorum using Kubernetes
URL: https://github.com/apache/zookeeper/pull/1289#discussion_r395082580
 
 

 ##########
 File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumCnxManager.java
 ##########
 @@ -359,20 +359,49 @@ public Thread newThread(Runnable r) {
      *
      * @param sid
      */
-    public void testInitiateConnection(long sid) throws Exception {
+    public void testInitiateConnection(long sid) {
         LOG.debug("Opening channel to server {}", sid);
-        Socket sock = new Socket();
-        setSockOpts(sock);
-        InetSocketAddress address = self.getVotingView().get(sid).electionAddr.getReachableOrOne();
-        sock.connect(address, cnxTO);
-        initiateConnection(sock, sid);
+        initiateConnection(self.getVotingView().get(sid).electionAddr, sid);
     }
 
     /**
+     * First we create the socket, perform SSL handshake and authentication if needed.
+     * Then we perform the initiaion protocol.
      * If this server has initiated the connection, then it gives up on the
      * connection if it loses challenge. Otherwise, it keeps the connection.
      */
-    public void initiateConnection(final Socket sock, final Long sid) {
+    public void initiateConnection(final MultipleAddresses electionAddr, final Long sid) {
+        Socket sock = null;
+        try {
+            LOG.debug("Opening channel to server {}", sid);
+            if (self.isSslQuorum()) {
 
 Review comment:
   The makeSocket() is also a nice idea :)
   
   I was thinking 1-2 times already to use PowerMock. On the other hand the new JDK versions seems to be more strict with reflection... so I am not sure how 'future-proof' it would be.
   
   I was also thinking if we should use the `zookeeper.serverCnxnFactory` system property in the test, but we don't use the `ServerCnxnFactory` approach for the leaser election AFAICS. And implementing that would have been a longer story. Maybe for 4.0 it would be good, if we would touch the Leader Election part.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services