hanm commented on a change in pull request #1191:
URL: https://github.com/apache/zookeeper/pull/1191#discussion_r428433876
##########
File path:
zookeeper-server/src/main/java/org/apache/zookeeper/server/SyncRequestProcessor.java
##########
@@ -178,27 +226,22 @@ public void run() {
ServerMetrics.getMetrics().SYNC_PROCESSOR_QUEUE_TIME.add(startProcessTime -
si.syncQueueStartTime);
// track the number of records written to the log
- if (!si.isThrottled() && zks.getZKDatabase().append(si)) {
- if (shouldSnapshot()) {
- resetSnapshotStats();
+ if (!si.isThrottled() && zkDB.append(si)) {
+ if (selfSnapTxnRollThreshold.meet(zkDB.getTxnCount(),
zkDB.getTxnSize())) {
// roll the log
- zks.getZKDatabase().rollLog();
- // take a snapshot
- if (!snapThreadMutex.tryAcquire()) {
- LOG.warn("Too busy to snap, skipping");
+ zkDB.rollLog();
+
+ if (onlySnapWhenSafetyIsThreatened) {
+ if
(safetySnapThreshold.meet(zkDB.getTxnsSinceLastSnap(),
zkDB.getTxnsSizeSinceLastSnap())) {
+ snapGenerator.takeSnapshot(false);
Review comment:
also if we want to hard code a value, it seems true is more safe than
false here for durability guarantees - but i lose track of what our default
options were when taking snapshot (fsync it or not, especially if IIRC we lost
the fsync parameter when introducing SnapStream..)
##########
File path: zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md
##########
@@ -1156,6 +1156,76 @@ property, when available, is noted below.
The default value is false.
+* *leader.snapPingIntervalInSeconds*
+ (Java system property only: **zookeeper.leader.snapPingIntervalInSeconds**)
Review comment:
are these really java system properties only? Put a config foo in
zoo.cfg and ZK will parse them and generate a zookeeoer.foo. similar for other
"java only system properties" listed here. might need update doc.
##########
File path:
zookeeper-server/src/main/java/org/apache/zookeeper/server/SnapshotGenerator.java
##########
@@ -0,0 +1,105 @@
+/**
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.server;
+
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.atomic.AtomicBoolean;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Util class Used to control the behavior abouthow we take snapshot.
+ */
+public class SnapshotGenerator {
+ private static final Logger LOG =
LoggerFactory.getLogger(SnapshotGenerator.class);
+
+ public static final String PURGE_AFTER_SNAPSHOT =
"zookeeper.purgeAfterSnapshot.enabled";
+ private static boolean purgeAfterSnapshot;
+
+ public static final String FSYNC_SNAPSHOT_FROM_SCHEDULER =
"zookeeper.fsyncSnapshotFromScheduler";
+ private static boolean fsyncSnapshotFromScheduler;
+
+ static {
+ purgeAfterSnapshot = Boolean.getBoolean(PURGE_AFTER_SNAPSHOT);
+ LOG.info("{} = {}", PURGE_AFTER_SNAPSHOT, purgeAfterSnapshot);
+
+ fsyncSnapshotFromScheduler = Boolean.parseBoolean(
Review comment:
why not use Boolean.getBoolean so it's consistent with previous property
parsing code (also less verbose)?
##########
File path:
zookeeper-server/src/main/java/org/apache/zookeeper/server/SyncRequestProcessor.java
##########
@@ -178,27 +226,22 @@ public void run() {
ServerMetrics.getMetrics().SYNC_PROCESSOR_QUEUE_TIME.add(startProcessTime -
si.syncQueueStartTime);
// track the number of records written to the log
- if (!si.isThrottled() && zks.getZKDatabase().append(si)) {
- if (shouldSnapshot()) {
- resetSnapshotStats();
+ if (!si.isThrottled() && zkDB.append(si)) {
+ if (selfSnapTxnRollThreshold.meet(zkDB.getTxnCount(),
zkDB.getTxnSize())) {
// roll the log
- zks.getZKDatabase().rollLog();
- // take a snapshot
- if (!snapThreadMutex.tryAcquire()) {
- LOG.warn("Too busy to snap, skipping");
+ zkDB.rollLog();
+
+ if (onlySnapWhenSafetyIsThreatened) {
+ if
(safetySnapThreshold.meet(zkDB.getTxnsSinceLastSnap(),
zkDB.getTxnsSizeSinceLastSnap())) {
+ snapGenerator.takeSnapshot(false);
Review comment:
should we pass snapGenerator.fsyncSnapshotFromScheduler as parameter
here instead of hardcoding a false?
##########
File path:
zookeeper-server/src/main/java/org/apache/zookeeper/server/SyncRequestProcessor.java
##########
@@ -178,27 +226,22 @@ public void run() {
ServerMetrics.getMetrics().SYNC_PROCESSOR_QUEUE_TIME.add(startProcessTime -
si.syncQueueStartTime);
// track the number of records written to the log
- if (!si.isThrottled() && zks.getZKDatabase().append(si)) {
- if (shouldSnapshot()) {
- resetSnapshotStats();
+ if (!si.isThrottled() && zkDB.append(si)) {
+ if (selfSnapTxnRollThreshold.meet(zkDB.getTxnCount(),
zkDB.getTxnSize())) {
// roll the log
- zks.getZKDatabase().rollLog();
- // take a snapshot
- if (!snapThreadMutex.tryAcquire()) {
- LOG.warn("Too busy to snap, skipping");
+ zkDB.rollLog();
+
+ if (onlySnapWhenSafetyIsThreatened) {
+ if
(safetySnapThreshold.meet(zkDB.getTxnsSinceLastSnap(),
zkDB.getTxnsSizeSinceLastSnap())) {
+ snapGenerator.takeSnapshot(false);
+
ServerMetrics.getMetrics().TAKING_SAFE_SNAPSHOT.add(1);
+ LOG.info("The txns size since last snapshot is
larger than 10x, "
+ + "going to take a snapshot for
safe.");
+ }
} else {
- new ZooKeeperThread("Snapshot Thread") {
- public void run() {
- try {
- zks.takeSnapshot();
- } catch (Exception e) {
- LOG.warn("Unexpected exception", e);
- } finally {
- snapThreadMutex.release();
- }
- }
- }.start();
+ snapGenerator.takeSnapshot(false);
Review comment:
similar here - should the parameter be hardcoded or taken from
fsyncSnapshotFromScheduler
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/ZooDefs.java
##########
@@ -32,6 +32,14 @@
public static final String ZOOKEEPER_NODE_SUBTREE = "/zookeeper/";
+ /**
+ * WARN: please don't retain the order, which is used to check
Review comment:
did you mean "don't change" or "retain", as opposed to "don't retain"?
my understanding is the order must be preserved here for the code to work.
##########
File path:
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Follower.java
##########
@@ -150,13 +157,95 @@ void followLeader() throws InterruptedException {
}
}
+ // Visible for testing
+ protected void snapPing(QuorumPacket qp) throws IOException {
+ int peerSnapPingVersion = -1;
+ long snapPingId = -1;
+ int snapCode = -1;
+ try {
+ ByteArrayInputStream bis = new ByteArrayInputStream(qp.getData());
+ DataInputStream dis = new DataInputStream(bis);
+ peerSnapPingVersion = dis.readInt();
+ snapPingId = dis.readLong();
+ snapCode = dis.readInt();
+ } catch (IOException e) {
+ LOG.error("Error while parsing SnapPing packet", e);
+ return;
+ }
+
+ if (peerSnapPingVersion != SnapPingManager.SNAP_PING_VERSION) {
+ LOG.warn("The SnapPing version on leader is {} which is not "
+ + "compatible with ours {}, will skip",
peerSnapPingVersion,
+ SnapPingManager.SNAP_PING_VERSION);
+ if (fzk.syncProcessor.isOnlySnapWhenSafetyIsThreatened()) {
+ LOG.info("SnapPing version incompatible, start self snapshot");
+ fzk.syncProcessor.setOnlySnapWhenSafetyIsThreatened(false);
+ }
+ return;
+ }
+
+ if (snapCode == SnapPingCode.CANCEL.ordinal()) {
+ if (fzk.syncProcessor.isOnlySnapWhenSafetyIsThreatened()) {
+ LOG.info("Snapshot schedule cancelled by leader, start self
snapshot");
+ fzk.syncProcessor.setOnlySnapWhenSafetyIsThreatened(false);
Review comment:
did we consider that the CANCEL packet could potentially lost so a
server in schedule snap mode will never be snapping again? Is there any built
in defense mechanism for that case (didn't read all part of code yet) to make
sure a server will not end up in not snapping state?
----------------------------------------------------------------
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
