Re: Leader election

2018-12-05 Thread Enrico Olivelli
Michael,
Leader election is not enough.
You must have some mechanism to fence off the partitioned leader.

If you are building a replicated state machine Apache Zookeeper + Apache
Bookkeeper can be a good choice
See this just an example:
https://github.com/ivankelly/bookkeeper-tutorial

This is the "bible" for ZooKeepers and it describes how to build such
systems and the importance of "fencing"
https://www.amazon.com/ZooKeeper-Distributed-Coordination-Flavio-Junqueira-ebook/dp/B00GRCODKS

If you are interested in BookKeeper ping on user@ Apache BookKeeper mailing
list

Enrico




Il gio 6 dic 2018, 08:18 Michael Borokhovich  ha
scritto:

> Thanks, I will check it out.
> However, do you know if it gives any better guarantees?
> Can it happen that we end up with 2 leaders or 0 leader for some period of
> time (for example, during network delays/partitions)?
>
>
>
> On Wed, Dec 5, 2018 at 10:54 PM 毛蛤丝  wrote:
>
> > suggest you use the ready-made implements of curator:
> > http://curator.apache.org/curator-recipes/leader-election.html
> > - 原始邮件 -
> > 发件人:Michael Borokhovich 
> > 收件人:"dev@zookeeper.apache.org" 
> > 主题:Leader election
> > 日期:2018年12月06日 07点29分
> >
> > Hello,
> > We have a service that runs on 3 hosts for high availability. However, at
> > any given time, exactly one instance must be active. So, we are thinking
> to
> > use Leader election using Zookeeper.
> > To this goal, on each service host we also start a ZK server, so we have
> a
> > 3-nodes ZK cluster and each service instance is a client to its dedicated
> > ZK server.
> > Then, we implement a leader election on top of Zookeeper using a basic
> > recipe:
> > https://zookeeper.apache.org/doc/r3.1.2/recipes.html#sc_leaderElection.
> > I have the following questions doubts regarding the approach:
> > 1. It seems like we can run into inconsistency issues when network
> > partition occurs. Zookeeper documentation says that the inconsistency
> > period may last “tens of seconds”. Am I understanding correctly that
> during
> > this time we may have 0 or 2 leaders?
> > 2. Is it possible to reduce this inconsistency time (let's say to 3
> > seconds) by tweaking tickTime and syncLimit parameters?
> > 3. Is there a way to guarantee exactly one leader all the time? Should we
> > implement a more complex leader election algorithm than the one suggested
> > in the recipe (using ephemeral_sequential nodes)?
> > Thanks,
> > Michael.
> >
>


Re: Leader election

2018-12-05 Thread Michael Borokhovich
Thanks, I will check it out.
However, do you know if it gives any better guarantees?
Can it happen that we end up with 2 leaders or 0 leader for some period of
time (for example, during network delays/partitions)?



On Wed, Dec 5, 2018 at 10:54 PM 毛蛤丝  wrote:

> suggest you use the ready-made implements of curator:
> http://curator.apache.org/curator-recipes/leader-election.html
> - 原始邮件 -
> 发件人:Michael Borokhovich 
> 收件人:"dev@zookeeper.apache.org" 
> 主题:Leader election
> 日期:2018年12月06日 07点29分
>
> Hello,
> We have a service that runs on 3 hosts for high availability. However, at
> any given time, exactly one instance must be active. So, we are thinking to
> use Leader election using Zookeeper.
> To this goal, on each service host we also start a ZK server, so we have a
> 3-nodes ZK cluster and each service instance is a client to its dedicated
> ZK server.
> Then, we implement a leader election on top of Zookeeper using a basic
> recipe:
> https://zookeeper.apache.org/doc/r3.1.2/recipes.html#sc_leaderElection.
> I have the following questions doubts regarding the approach:
> 1. It seems like we can run into inconsistency issues when network
> partition occurs. Zookeeper documentation says that the inconsistency
> period may last “tens of seconds”. Am I understanding correctly that during
> this time we may have 0 or 2 leaders?
> 2. Is it possible to reduce this inconsistency time (let's say to 3
> seconds) by tweaking tickTime and syncLimit parameters?
> 3. Is there a way to guarantee exactly one leader all the time? Should we
> implement a more complex leader election algorithm than the one suggested
> in the recipe (using ephemeral_sequential nodes)?
> Thanks,
> Michael.
>


回复:Leader election

2018-12-05 Thread 毛蛤丝
suggest you use the ready-made implements of curator:
http://curator.apache.org/curator-recipes/leader-election.html
- 原始邮件 -
发件人:Michael Borokhovich 
收件人:"dev@zookeeper.apache.org" 
主题:Leader election
日期:2018年12月06日 07点29分

Hello,
We have a service that runs on 3 hosts for high availability. However, at
any given time, exactly one instance must be active. So, we are thinking to
use Leader election using Zookeeper.
To this goal, on each service host we also start a ZK server, so we have a
3-nodes ZK cluster and each service instance is a client to its dedicated
ZK server.
Then, we implement a leader election on top of Zookeeper using a basic
recipe:
https://zookeeper.apache.org/doc/r3.1.2/recipes.html#sc_leaderElection.
I have the following questions doubts regarding the approach:
1. It seems like we can run into inconsistency issues when network
partition occurs. Zookeeper documentation says that the inconsistency
period may last “tens of seconds”. Am I understanding correctly that during
this time we may have 0 or 2 leaders?
2. Is it possible to reduce this inconsistency time (let's say to 3
seconds) by tweaking tickTime and syncLimit parameters?
3. Is there a way to guarantee exactly one leader all the time? Should we
implement a more complex leader election algorithm than the one suggested
in the recipe (using ephemeral_sequential nodes)?
Thanks,
Michael.


[GitHub] zookeeper issue #731: [ZOOKEEPER-3208] Remove the SSLTest.java.orig introduc...

2018-12-05 Thread dineshappavoo
Github user dineshappavoo commented on the issue:

https://github.com/apache/zookeeper/pull/731
  
This LGTM 👍 


---


[GitHub] zookeeper issue #703: [ZOOKEEPER-1818] Correctly handle potential inconsiste...

2018-12-05 Thread hanm
Github user hanm commented on the issue:

https://github.com/apache/zookeeper/pull/703
  
@lvfangmin lgtm, thanks!


---


[GitHub] zookeeper pull request #703: [ZOOKEEPER-1818] Correctly handle potential inc...

2018-12-05 Thread hanm
Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/703#discussion_r239309222
  
--- Diff: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/FastLeaderElection.java
 ---
@@ -1023,9 +1016,9 @@ else if (validVoter(n.sid) && validVoter(n.leader)) {
  */
 if (n == null) {
 setPeerState(proposedLeader, voteSet);
-
 Vote endVote = new Vote(proposedLeader,
-proposedZxid, proposedEpoch);
+proposedZxid, logicalclock.get(), 
--- End diff --

sgtm to keep consistent with existing code style.


---


[GitHub] zookeeper pull request #628: ZOOKEEPER-3140: Allow Followers to host Observe...

2018-12-05 Thread enixon
Github user enixon closed the pull request at:

https://github.com/apache/zookeeper/pull/628


---


[GitHub] zookeeper pull request #628: ZOOKEEPER-3140: Allow Followers to host Observe...

2018-12-05 Thread enixon
GitHub user enixon reopened a pull request:

https://github.com/apache/zookeeper/pull/628

ZOOKEEPER-3140: Allow Followers to host Observers

Creates a new abstraction, LearnerMaster, to represent the portions of the 
Leader logic that are used in LearnerHandler. Leader implements LearnerMaster 
and a new class ObserverMaster implements LearnerMaster. Followers have the 
option of instantiating a ObserverMaster thread when they assume their role and 
so support Learner traffic.

A new parameter 'observerMasterPort' is used to control which Follower 
instances host Observers.


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/enixon/zookeeper learner-master

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/628.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #628


commit ab5be29541e9968654855115f654207c04e8f9f6
Author: Brian Nixon 
Date:   2018-09-12T21:07:22Z

ZOOKEEPER-3140: Allow Followers to host Observers

commit 1f915d4e1b9b7d4eab67c0117bc668018ab0e791
Author: Brian Nixon 
Date:   2018-09-15T00:12:40Z

address the findbugs nits

commit 0e522c9e572d6ef0732cee5defe84a614598442c
Author: Brian Nixon 
Date:   2018-09-27T01:11:22Z

add comments and address style nits

commit a9cc50fef0921e5b3b390eface3c0401c3fb67dc
Author: Brian Nixon 
Date:   2018-09-27T22:41:39Z

fix flaky test ObserverMasterTest::testAdminCommands

commit 3f453143d900624841d4d3ba1cd3e8a8ca227ab5
Author: Brian Nixon 
Date:   2018-10-09T22:54:17Z

address Michael's initial feedback

todo
 - add documentation to 
zookeeper-docs/src/documentation/content/xdocs/zookeeperObservers.xml to match 
https://jira.apache.org/jira/browse/ZOOKEEPER-3140 description
 - rework method docs in Learner to reflect that Followers still connect to 
the Leader (not to generic LearnerMaster's)

commit 534eba61c79e933f0fdc54961ffb33dac8d5da0b
Author: Brian Nixon 
Date:   2018-10-10T00:55:07Z

fix 50% failure rate of ObserverMasterTest::testAdminCommands

commit 332ade54b4f314b6e9084b4781da9015f1d8a13f
Author: Brian Nixon 
Date:   2018-10-22T23:45:33Z

add expanded documentation to zookeeperObservers.xml and Learner.java




---


[GitHub] zookeeper pull request #628: ZOOKEEPER-3140: Allow Followers to host Observe...

2018-12-05 Thread enixon
Github user enixon closed the pull request at:

https://github.com/apache/zookeeper/pull/628


---


[GitHub] zookeeper pull request #628: ZOOKEEPER-3140: Allow Followers to host Observe...

2018-12-05 Thread enixon
GitHub user enixon reopened a pull request:

https://github.com/apache/zookeeper/pull/628

ZOOKEEPER-3140: Allow Followers to host Observers

Creates a new abstraction, LearnerMaster, to represent the portions of the 
Leader logic that are used in LearnerHandler. Leader implements LearnerMaster 
and a new class ObserverMaster implements LearnerMaster. Followers have the 
option of instantiating a ObserverMaster thread when they assume their role and 
so support Learner traffic.

A new parameter 'observerMasterPort' is used to control which Follower 
instances host Observers.


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/enixon/zookeeper learner-master

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/628.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #628


commit ab5be29541e9968654855115f654207c04e8f9f6
Author: Brian Nixon 
Date:   2018-09-12T21:07:22Z

ZOOKEEPER-3140: Allow Followers to host Observers

commit 1f915d4e1b9b7d4eab67c0117bc668018ab0e791
Author: Brian Nixon 
Date:   2018-09-15T00:12:40Z

address the findbugs nits

commit 0e522c9e572d6ef0732cee5defe84a614598442c
Author: Brian Nixon 
Date:   2018-09-27T01:11:22Z

add comments and address style nits

commit a9cc50fef0921e5b3b390eface3c0401c3fb67dc
Author: Brian Nixon 
Date:   2018-09-27T22:41:39Z

fix flaky test ObserverMasterTest::testAdminCommands

commit 3f453143d900624841d4d3ba1cd3e8a8ca227ab5
Author: Brian Nixon 
Date:   2018-10-09T22:54:17Z

address Michael's initial feedback

todo
 - add documentation to 
zookeeper-docs/src/documentation/content/xdocs/zookeeperObservers.xml to match 
https://jira.apache.org/jira/browse/ZOOKEEPER-3140 description
 - rework method docs in Learner to reflect that Followers still connect to 
the Leader (not to generic LearnerMaster's)

commit 534eba61c79e933f0fdc54961ffb33dac8d5da0b
Author: Brian Nixon 
Date:   2018-10-10T00:55:07Z

fix 50% failure rate of ObserverMasterTest::testAdminCommands

commit 332ade54b4f314b6e9084b4781da9015f1d8a13f
Author: Brian Nixon 
Date:   2018-10-22T23:45:33Z

add expanded documentation to zookeeperObservers.xml and Learner.java




---


Leader election

2018-12-05 Thread Michael Borokhovich
Hello,

We have a service that runs on 3 hosts for high availability. However, at
any given time, exactly one instance must be active. So, we are thinking to
use Leader election using Zookeeper.
To this goal, on each service host we also start a ZK server, so we have a
3-nodes ZK cluster and each service instance is a client to its dedicated
ZK server.
Then, we implement a leader election on top of Zookeeper using a basic
recipe:
https://zookeeper.apache.org/doc/r3.1.2/recipes.html#sc_leaderElection.

I have the following questions doubts regarding the approach:

1. It seems like we can run into inconsistency issues when network
partition occurs. Zookeeper documentation says that the inconsistency
period may last “tens of seconds”. Am I understanding correctly that during
this time we may have 0 or 2 leaders?
2. Is it possible to reduce this inconsistency time (let's say to 3
seconds) by tweaking tickTime and syncLimit parameters?
3. Is there a way to guarantee exactly one leader all the time? Should we
implement a more complex leader election algorithm than the one suggested
in the recipe (using ephemeral_sequential nodes)?

Thanks,
Michael.


[GitHub] zookeeper pull request #731: [ZOOKEEPER-3208] Remove the SSLTest.java.orig i...

2018-12-05 Thread lvfangmin
GitHub user lvfangmin opened a pull request:

https://github.com/apache/zookeeper/pull/731

[ZOOKEEPER-3208] Remove the SSLTest.java.orig introduced in ZOOKEEPER-3032



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/lvfangmin/zookeeper ZOOKEEPER-3208

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/731.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #731


commit 1ed1820ba13b50724948a195548da0f7f07f2583
Author: Fangmin Lyu 
Date:   2018-12-05T22:21:57Z

Remove the SSLTest.java.orig introduced in ZOOKEEPER-3032




---


[jira] [Updated] (ZOOKEEPER-3208) Remove the SSLTest.java.orig introduced in ZOOKEEPER-3032

2018-12-05 Thread ASF GitHub Bot (JIRA)


 [ 
https://issues.apache.org/jira/browse/ZOOKEEPER-3208?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

ASF GitHub Bot updated ZOOKEEPER-3208:
--
Labels: pull-request-available  (was: )

> Remove the SSLTest.java.orig introduced in ZOOKEEPER-3032
> -
>
> Key: ZOOKEEPER-3208
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-3208
> Project: ZooKeeper
>  Issue Type: Improvement
>Reporter: Fangmin Lv
>Assignee: Fangmin Lv
>Priority: Trivial
>  Labels: pull-request-available
> Fix For: 3.6.0
>
>
> This file was introduced by mistake when we're doing the MAVEN migrating in 
> ZOOKEEPER-3032.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)


[jira] [Created] (ZOOKEEPER-3208) Remove the SSLTest.java.orig introduced in ZOOKEEPER-3032

2018-12-05 Thread Fangmin Lv (JIRA)
Fangmin Lv created ZOOKEEPER-3208:
-

 Summary: Remove the SSLTest.java.orig introduced in ZOOKEEPER-3032
 Key: ZOOKEEPER-3208
 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-3208
 Project: ZooKeeper
  Issue Type: Improvement
Reporter: Fangmin Lv
Assignee: Fangmin Lv
 Fix For: 3.6.0


This file was introduced by mistake when we're doing the MAVEN migrating in 
ZOOKEEPER-3032.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)


[GitHub] zookeeper pull request #680: ZOOKEEPER-3174: Quorum TLS - support reloading ...

2018-12-05 Thread anmolnar
Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/680#discussion_r239130075
  
--- Diff: 
zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java ---
@@ -446,4 +458,119 @@ private void configureSSLServerSocket(SSLServerSocket 
sslServerSocket) {
 LOG.debug("Using Java8-optimized cipher suites for Java version 
{}", javaVersion);
 return DEFAULT_CIPHERS_JAVA8;
 }
+
+/**
+ * Enables automatic reloading of the trust store and key store files 
when they change on disk.
+ *
+ * @throws IOException if creating the FileChangeWatcher objects fails.
+ */
+public void enableCertFileReloading() throws IOException {
+LOG.info("enabling cert file reloading");
+ZKConfig config = new ZKConfig();
+String keyStoreLocation = 
config.getProperty(sslKeystoreLocationProperty);
+if (keyStoreLocation != null && !keyStoreLocation.isEmpty()) {
+final Path filePath = 
Paths.get(keyStoreLocation).toAbsolutePath();
+Path parentPath = filePath.getParent();
+if (parentPath == null) {
+throw new IOException(
+"Key store path does not have a parent: " + 
filePath);
+}
+FileChangeWatcher newKeyStoreFileWatcher = new 
FileChangeWatcher(
+parentPath,
+watchEvent -> {
+handleWatchEvent(filePath, watchEvent);
+});
+// stop old watcher if there is one
+if (keyStoreFileWatcher != null) {
+keyStoreFileWatcher.stop();
+}
+keyStoreFileWatcher = newKeyStoreFileWatcher;
+keyStoreFileWatcher.start();
+}
+String trustStoreLocation = 
config.getProperty(sslTruststoreLocationProperty);
+if (trustStoreLocation != null && !trustStoreLocation.isEmpty()) {
+final Path filePath = 
Paths.get(trustStoreLocation).toAbsolutePath();
+Path parentPath = filePath.getParent();
+if (parentPath == null) {
+throw new IOException(
+"Trust store path does not have a parent: " + 
filePath);
+}
+FileChangeWatcher newTrustStoreFileWatcher = new 
FileChangeWatcher(
+parentPath,
+watchEvent -> {
+handleWatchEvent(filePath, watchEvent);
+});
+// stop old watcher if there is one
+if (trustStoreFileWatcher != null) {
+trustStoreFileWatcher.stop();
+}
+trustStoreFileWatcher = newTrustStoreFileWatcher;
+trustStoreFileWatcher.start();
+}
+}
+
+/**
+ * Disables automatic reloading of the trust store and key store files 
when they change on disk.
+ * Stops background threads and closes WatchService instances.
+ */
+public void disableCertFileReloading() {
--- End diff --

This method can be private.


---


[GitHub] zookeeper pull request #680: ZOOKEEPER-3174: Quorum TLS - support reloading ...

2018-12-05 Thread anmolnar
Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/680#discussion_r239143301
  
--- Diff: 
zookeeper-server/src/main/java/org/apache/zookeeper/common/FileChangeWatcher.java
 ---
@@ -0,0 +1,253 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.common;
+
+import com.sun.nio.file.SensitivityWatchEventModifier;
+import org.apache.zookeeper.server.ZooKeeperThread;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.IOException;
+import java.nio.file.ClosedWatchServiceException;
+import java.nio.file.FileSystem;
+import java.nio.file.Path;
+import java.nio.file.StandardWatchEventKinds;
+import java.nio.file.WatchEvent;
+import java.nio.file.WatchKey;
+import java.nio.file.WatchService;
+import java.util.function.Consumer;
+
+/**
+ * Instances of this class can be used to watch a directory for file 
changes. When a file is added to, deleted from,
+ * or is modified in the given directory, the callback provided by the 
user will be called from a background thread.
+ * Some things to keep in mind:
+ * 
+ * The callback should be thread-safe.
+ * Changes that happen around the time the thread is started may be 
missed.
+ * There is a delay between a file changing and the callback 
firing.
+ * The watch is not recursive - changes to subdirectories will not 
trigger a callback.
+ * 
+ */
+public final class FileChangeWatcher {
--- End diff --

Maybe it doesn't make sense, but I'm thinking of whether it would be better 
to make `FileChangeWatcher` class the thread itself instead using a "wrapped" 
Thread instance. In which case you might not need to forward the `stop()` call, 
but can call it directly.


---


[GitHub] zookeeper pull request #680: ZOOKEEPER-3174: Quorum TLS - support reloading ...

2018-12-05 Thread anmolnar
Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/680#discussion_r239138642
  
--- Diff: 
zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java ---
@@ -446,4 +458,119 @@ private void configureSSLServerSocket(SSLServerSocket 
sslServerSocket) {
 LOG.debug("Using Java8-optimized cipher suites for Java version 
{}", javaVersion);
 return DEFAULT_CIPHERS_JAVA8;
 }
+
+/**
+ * Enables automatic reloading of the trust store and key store files 
when they change on disk.
+ *
+ * @throws IOException if creating the FileChangeWatcher objects fails.
+ */
+public void enableCertFileReloading() throws IOException {
+LOG.info("enabling cert file reloading");
+ZKConfig config = new ZKConfig();
+String keyStoreLocation = 
config.getProperty(sslKeystoreLocationProperty);
+if (keyStoreLocation != null && !keyStoreLocation.isEmpty()) {
+final Path filePath = 
Paths.get(keyStoreLocation).toAbsolutePath();
+Path parentPath = filePath.getParent();
+if (parentPath == null) {
+throw new IOException(
+"Key store path does not have a parent: " + 
filePath);
+}
+FileChangeWatcher newKeyStoreFileWatcher = new 
FileChangeWatcher(
+parentPath,
+watchEvent -> {
+handleWatchEvent(filePath, watchEvent);
+});
+// stop old watcher if there is one
+if (keyStoreFileWatcher != null) {
+keyStoreFileWatcher.stop();
+}
+keyStoreFileWatcher = newKeyStoreFileWatcher;
+keyStoreFileWatcher.start();
+}
+String trustStoreLocation = 
config.getProperty(sslTruststoreLocationProperty);
+if (trustStoreLocation != null && !trustStoreLocation.isEmpty()) {
+final Path filePath = 
Paths.get(trustStoreLocation).toAbsolutePath();
+Path parentPath = filePath.getParent();
+if (parentPath == null) {
+throw new IOException(
+"Trust store path does not have a parent: " + 
filePath);
+}
+FileChangeWatcher newTrustStoreFileWatcher = new 
FileChangeWatcher(
+parentPath,
+watchEvent -> {
+handleWatchEvent(filePath, watchEvent);
+});
+// stop old watcher if there is one
+if (trustStoreFileWatcher != null) {
+trustStoreFileWatcher.stop();
+}
+trustStoreFileWatcher = newTrustStoreFileWatcher;
+trustStoreFileWatcher.start();
+}
+}
+
+/**
+ * Disables automatic reloading of the trust store and key store files 
when they change on disk.
+ * Stops background threads and closes WatchService instances.
+ */
+public void disableCertFileReloading() {
+if (keyStoreFileWatcher != null) {
+keyStoreFileWatcher.stop();
+keyStoreFileWatcher = null;
+}
+if (trustStoreFileWatcher != null) {
+trustStoreFileWatcher.stop();
+trustStoreFileWatcher = null;
+}
+}
+
+// Finalizer guardian object, see Effective Java item 7
+// TODO: finalize() is deprecated starting with Java 10. This needs to 
be
+// replaced with an explicit shutdown call.
+@SuppressWarnings("unused")
+private final Object finalizerGuardian = new Object() {
--- End diff --

Reading the referenced literature about this, I believe it should be better 
to avoid using finalizer like this. We might even be better avoid using 
finalizers entirely. There's no guarantee when finalizer gets executed, not 
even guarantee to be executed at all, it has a huge performance penalty, etc.

We should rather implement an explicit `close()` method with the 
`AutoClosable` interface and call it from QuorumPeer's shutdown() method.


---


[GitHub] zookeeper pull request #680: ZOOKEEPER-3174: Quorum TLS - support reloading ...

2018-12-05 Thread anmolnar
Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/680#discussion_r239144917
  
--- Diff: 
zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java ---
@@ -446,4 +458,119 @@ private void configureSSLServerSocket(SSLServerSocket 
sslServerSocket) {
 LOG.debug("Using Java8-optimized cipher suites for Java version 
{}", javaVersion);
 return DEFAULT_CIPHERS_JAVA8;
 }
+
+/**
+ * Enables automatic reloading of the trust store and key store files 
when they change on disk.
+ *
+ * @throws IOException if creating the FileChangeWatcher objects fails.
+ */
+public void enableCertFileReloading() throws IOException {
+LOG.info("enabling cert file reloading");
+ZKConfig config = new ZKConfig();
+String keyStoreLocation = 
config.getProperty(sslKeystoreLocationProperty);
+if (keyStoreLocation != null && !keyStoreLocation.isEmpty()) {
+final Path filePath = 
Paths.get(keyStoreLocation).toAbsolutePath();
+Path parentPath = filePath.getParent();
+if (parentPath == null) {
+throw new IOException(
+"Key store path does not have a parent: " + 
filePath);
+}
+FileChangeWatcher newKeyStoreFileWatcher = new 
FileChangeWatcher(
+parentPath,
+watchEvent -> {
+handleWatchEvent(filePath, watchEvent);
+});
+// stop old watcher if there is one
+if (keyStoreFileWatcher != null) {
+keyStoreFileWatcher.stop();
+}
+keyStoreFileWatcher = newKeyStoreFileWatcher;
+keyStoreFileWatcher.start();
+}
+String trustStoreLocation = 
config.getProperty(sslTruststoreLocationProperty);
+if (trustStoreLocation != null && !trustStoreLocation.isEmpty()) {
--- End diff --

This logic is redundant and can be extracted in a separate method.


---


ZooKeeper_branch35_jdk8 - Build # 1218 - Failure

2018-12-05 Thread Apache Jenkins Server
See https://builds.apache.org/job/ZooKeeper_branch35_jdk8/1218/

###
## LAST 60 LINES OF THE CONSOLE 
###
[...truncated 66.18 KB...]
[junit] Running org.apache.zookeeper.test.ServerCnxnTest in thread 1
[junit] Running org.apache.zookeeper.test.SessionInvalidationTest in thread 
2
[junit] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 
0.723 sec, Thread: 2, Class: org.apache.zookeeper.test.SessionInvalidationTest
[junit] Running org.apache.zookeeper.test.SessionTest in thread 2
[junit] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 
3.346 sec, Thread: 1, Class: org.apache.zookeeper.test.ServerCnxnTest
[junit] Running org.apache.zookeeper.test.SessionTimeoutTest in thread 1
[junit] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 
1.693 sec, Thread: 1, Class: org.apache.zookeeper.test.SessionTimeoutTest
[junit] Running org.apache.zookeeper.test.SessionTrackerCheckTest in thread 
1
[junit] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 
0.084 sec, Thread: 1, Class: org.apache.zookeeper.test.SessionTrackerCheckTest
[junit] Running org.apache.zookeeper.test.SessionUpgradeTest in thread 1
[junit] Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 
12.148 sec, Thread: 2, Class: org.apache.zookeeper.test.SessionTest
[junit] Running org.apache.zookeeper.test.StandaloneTest in thread 2
[junit] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 
2.631 sec, Thread: 2, Class: org.apache.zookeeper.test.StandaloneTest
[junit] Running org.apache.zookeeper.test.StatTest in thread 2
[junit] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 
1.323 sec, Thread: 2, Class: org.apache.zookeeper.test.StatTest
[junit] Running org.apache.zookeeper.test.StaticHostProviderTest in thread 2
[junit] Tests run: 26, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 
1.671 sec, Thread: 2, Class: org.apache.zookeeper.test.StaticHostProviderTest
[junit] Running org.apache.zookeeper.test.StringUtilTest in thread 2
[junit] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 
0.072 sec, Thread: 2, Class: org.apache.zookeeper.test.StringUtilTest
[junit] Running org.apache.zookeeper.test.SyncCallTest in thread 2
[junit] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 
0.696 sec, Thread: 2, Class: org.apache.zookeeper.test.SyncCallTest
[junit] Running org.apache.zookeeper.test.TruncateTest in thread 2
[junit] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 
23.645 sec, Thread: 1, Class: org.apache.zookeeper.test.SessionUpgradeTest
[junit] Running org.apache.zookeeper.test.WatchEventWhenAutoResetTest in 
thread 1
[junit] Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 
9.431 sec, Thread: 2, Class: org.apache.zookeeper.test.TruncateTest
[junit] Running org.apache.zookeeper.test.WatchedEventTest in thread 2
[junit] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 
0.07 sec, Thread: 2, Class: org.apache.zookeeper.test.WatchedEventTest
[junit] Running org.apache.zookeeper.test.WatcherFuncTest in thread 2
[junit] Tests run: 6, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 
2.68 sec, Thread: 2, Class: org.apache.zookeeper.test.WatcherFuncTest
[junit] Running org.apache.zookeeper.test.WatcherTest in thread 2
[junit] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 
20.98 sec, Thread: 1, Class: 
org.apache.zookeeper.test.WatchEventWhenAutoResetTest
[junit] Running org.apache.zookeeper.test.X509AuthTest in thread 1
[junit] Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 
0.084 sec, Thread: 1, Class: org.apache.zookeeper.test.X509AuthTest
[junit] Running org.apache.zookeeper.test.ZkDatabaseCorruptionTest in 
thread 1
[junit] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 
11.571 sec, Thread: 1, Class: org.apache.zookeeper.test.ZkDatabaseCorruptionTest
[junit] Running org.apache.zookeeper.test.ZooKeeperQuotaTest in thread 1
[junit] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 
0.753 sec, Thread: 1, Class: org.apache.zookeeper.test.ZooKeeperQuotaTest
[junit] Running org.apache.zookeeper.util.PemReaderTest in thread 1
[junit] Tests run: 8, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 
30.13 sec, Thread: 2, Class: org.apache.zookeeper.test.WatcherTest
[junit] Running org.apache.jute.BinaryInputArchiveTest in thread 2
[junit] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 
0.072 sec, Thread: 2, Class: org.apache.jute.BinaryInputArchiveTest
[junit] Tests run: 64, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 
3.215 sec, Thread: 1, Class: org.apache.zookeeper.util.PemReaderTest
[junit] Tests run: 

[jira] [Commented] (ZOOKEEPER-3188) Improve resilience to network

2018-12-05 Thread Ted Dunning (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-3188?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16710015#comment-16710015
 ] 

Ted Dunning commented on ZOOKEEPER-3188:


Taking the questions in order:


bq. Did we consider the compatibility requirement here? Will the new 
configuration format be backward compatible? One concrete use case is if a 
customer upgrades to new version with this multiple address per server 
capability but wants to roll back without rewriting the config files to older 
version.

Yes. We considered this.

The compatibility is such that old configurations will work with the new 
version. New configurations will likely not work with older versions (this is 
life). Upgrading without configuration changes will allow transparent roll 
back. Upgrading and changing the configuration to take advantage of multiple 
configurations will require configuration change to roll back. I think that 
this is unavoidable with the current configuration format. A better JSON-ish 
format would be much easier to future proof.

If the upgrade is done using multiple DNS A records for each host instead of 
configuration changes, then transparent roll back should be possible because 
the old code just takes the first address while the new code accepts all 
addresses.

.bq Did we evaluate the impact of this feature on existing server to server 
mutual authentication and authorization feature (e.g. ZOOKEEPER-1045 for 
Kerberos, ZOOKEEPER-236 for SSL), and also the impact on operations? For 
example how to configure Kerberos principals and / or SSL certs per host given 
multiple potential IP address and / or FQDN names per server?

Yes. This was considered.

There are two important cases to consider. The first is the one that arises due 
to multiple DNS records for the same host name. In this case, binding and 
authenticating against the same host name should be transparent. We will test 
this as much as feasible. 

The second case is where there are multiple host names embedded in the 
configuration. This case should also work but each separate address must be 
separately authenticated. Again, we will test this as much as possible.

.bq Could we provide more details on expected level of support with regards to 
dynamic reconfiguration feature? 

I don't understand the question. Dynamic reconfiguration involves changing the 
dynamic part of the configuration file. That can involve addresses, for 
instance. Such changes should be handled exactly the way they are now and there 
should be no interactions with the changes to the networking stack. A commit of 
a new config is a commit.

.bq Examples would be great - for example: we would support adding, removing, 
or updating server address that's appertained to a given server via dynamic 
reconfiguration, and also the expected behavior in each case. For example, 
adding a new address to an existing ensemble member should not cause any 
disconnect / reconnect but removing an in use address of a server should cause 
a disconnect. Likely the dynamic reconfig API / CLI / doc should be updated 
because of this.

I don't really see how this pertains other than the desire not to lose a live 
connection. The documentation, in particular, should be essentially identical 
except that an example of adding an address might be nice (but kind of 
redundant).


> Improve resilience to network
> -
>
> Key: ZOOKEEPER-3188
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-3188
> Project: ZooKeeper
>  Issue Type: Bug
>Reporter: Ted Dunning
>Priority: Major
>  Labels: pull-request-available
>  Time Spent: 0.5h
>  Remaining Estimate: 0h
>
> We propose to add network level resiliency to Zookeeper. The ideas that we 
> have on the topic have been discussed on the mailing list and via a 
> specification document that is located at 
> [https://docs.google.com/document/d/1iGVwxeHp57qogwfdodCh9b32P2_kOQaJZ2GDo7j36fI/edit?usp=sharing]
> That document is copied to this issue which is being created to report the 
> results of experimental implementations.
> h1. Zookeeper Network Resilience
> h2. Background
> Zookeeper is designed to help in building distributed systems. It provides a 
> variety of operations for doing this and all of these operations have rather 
> strict guarantees on semantics. Zookeeper itself is a distributed system made 
> up of cluster containing a leader and a number of followers. The leader is 
> designated in a process known as leader election in which a majority of all 
> nodes in the cluster must agree on a leader. All subsequent operations are 
> initiated by the leader and completed when a majority of nodes have confirmed 
> the operation. Whenever an operation cannot be confirmed by a majority or 
> whenever the leader goes missing for a time, a new leader 

[jira] [Comment Edited] (ZOOKEEPER-3188) Improve resilience to network

2018-12-05 Thread Ted Dunning (JIRA)


[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-3188?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16710015#comment-16710015
 ] 

Ted Dunning edited comment on ZOOKEEPER-3188 at 12/5/18 12:37 PM:
--

Taking the questions in order:


bq. Did we consider the compatibility requirement here? Will the new 
configuration format be backward compatible? One concrete use case is if a 
customer upgrades to new version with this multiple address per server 
capability but wants to roll back without rewriting the config files to older 
version.

Yes. We considered this.

The compatibility is such that old configurations will work with the new 
version. New configurations will likely not work with older versions (this is 
life). Upgrading without configuration changes will allow transparent roll 
back. Upgrading and changing the configuration to take advantage of multiple 
configurations will require configuration change to roll back. I think that 
this is unavoidable with the current configuration format. A better JSON-ish 
format would be much easier to future proof.

If the upgrade is done using multiple DNS A records for each host instead of 
configuration changes, then transparent roll back should be possible because 
the old code just takes the first address while the new code accepts all 
addresses.

bq. Did we evaluate the impact of this feature on existing server to server 
mutual authentication and authorization feature (e.g. ZOOKEEPER-1045 for 
Kerberos, ZOOKEEPER-236 for SSL), and also the impact on operations? For 
example how to configure Kerberos principals and / or SSL certs per host given 
multiple potential IP address and / or FQDN names per server?

Yes. This was considered.

There are two important cases to consider. The first is the one that arises due 
to multiple DNS records for the same host name. In this case, binding and 
authenticating against the same host name should be transparent. We will test 
this as much as feasible. 

The second case is where there are multiple host names embedded in the 
configuration. This case should also work but each separate address must be 
separately authenticated. Again, we will test this as much as possible.

bq. Could we provide more details on expected level of support with regards to 
dynamic reconfiguration feature? 

I don't understand the question. Dynamic reconfiguration involves changing the 
dynamic part of the configuration file. That can involve addresses, for 
instance. Such changes should be handled exactly the way they are now and there 
should be no interactions with the changes to the networking stack. A commit of 
a new config is a commit.

bq. Examples would be great - for example: we would support adding, removing, 
or updating server address that's appertained to a given server via dynamic 
reconfiguration, and also the expected behavior in each case. For example, 
adding a new address to an existing ensemble member should not cause any 
disconnect / reconnect but removing an in use address of a server should cause 
a disconnect. Likely the dynamic reconfig API / CLI / doc should be updated 
because of this.

I don't really see how this pertains other than the desire not to lose a live 
connection. The documentation, in particular, should be essentially identical 
except that an example of adding an address might be nice (but kind of 
redundant).



was (Author: tdunning):
Taking the questions in order:


bq. Did we consider the compatibility requirement here? Will the new 
configuration format be backward compatible? One concrete use case is if a 
customer upgrades to new version with this multiple address per server 
capability but wants to roll back without rewriting the config files to older 
version.

Yes. We considered this.

The compatibility is such that old configurations will work with the new 
version. New configurations will likely not work with older versions (this is 
life). Upgrading without configuration changes will allow transparent roll 
back. Upgrading and changing the configuration to take advantage of multiple 
configurations will require configuration change to roll back. I think that 
this is unavoidable with the current configuration format. A better JSON-ish 
format would be much easier to future proof.

If the upgrade is done using multiple DNS A records for each host instead of 
configuration changes, then transparent roll back should be possible because 
the old code just takes the first address while the new code accepts all 
addresses.

.bq Did we evaluate the impact of this feature on existing server to server 
mutual authentication and authorization feature (e.g. ZOOKEEPER-1045 for 
Kerberos, ZOOKEEPER-236 for SSL), and also the impact on operations? For 
example how to configure Kerberos principals and / or SSL certs per host given 
multiple potential IP address and / or FQDN names per server?

Yes. This was 

[GitHub] zookeeper pull request #730: Zookeeper-3188: Improve resilience to network

2018-12-05 Thread ArtemChernatsky
GitHub user ArtemChernatsky opened a pull request:

https://github.com/apache/zookeeper/pull/730

Zookeeper-3188: Improve resilience to network

According to issue 
[ZOOKEEPER-3188](https://issues.apache.org/jira/browse/ZOOKEEPER-3188) added 
ability to specify several addresses for quorum operations. Also added 
reconnection attempts if connection to leader lost.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/mapr-demos/zookeeper ZOOKEEPER-3188-master

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/730.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #730


commit 56e0b6c60c4ccee09e2de8b407705c3f51602419
Author: ArtemChernatsky 
Date:   2018-11-23T16:31:59Z

added multiple addresses support for quorum




---


[GitHub] zookeeper pull request #725: Zookeeper-3188: Improve resilience to network

2018-12-05 Thread ArtemChernatsky
Github user ArtemChernatsky closed the pull request at:

https://github.com/apache/zookeeper/pull/725


---


[GitHub] zookeeper issue #715: Rollup of blocker/critical fixes for 3.5 (to trigger C...

2018-12-05 Thread nkalmar
Github user nkalmar commented on the issue:

https://github.com/apache/zookeeper/pull/715
  
Oh, so it's basically an aggregation of the flaky test fixes, I already 
seen some of your PR on them. 

This is awesome, thanks for this!


---