You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2021/01/18 15:06:52 UTC
[GitHub] [zookeeper] srdo opened a new pull request #1581: ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.emp…
srdo opened a new pull request #1581:
URL: https://github.com/apache/zookeeper/pull/1581
…ty is true
snapshot.trust.empty is an escape hatch for users upgrading from 3.4.x to later Zookeeper
versions, allowing nodes to start with a non-empty transaction log but no snapshot.
The intent is for this setting to be enabled for a short while during the upgrade,
and then disabled again, as the check it disables is a safety feature.
Prior to this PR, a node would only write a snapshot locally if it became leader,
or if it had fallen so far behind the leader that the leader sent a SNAP message instead
of a DIFF. This made the upgrade process inconvenient, as not all nodes would create
a snapshot when snapshot.trust.empty was true, meaning that the safety check could
not be flipped back on.
This PR makes follower nodes write a local snapshot when they receive NEWLEADER,
if they have no local snapshot and snapshot.trust.empty is true.
----------------------------------------------------------------
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
[GitHub] [zookeeper] srdo commented on pull request #1581: ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.emp…
Posted by GitBox <gi...@apache.org>.
srdo commented on pull request #1581:
URL: https://github.com/apache/zookeeper/pull/1581#issuecomment-762485045
I think these test failures are on master as well, see e.g. https://github.com/apache/zookeeper/runs/1717919885. Seems to be tests trying to bind in-use ports.
----------------------------------------------------------------
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
[GitHub] [zookeeper] ztzg closed pull request #1581: ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.emp…
Posted by GitBox <gi...@apache.org>.
ztzg closed pull request #1581:
URL: https://github.com/apache/zookeeper/pull/1581
----------------------------------------------------------------
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
[GitHub] [zookeeper] srdo commented on pull request #1581: ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.emp…
Posted by GitBox <gi...@apache.org>.
srdo commented on pull request #1581:
URL: https://github.com/apache/zookeeper/pull/1581#issuecomment-762307505
Additional context here https://mail-archives.apache.org/mod_mbox/zookeeper-dev/202101.mbox/%3cpony-db78e81c2a8c7e7aa50468ebe248816470b8ee2f-8a8224758cb92c542b444f6af8884c82154cdaf2@dev.zookeeper.apache.org%3e
----------------------------------------------------------------
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
[GitHub] [zookeeper] srdo commented on a change in pull request #1581: ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.emp…
Posted by GitBox <gi...@apache.org>.
srdo commented on a change in pull request #1581:
URL: https://github.com/apache/zookeeper/pull/1581#discussion_r560072321
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
##########
@@ -530,6 +530,10 @@ public void takeSnapshot(boolean syncSnap) {
ServerMetrics.getMetrics().SNAPSHOT_TIME.add(elapsed);
}
+ public boolean shouldForceWriteInitialSnapshotDuring34Migration() {
Review comment:
Ok, will fix. How about `shouldForceWriteInitialSnapshotAfterLeaderElection`?
----------------------------------------------------------------
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
[GitHub] [zookeeper] srdo commented on pull request #1581: ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.emp…
Posted by GitBox <gi...@apache.org>.
srdo commented on pull request #1581:
URL: https://github.com/apache/zookeeper/pull/1581#issuecomment-792040020
Sounds good, thank you.
----------------------------------------------------------------
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
[GitHub] [zookeeper] ztzg commented on a change in pull request #1581: ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.emp…
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1581:
URL: https://github.com/apache/zookeeper/pull/1581#discussion_r577814512
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/EmptiedSnapshotRecoveryTest.java
##########
@@ -143,6 +144,48 @@ public void testRestoreWithTrustedEmptySnapFiles() throws Exception {
runTest(false, true);
}
+ @Test
+ public void testRestoreWithTrustedEmptySnapFilesWhenFollowing() throws Exception {
+ QuorumUtil qu = new QuorumUtil(1);
+ try {
+ qu.startAll();
+ String connString = qu.getConnectionStringForServer(1);
+ try (ZooKeeper zk = new ZooKeeper(connString, CONNECTION_TIMEOUT, this)) {
+ for (int i = 0; i < N_TRANSACTIONS; i++) {
+ zk.create("/node-" + i, new byte[0], Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+ }
+ }
Review comment:
> The goal here is not to make the nodes write a snapshot due to hitting the snapcount. I just want to put some data in Zookeeper, since if Zookeeper is empty, it will create a snapshot file on boot, even without this change.
Okay.
(I understand the rest of the PR, and don't have any objections. I was just wondering if the `N_TRANSACTIONS` was because you were trying to hit `snapCount`, like other tests in that file seem to be doing.)
Thank you for the clarification!
----------------------------------------------------------------
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
[GitHub] [zookeeper] eolivelli commented on a change in pull request #1581: ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.emp…
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1581:
URL: https://github.com/apache/zookeeper/pull/1581#discussion_r560066468
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
##########
@@ -530,6 +530,10 @@ public void takeSnapshot(boolean syncSnap) {
ServerMetrics.getMetrics().SNAPSHOT_TIME.add(elapsed);
}
+ public boolean shouldForceWriteInitialSnapshotDuring34Migration() {
Review comment:
there is no need to add 'During34Migration', we can just leave a javadoc
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/EmptiedSnapshotRecoveryTest.java
##########
@@ -143,6 +144,48 @@ public void testRestoreWithTrustedEmptySnapFiles() throws Exception {
runTest(false, true);
}
+ @Test
+ public void testRestoreWithTrustedEmptySnapFilesWhenFollowing() throws Exception {
+ QuorumUtil qu = new QuorumUtil(1);
+ try {
+ qu.startAll();
+ String connString = qu.getConnectionStringForServer(1);
+ try (ZooKeeper zk = new ZooKeeper(connString, CONNECTION_TIMEOUT, this)) {
+ for (int i = 0; i < N_TRANSACTIONS; i++) {
+ zk.create("/node-" + i, new byte[0], Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+ }
+ }
+ int leaderIndex = qu.getLeaderServer();
+ //Shut down the cluster and delete the snapshots from the followers
+ for (int i = 1; i <= qu.ALL; i++) {
+ qu.shutdown(i);
+ if (i != leaderIndex) {
+ FileTxnSnapLog txnLogFactory = qu.getPeer(i).peer.getTxnFactory();
+ List<File> snapshots = txnLogFactory.findNRecentSnapshots(10);
+ assertTrue(snapshots.size() > 0, "We have a snapshot to corrupt");
+ for (File file : snapshots) {
+ Files.deleteIfExists(file.toPath());
Review comment:
we are sure that the file exists, we can just use 'delete', correct ?
----------------------------------------------------------------
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
[GitHub] [zookeeper] srdo commented on pull request #1581: ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.emp…
Posted by GitBox <gi...@apache.org>.
srdo commented on pull request #1581:
URL: https://github.com/apache/zookeeper/pull/1581#issuecomment-786077037
Turns out it was just a flaky test. I think this should be ready to go in now. I'm not sure what your backporting policy is like, but it would be nice to get on as many branches as possible :)
----------------------------------------------------------------
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
[GitHub] [zookeeper] ztzg commented on pull request #1581: ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.emp…
Posted by GitBox <gi...@apache.org>.
ztzg commented on pull request #1581:
URL: https://github.com/apache/zookeeper/pull/1581#issuecomment-780699848
> @ztzg please consider adding this to 3.7.
Okay, noted.
----------------------------------------------------------------
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
[GitHub] [zookeeper] ztzg commented on a change in pull request #1581: ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.emp…
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1581:
URL: https://github.com/apache/zookeeper/pull/1581#discussion_r577876677
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/EmptiedSnapshotRecoveryTest.java
##########
@@ -143,6 +144,48 @@ public void testRestoreWithTrustedEmptySnapFiles() throws Exception {
runTest(false, true);
}
+ @Test
+ public void testRestoreWithTrustedEmptySnapFilesWhenFollowing() throws Exception {
+ QuorumUtil qu = new QuorumUtil(1);
+ try {
+ qu.startAll();
+ String connString = qu.getConnectionStringForServer(1);
+ try (ZooKeeper zk = new ZooKeeper(connString, CONNECTION_TIMEOUT, this)) {
+ for (int i = 0; i < N_TRANSACTIONS; i++) {
+ zk.create("/node-" + i, new byte[0], Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+ }
+ }
Review comment:
Right; we are on the same page :)
Would you mind rebasing this on top of the latest `master`, so that CI picks the new configuration and re-runs the test suite with a higher chance of success?
----------------------------------------------------------------
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
[GitHub] [zookeeper] srdo commented on a change in pull request #1581: ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.emp…
Posted by GitBox <gi...@apache.org>.
srdo commented on a change in pull request #1581:
URL: https://github.com/apache/zookeeper/pull/1581#discussion_r577807308
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/EmptiedSnapshotRecoveryTest.java
##########
@@ -143,6 +144,48 @@ public void testRestoreWithTrustedEmptySnapFiles() throws Exception {
runTest(false, true);
}
+ @Test
+ public void testRestoreWithTrustedEmptySnapFilesWhenFollowing() throws Exception {
+ QuorumUtil qu = new QuorumUtil(1);
+ try {
+ qu.startAll();
+ String connString = qu.getConnectionStringForServer(1);
+ try (ZooKeeper zk = new ZooKeeper(connString, CONNECTION_TIMEOUT, this)) {
+ for (int i = 0; i < N_TRANSACTIONS; i++) {
+ zk.create("/node-" + i, new byte[0], Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+ }
+ }
Review comment:
The goal here is not to make the nodes write a snapshot due to hitting the snapcount. I just want to put some data in Zookeeper, since if Zookeeper is empty, it will create a snapshot file on boot, even without this change.
The behavior this PR changes is that if Zookeeper is booted with the trust empty property set to true, and the cluster is not empty, then the follower nodes will write a snapshot file if they don't already have one.
The issue with the code as it is without this change is that if you have a 3.4 cluster with low traffic and you then enable ZOOKEEPER_SNAPSHOT_TRUST_EMPTY and upgrade to 3.5+, then it may take a very long time for follower nodes to write snapshots, since they only do it if they become leader, or if Zookeeper is empty, or if the snapcount is hit.
I have added a few lines to the test to assert that the data created here is still there once the followers come up, that seems like a healthy thing to check.
----------------------------------------------------------------
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
[GitHub] [zookeeper] srdo commented on a change in pull request #1581: ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.emp…
Posted by GitBox <gi...@apache.org>.
srdo commented on a change in pull request #1581:
URL: https://github.com/apache/zookeeper/pull/1581#discussion_r577894017
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/EmptiedSnapshotRecoveryTest.java
##########
@@ -143,6 +144,48 @@ public void testRestoreWithTrustedEmptySnapFiles() throws Exception {
runTest(false, true);
}
+ @Test
+ public void testRestoreWithTrustedEmptySnapFilesWhenFollowing() throws Exception {
+ QuorumUtil qu = new QuorumUtil(1);
+ try {
+ qu.startAll();
+ String connString = qu.getConnectionStringForServer(1);
+ try (ZooKeeper zk = new ZooKeeper(connString, CONNECTION_TIMEOUT, this)) {
+ for (int i = 0; i < N_TRANSACTIONS; i++) {
+ zk.create("/node-" + i, new byte[0], Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+ }
+ }
Review comment:
Sure. Rebased and squashed. FWIW I ran the two failing tests locally, and they passed, so am guessing it's just flakiness.
----------------------------------------------------------------
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
[GitHub] [zookeeper] srdo commented on a change in pull request #1581: ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.emp…
Posted by GitBox <gi...@apache.org>.
srdo commented on a change in pull request #1581:
URL: https://github.com/apache/zookeeper/pull/1581#discussion_r560072108
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/EmptiedSnapshotRecoveryTest.java
##########
@@ -143,6 +144,48 @@ public void testRestoreWithTrustedEmptySnapFiles() throws Exception {
runTest(false, true);
}
+ @Test
+ public void testRestoreWithTrustedEmptySnapFilesWhenFollowing() throws Exception {
+ QuorumUtil qu = new QuorumUtil(1);
+ try {
+ qu.startAll();
+ String connString = qu.getConnectionStringForServer(1);
+ try (ZooKeeper zk = new ZooKeeper(connString, CONNECTION_TIMEOUT, this)) {
+ for (int i = 0; i < N_TRANSACTIONS; i++) {
+ zk.create("/node-" + i, new byte[0], Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+ }
+ }
+ int leaderIndex = qu.getLeaderServer();
+ //Shut down the cluster and delete the snapshots from the followers
+ for (int i = 1; i <= qu.ALL; i++) {
+ qu.shutdown(i);
+ if (i != leaderIndex) {
+ FileTxnSnapLog txnLogFactory = qu.getPeer(i).peer.getTxnFactory();
+ List<File> snapshots = txnLogFactory.findNRecentSnapshots(10);
+ assertTrue(snapshots.size() > 0, "We have a snapshot to corrupt");
+ for (File file : snapshots) {
+ Files.deleteIfExists(file.toPath());
Review comment:
Yes, will fix
----------------------------------------------------------------
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
[GitHub] [zookeeper] ztzg commented on a change in pull request #1581: ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.emp…
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1581:
URL: https://github.com/apache/zookeeper/pull/1581#discussion_r577767628
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/EmptiedSnapshotRecoveryTest.java
##########
@@ -143,6 +144,48 @@ public void testRestoreWithTrustedEmptySnapFiles() throws Exception {
runTest(false, true);
}
+ @Test
+ public void testRestoreWithTrustedEmptySnapFilesWhenFollowing() throws Exception {
+ QuorumUtil qu = new QuorumUtil(1);
+ try {
+ qu.startAll();
+ String connString = qu.getConnectionStringForServer(1);
+ try (ZooKeeper zk = new ZooKeeper(connString, CONNECTION_TIMEOUT, this)) {
+ for (int i = 0; i < N_TRANSACTIONS; i++) {
+ zk.create("/node-" + i, new byte[0], Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+ }
+ }
Review comment:
This section creates transactions, ensuring a reasonable `xzid`, but might *not* result in a snapshot, as it doesn't lower the `snapCount` using `SyncRequestProcessor.setSnapCount`. (The other tests do, and might run before. Might be a good idea to be explicit if the goal of running `N_TRANSACTIONS` is to create one?)
This does *not* mean that the test is broken: a snapshot is present, but it is there because of the `initialize` file left behind by `ClientBase.createTmpDir`.
----------------------------------------------------------------
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
[GitHub] [zookeeper] srdo commented on a change in pull request #1581: ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.emp…
Posted by GitBox <gi...@apache.org>.
srdo commented on a change in pull request #1581:
URL: https://github.com/apache/zookeeper/pull/1581#discussion_r577807308
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/EmptiedSnapshotRecoveryTest.java
##########
@@ -143,6 +144,48 @@ public void testRestoreWithTrustedEmptySnapFiles() throws Exception {
runTest(false, true);
}
+ @Test
+ public void testRestoreWithTrustedEmptySnapFilesWhenFollowing() throws Exception {
+ QuorumUtil qu = new QuorumUtil(1);
+ try {
+ qu.startAll();
+ String connString = qu.getConnectionStringForServer(1);
+ try (ZooKeeper zk = new ZooKeeper(connString, CONNECTION_TIMEOUT, this)) {
+ for (int i = 0; i < N_TRANSACTIONS; i++) {
+ zk.create("/node-" + i, new byte[0], Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+ }
+ }
Review comment:
The goal here is not to make the nodes write a snapshot due to hitting the snapcount. I just want to put some data in Zookeeper, since if Zookeeper is empty, it will create a snapshot file on boot, even without this change.
The behavior this PR changes is that if Zookeeper is booted with the trust empty property set to true, and the cluster is not empty, then the follower nodes will write a snapshot file if they don't already have one.
The issue with the code as it is without this change is that if you have a 3.4 cluster with low traffic and you then enable ZOOKEEPER_SNAPSHOT_TRUST_EMPTY and upgrade to 3.5+, then it may take a very long time for follower nodes to write snapshots, since they only do it if they become leader, or if Zookeeper is empty, or if the snapcount is hit. That makes ZOOKEEPER_SNAPSHOT_TRUST_EMPTY pretty inconvenient, since you can't remove that property again until all nodes have at least one snapshot.
I have added a few lines to the test to assert that the data created here is still there once the followers come up, that seems like a healthy thing to check.
----------------------------------------------------------------
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
[GitHub] [zookeeper] srdo commented on a change in pull request #1581: ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.emp…
Posted by GitBox <gi...@apache.org>.
srdo commented on a change in pull request #1581:
URL: https://github.com/apache/zookeeper/pull/1581#discussion_r577807308
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/EmptiedSnapshotRecoveryTest.java
##########
@@ -143,6 +144,48 @@ public void testRestoreWithTrustedEmptySnapFiles() throws Exception {
runTest(false, true);
}
+ @Test
+ public void testRestoreWithTrustedEmptySnapFilesWhenFollowing() throws Exception {
+ QuorumUtil qu = new QuorumUtil(1);
+ try {
+ qu.startAll();
+ String connString = qu.getConnectionStringForServer(1);
+ try (ZooKeeper zk = new ZooKeeper(connString, CONNECTION_TIMEOUT, this)) {
+ for (int i = 0; i < N_TRANSACTIONS; i++) {
+ zk.create("/node-" + i, new byte[0], Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+ }
+ }
Review comment:
The goal here is not to make the nodes write a snapshot. I just want to put some data in Zookeeper, since if Zookeeper is empty, it will create a snapshot file on boot, even without this change.
The behavior this PR changes is that if Zookeeper is booted with the trust empty property set to true, and the cluster is not empty, then the follower nodes will write a snapshot file if they don't already have one.
I have added a few lines to the test to assert that the data created here is still there once the followers come up, that seems like a healthy thing to check.
----------------------------------------------------------------
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
[GitHub] [zookeeper] srdo commented on pull request #1581: ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.emp…
Posted by GitBox <gi...@apache.org>.
srdo commented on pull request #1581:
URL: https://github.com/apache/zookeeper/pull/1581#issuecomment-781620252
The failing test is a different one this time than last time. I think it's just flaky.
----------------------------------------------------------------
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
[GitHub] [zookeeper] srdo commented on a change in pull request #1581: ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.emp…
Posted by GitBox <gi...@apache.org>.
srdo commented on a change in pull request #1581:
URL: https://github.com/apache/zookeeper/pull/1581#discussion_r577907599
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/EmptiedSnapshotRecoveryTest.java
##########
@@ -143,6 +144,48 @@ public void testRestoreWithTrustedEmptySnapFiles() throws Exception {
runTest(false, true);
}
+ @Test
+ public void testRestoreWithTrustedEmptySnapFilesWhenFollowing() throws Exception {
+ QuorumUtil qu = new QuorumUtil(1);
+ try {
+ qu.startAll();
+ String connString = qu.getConnectionStringForServer(1);
+ try (ZooKeeper zk = new ZooKeeper(connString, CONNECTION_TIMEOUT, this)) {
+ for (int i = 0; i < N_TRANSACTIONS; i++) {
+ zk.create("/node-" + i, new byte[0], Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+ }
+ }
Review comment:
Also just a note on rebasing on top of master, since the behavior surprised me, and I thought I'd share: GH Actions' checkout action does not check out the PR branch, it instead creates a merge commit between the PR branch and the target branch, which it then runs the rest of the CI workflow on. So likely the previous run did run with the config from master.
----------------------------------------------------------------
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
[GitHub] [zookeeper] ztzg commented on pull request #1581: ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.emp…
Posted by GitBox <gi...@apache.org>.
ztzg commented on pull request #1581:
URL: https://github.com/apache/zookeeper/pull/1581#issuecomment-792039476
Thank you, @srdo! This is now in `master`, `branch-3.7`, and `branch-3.7.0`.
----------------------------------------------------------------
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
[GitHub] [zookeeper] srdo commented on a change in pull request #1581: ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.emp…
Posted by GitBox <gi...@apache.org>.
srdo commented on a change in pull request #1581:
URL: https://github.com/apache/zookeeper/pull/1581#discussion_r577812276
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/EmptiedSnapshotRecoveryTest.java
##########
@@ -143,6 +144,48 @@ public void testRestoreWithTrustedEmptySnapFiles() throws Exception {
runTest(false, true);
}
+ @Test
+ public void testRestoreWithTrustedEmptySnapFilesWhenFollowing() throws Exception {
+ QuorumUtil qu = new QuorumUtil(1);
+ try {
+ qu.startAll();
+ String connString = qu.getConnectionStringForServer(1);
+ try (ZooKeeper zk = new ZooKeeper(connString, CONNECTION_TIMEOUT, this)) {
+ for (int i = 0; i < N_TRANSACTIONS; i++) {
+ zk.create("/node-" + i, new byte[0], Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+ }
+ }
Review comment:
I guess I could use another number than N_TRANSACTIONS if it is misleading?
----------------------------------------------------------------
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
[GitHub] [zookeeper] srdo commented on a change in pull request #1581: ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.emp…
Posted by GitBox <gi...@apache.org>.
srdo commented on a change in pull request #1581:
URL: https://github.com/apache/zookeeper/pull/1581#discussion_r577814854
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/EmptiedSnapshotRecoveryTest.java
##########
@@ -143,6 +144,48 @@ public void testRestoreWithTrustedEmptySnapFiles() throws Exception {
runTest(false, true);
}
+ @Test
+ public void testRestoreWithTrustedEmptySnapFilesWhenFollowing() throws Exception {
+ QuorumUtil qu = new QuorumUtil(1);
+ try {
+ qu.startAll();
+ String connString = qu.getConnectionStringForServer(1);
+ try (ZooKeeper zk = new ZooKeeper(connString, CONNECTION_TIMEOUT, this)) {
+ for (int i = 0; i < N_TRANSACTIONS; i++) {
+ zk.create("/node-" + i, new byte[0], Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+ }
+ }
Review comment:
Just to summarize what this test is intended to do:
It is supposed to set up a quorum with some data in it. It then deletes the snapshots on all follower nodes. This simulates what a low-traffic cluster being upgraded from 3.4 might look like. The cluster is restarted with ZOOKEEPER_SNAPSHOT_TRUST_EMPTY=true, and the test verifies that all nodes get a snapshot written. This will allow the operator to remove ZOOKEEPER_SNAPSHOT_TRUST_EMPTY again and reboot the cluster shortly after, easing migration.
----------------------------------------------------------------
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
[GitHub] [zookeeper] srdo commented on pull request #1581: ZOOKEEPER-3781: Create snapshots on followers when snapshot.trust.emp…
Posted by GitBox <gi...@apache.org>.
srdo commented on pull request #1581:
URL: https://github.com/apache/zookeeper/pull/1581#issuecomment-768323461
@eolivelli I addressed your comments (Sorry for pinging, I'm not sure Github sends messages when I push updates)
----------------------------------------------------------------
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