[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user suyogmapara commented on the issue: https://github.com/apache/zookeeper/pull/567 Hi @anmolnar, do I need to do anything before this can be merged? ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user suyogmapara commented on the issue: https://github.com/apache/zookeeper/pull/567 @tamaashu can you please take a look and approve? ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user suyogmapara commented on the issue: https://github.com/apache/zookeeper/pull/567 Thanks @anmolnar, Hi @tamaashu, can you please let me know how should I move my documentation changes? ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user suyogmapara commented on the issue: https://github.com/apache/zookeeper/pull/567 Hi @anmolnar please take a look at updated documentation. This time after regeneration, other parts of the documentation has changed. I have also rebased on the latest upstream/master ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user suyogmapara commented on the issue: https://github.com/apache/zookeeper/pull/567 Hi @breed @anmolnar, can you please take a look at this PR? ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user suyogmapara commented on the issue: https://github.com/apache/zookeeper/pull/567 @anmolnar thank you, I will surely update the docs with some more details. I am currently waiting to see if there is anything else I need to address in the next iteration. ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user suyogmapara commented on the issue: https://github.com/apache/zookeeper/pull/567 Hi @maoling Regarding 2, I agree that this would be the outcome. But sorry it is not clear to me why do you think that the result is surprising. You are setting log-size-limit slightly larger than the pre-alloc size and so you would see that actual log size is twice the preAllocSize. Regarding 1, it seems to me that we configured zookeeper to perform poorly and so it would do poorly. It is similar to setting snapCount = 1.As I said above, I can explicitly prevent this case if you guys think such a validation is useful. As a side note, example of a good config would be preAllocSize = 64 MB, txnLogSizeLimit = 512 MB. Regarding 3, 1 MB is a quite large and typical configuration of snapCount would mean we will have txnLogs of several GBs. Regarding 4, txnLogSizeLimitInKb controls maximum size of the transaction log. I believe there is a separate parameter that controls batch commit size on observers. ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user suyogmapara commented on the issue: https://github.com/apache/zookeeper/pull/567 Hi @maoling I tested the patch with following parameters: 1. PreAllocSize = 100 KB, TxnLogSizeLimit = 200 KB, 100 transactions of little over 10 KBs. Outcome: 5 transaction files with little over 200 KBs each (rounded up to last transaction). 2. PreAllocSize = 100 KB, TxnLogSizeLimit = 100 KB, 100 transactions of little over 10 KBs. Outcome: 100 transaction files one per each transaction 3. PreAllocSize = 100 KB, TxnLogSizeLimit = 50 KB, 100 transactions of little over 10 KBs. Outcome: 100 transaction files one per each transaction. Second and third outcomes are expected as every time it tries to pre-allocate more, we would be rolling the log. Of course those won't be sensible configurations, but if someone uses them, only performance would be impacted and not the correctness. Please let me know if you disagree. @anmolnar snapCount doesn't give direct handle on the transaction log size. Changing average transaction size from 1 KB to 100 KB would increase the transaction log size by 2 orders of magnitude and may push they sync time over the edge. More importantly reducing the snapCount would also increase how frequently we snapshot and so we would be forced to find a sweet spot to balance the two conflicting goals. On the other hand, txnLogSizeLimit provides cheap and independent control on the worst case seek time during the diff sync. ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user suyogmapara commented on the issue: https://github.com/apache/zookeeper/pull/567 Hi @maoling Regarding 1, preAllocSize controls how big the contiguous region of transaction file would be on disk (to reduce number seeks you need to do) while this feature controls how big the file size would be before we roll to the next transaction log. It would make sense to set later one as some multiple of the first one. If you set both values closers or if preAllocSize is smaller, you are right you would get very frequent log rolls (maybe for every transaction in 1.2), but as far as I can think of it shouldn't impact correctness. I don't have checks against such values mainly because it is not clear what is the minimum value I should allow. Basically the same reason we don't have check against setting snapCount = 2, for example. Regarding your example, can you please elaborate your values in 1.1? Regarding 2, during diff sync, leader will seek for the last transaction follower has seen in the in-memory committed log cache. If the zxid is older than oldest transaction then leader has to first send over the transactions from the earlier transaction log file. For starting the stream of transaction, leader has to do a linear search for the zxid, in the transaction log (as we don't have an index over it). Larger the transaction log file, longer it takes. It can happen if you have especially larger individual transactions. This feature give you control over the maximum size. ---
[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...
Github user suyogmapara commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/567#discussion_r210988903 --- Diff: src/java/test/org/apache/zookeeper/server/persistence/FileTxnLogTest.java --- @@ -17,9 +17,13 @@ */ package org.apache.zookeeper.server.persistence; -import org.apache.zookeeper.ZKTestCase; -import org.apache.zookeeper.ZooDefs; +import org.apache.zookeeper.*; +import org.apache.zookeeper.data.Stat; --- End diff -- Hi @maoling do you have any more comments or I should proceed with PR. Basically I am waiting on your review :) ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user suyogmapara commented on the issue: https://github.com/apache/zookeeper/pull/567 Thanks @anmolnar for the review. Regarding randomization, I think they did it for snapCount because snapshot is potentially expensive IO and entire ensemble doing it at the same time may be an issue. This feature doesn't do snapshot when we roll the log and just switches the file we are writing to. So it may be okay if everyone do it at the same time. Please let me know if you disagree. ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user suyogmapara commented on the issue: https://github.com/apache/zookeeper/pull/567 @anmolnar I updated the documentation, please let me know if it looks alright ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user suyogmapara commented on the issue: https://github.com/apache/zookeeper/pull/567 @anmholnar Sorry I got caught up in other work, I will update this PR with the documents today. ---
[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...
Github user suyogmapara commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/567#discussion_r206338732 --- Diff: src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java --- @@ -102,6 +102,21 @@ /** Maximum time we allow for elapsed fsync before WARNing */ private final static long fsyncWarningThresholdMS; +/** + * This parameter limit the size of each txnlog to a given limit (KB). + * It does not affect how often the system will take a snapshot [zookeeper.snapCount] + * We roll the txnlog when either of the two limits are reached. + * Also since we only roll the logs at transaction boundaries, actual file size can exceed + * this limit by the maximum size of a serialized transaction. + * The feature is disabled by default (-1) + */ +public static final String LOG_SIZE_LIMIT = "zookeeper.txnlogSizeLimitInKb"; --- End diff -- Thanks @anmolnar I will update the patch soon with documentation. ---
[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...
Github user suyogmapara commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/567#discussion_r205923249 --- Diff: src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java --- @@ -102,6 +102,21 @@ /** Maximum time we allow for elapsed fsync before WARNing */ private final static long fsyncWarningThresholdMS; +/** + * This parameter limit the size of each txnlog to a given limit (KB). + * It does not affect how often the system will take a snapshot [zookeeper.snapCount] + * We roll the txnlog when either of the two limits are reached. + * Also since we only roll the logs at transaction boundaries, actual file size can exceed + * this limit by the maximum size of a serialized transaction. + * The feature is disabled by default (-1) + */ +public static final String LOG_SIZE_LIMIT = "zookeeper.txnlogSizeLimitInKb"; --- End diff -- Thanks @breed, are you referring to https://github.com/apache/zookeeper/blob/master/docs/zookeeperAdmin.html? ---
[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...
Github user suyogmapara commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/567#discussion_r205922930
--- Diff:
src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java ---
@@ -127,14 +127,11 @@
Long logSize = Long.getLong(LOG_SIZE_LIMIT, -1);
if (logSize > 0) {
+LOG.info("{} = {}", LOG_SIZE_LIMIT, logSize);
+
--- End diff --
I renamed the property as per the other comment, I think now the log is
readable as is. Please let me know if you think otherwise.
---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user suyogmapara commented on the issue: https://github.com/apache/zookeeper/pull/567 @maoling Thanks for the review, regarding concern around PreAllocSize, thanks to ZOOKEEPER-2249, setting txnLogSizeLimit less than preAllocSize should not cause any issue. I can add explicit test for it if you guys think it is valuable. ---
[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...
Github user suyogmapara commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/567#discussion_r205897728
--- Diff: src/java/test/org/apache/zookeeper/test/TxnLogSizeLimitTest.java
---
@@ -0,0 +1,173 @@
+/**
+ * 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.test;
+
+import java.io.File;
+import java.util.HashSet;
+import java.util.Random;
+
+import org.apache.log4j.Logger;
+import org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.PortAssignment;
+import org.apache.zookeeper.WatchedEvent;
+import org.apache.zookeeper.Watcher;
+import org.apache.zookeeper.ZKTestCase;
+import org.apache.zookeeper.ZooDefs.Ids;
+import org.apache.zookeeper.ZooKeeper;
+import org.apache.zookeeper.data.Stat;
+import org.apache.zookeeper.proto.CreateRequest;
+import org.apache.zookeeper.server.ServerCnxnFactory;
+import org.apache.zookeeper.server.ZKDatabase;
+import org.apache.zookeeper.server.ZooKeeperServer;
+import org.apache.zookeeper.server.persistence.FileTxnLog;
+import org.apache.zookeeper.server.persistence.FileTxnSnapLog;
+import org.apache.zookeeper.txn.TxnHeader;
+import org.junit.Assert;
+import org.junit.Test;
+
+/**
+ * Test loading committed proposal from txnlog. Learner uses these
proposals to
+ * catch-up with leader
+ */
+public class TxnLogSizeLimitTest extends ZKTestCase implements Watcher {
+private static final Logger LOG = Logger
--- End diff --
Unit tests define some constants. Would you suggest I move it to per test?
Also what is the general guideline for creating test classes?
---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user suyogmapara commented on the issue: https://github.com/apache/zookeeper/pull/567 Thanks for the review @anmolnar. Please take a look at unit tests when you get a chance. I have addressed the comments. I will also add documentation in a separate commit. ---
[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...
Github user suyogmapara commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/567#discussion_r202855015
--- Diff:
src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java ---
@@ -110,6 +124,18 @@
if ((fsyncWarningThreshold =
Long.getLong("zookeeper.fsync.warningthresholdms")) == null)
fsyncWarningThreshold =
Long.getLong("fsync.warningthresholdms", 1000);
fsyncWarningThresholdMS = fsyncWarningThreshold;
+
+Long logSize = Long.getLong(LOG_SIZE_LIMIT, -1);
+if (logSize > 0) {
+// Convert to bytes
+logSize = logSize * 1024;
+if (logSize <= preAllocSize) {
+LOG.error("Ignoring invalid txn log size limit (lesser
than preAllocSize)");
+} else {
+LOG.info(LOG_SIZE_LIMIT + "=" + logSize);
--- End diff --
Flipped the order of the two lines.
---
[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...
Github user suyogmapara commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/567#discussion_r202854476
--- Diff:
src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java ---
@@ -110,6 +124,18 @@
if ((fsyncWarningThreshold =
Long.getLong("zookeeper.fsync.warningthresholdms")) == null)
fsyncWarningThreshold =
Long.getLong("fsync.warningthresholdms", 1000);
fsyncWarningThresholdMS = fsyncWarningThreshold;
+
+Long logSize = Long.getLong(LOG_SIZE_LIMIT, -1);
+if (logSize > 0) {
+// Convert to bytes
+logSize = logSize * 1024;
+if (logSize <= preAllocSize) {
+LOG.error("Ignoring invalid txn log size limit (lesser
than preAllocSize)");
--- End diff --
Removed this line, it was due to incorrect merge.
---
[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...
GitHub user suyogmapara opened a pull request: https://github.com/apache/zookeeper/pull/567 ZOOKEEPER-3071: Add a config parameter to control transaction log size You can merge this pull request into a Git repository by running: $ git pull https://github.com/suyogmapara/zookeeper ZOOKEEPER-3071 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/567.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 #567 commit d833afec8961febc7b676e3f5346bf715f702c57 Author: Suyog Mapara Date: 2018-07-13T00:58:42Z ZOOKEEPER-3071: Add a config parameter to control transaction log size ---
