[GitHub] [zookeeper] eolivelli commented on a change in pull request #1251: ZOOKEEPER-3720: Fix rolling upgrade failure (invalid protocol version)

2020-02-12 Thread GitBox 
eolivelli commented on a change in pull request #1251: ZOOKEEPER-3720: Fix 
rolling upgrade failure (invalid protocol version)
URL: https://github.com/apache/zookeeper/pull/1251#discussion_r378436446
 
 

 ##
 File path: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumCnxManager.java
 ##
 @@ -113,9 +115,12 @@
 private AtomicLong observerCounter = new AtomicLong(-1);
 
 /*
- * Protocol identifier used among peers
+ * Protocol identifier used among peers (must be a negative number for 
backward compatibility reasons)
  */
-public static final long PROTOCOL_VERSION = -65535L;
+// the following protocol version was sent in every connection initiation 
message since ZOOKEEPER-2186 released in 3.4.7
+public static final long PROTOCOL_VERSION_3_4_7 = -65536L;
 
 Review comment:
   I would call this constant PROTOCOL_VERSION_V1
   and the other one PROTOCOL_VERSION_V2


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [zookeeper] eolivelli commented on a change in pull request #1251: ZOOKEEPER-3720: Fix rolling upgrade failure (invalid protocol version)

2020-02-12 Thread GitBox 
eolivelli commented on a change in pull request #1251: ZOOKEEPER-3720: Fix 
rolling upgrade failure (invalid protocol version)
URL: https://github.com/apache/zookeeper/pull/1251#discussion_r378438237
 
 

 ##
 File path: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumCnxManager.java
 ##
 @@ -416,10 +424,20 @@ private boolean startConnection(Socket sock, Long sid) 
throws IOException {
 
 // Sending id and challenge
 
-// represents protocol version (in other words - message type)
-dout.writeLong(PROTOCOL_VERSION);
+// First sending the protocol version (in other words - message 
type).
+// For backward compatibility reasons we stick to the old protocol 
version, unless the MultiAddress
+// feature is enabled. During rolling upgrade, we must make sure 
that all the servers can
+// understand the protocol version we use to avoid multiple 
partitions. see ZOOKEEPER-3720
+long protocolVersion = self.isMultiAddressEnabled() ? 
PROTOCOL_VERSION_3_6_0_MULTI_ADDRESS : PROTOCOL_VERSION_3_4_7;
+dout.writeLong(protocolVersion);
 
 Review comment:
   In case of new protocol (v2) I would like to spend here a 64bit long of 
'flags' (filled with zeros in 3.6.0) in order to introduce a more futureproof 
'feature detection' that is better than using protocol version numbers.
   
   We can then add 'multiaddress' as an enabled feature.
   
   We should do more reasoning and design steps but this way of coding the 
initial handshake is quite common and it will open up the way for future 
improvements.
   
   


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [zookeeper] eolivelli commented on a change in pull request #1251: ZOOKEEPER-3720: Fix rolling upgrade failure (invalid protocol version)

2020-02-12 Thread GitBox 
eolivelli commented on a change in pull request #1251: ZOOKEEPER-3720: Fix 
rolling upgrade failure (invalid protocol version)
URL: https://github.com/apache/zookeeper/pull/1251#discussion_r378268889
 
 

 ##
 File path: zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md
 ##
 @@ -1584,6 +1596,16 @@ and [SASL authentication for 
ZooKeeper](https://cwiki.apache.org/confluence/disp
 (e.g. the zk/myh...@example.com client principal will be authenticated in 
ZooKeeper as zk/myhost)
 Default: false
 
+* *multiAddress.enable* :
 
 Review comment:
   multiAddress.enabled


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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [zookeeper] eolivelli commented on a change in pull request #1251: ZOOKEEPER-3720: Fix rolling upgrade failure (invalid protocol version)

2020-02-12 Thread GitBox 
eolivelli commented on a change in pull request #1251: ZOOKEEPER-3720: Fix 
rolling upgrade failure (invalid protocol version)
URL: https://github.com/apache/zookeeper/pull/1251#discussion_r378271226
 
 

 ##
 File path: 
zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/QuorumPeerMainMultiAddressTest.java
 ##
 @@ -230,6 +233,29 @@ public void shouldReconfigIncrementally_IPv6() throws 
Exception {
 checkIfZooKeeperQuorumWorks(newQuorumConfig);
   }
 
+  @Test(expected = KeeperException.class)
 
 Review comment:
   this way of testing for an expected exception is too broad, you don't know 
where KeeperException has been thrown.
   I suggest to push the assertion around the  ReconfigTest.reconfig call


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:
us...@infra.apache.org


With regards,
Apache Git Services