[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...

2018-11-12 Thread asfgit
Github user asfgit closed the pull request at:

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


---


[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...

2018-08-17 Thread suyogmapara
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 pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...

2018-08-13 Thread anmolnar
Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/567#discussion_r209526014
  
--- Diff: zookeeper-docs/src/documentation/content/xdocs/zookeeperAdmin.xml 
---
@@ -840,6 +840,26 @@ server.3=zoo3:2888:3888
 
   
 
+  
+txnLogSizeLimitInKb
+
+
+  (Java system property: zookeeper.txnLogSizeLimitInKb)
+
+  Zookeeper transaction log file can also be controlled 
more
+  directly using txnLogSizeLimitInKb. Larger txn logs can lead 
to
+  slower follower syncs when sync is done using transaction 
log.
+  This is because leader has to scan through the appropriate 
log
+  file on disk to find the transaction to start sync from.
+  This feature is turned off by this default and snapCount is 
the
--- End diff --

I think it's okay to update HTML and PDF too. As long as they're part of 
the repo, we should keep them up-to-date.


---


[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...

2018-08-12 Thread maoling
Github user maoling commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/567#discussion_r209484474
  
--- 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 --

import *  ?


---


[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...

2018-08-12 Thread maoling
Github user maoling commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/567#discussion_r209484281
  
--- 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 --

add a unit: kb in the log is better?


---


[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...

2018-08-12 Thread maoling
Github user maoling commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/567#discussion_r209484163
  
--- Diff: zookeeper-docs/src/documentation/content/xdocs/zookeeperAdmin.xml 
---
@@ -840,6 +840,26 @@ server.3=zoo3:2888:3888
 
   
 
+  
+txnLogSizeLimitInKb
+
+
+  (Java system property: zookeeper.txnLogSizeLimitInKb)
+
+  Zookeeper transaction log file can also be controlled 
more
+  directly using txnLogSizeLimitInKb. Larger txn logs can lead 
to
+  slower follower syncs when sync is done using transaction 
log.
+  This is because leader has to scan through the appropriate 
log
+  file on disk to find the transaction to start sync from.
+  This feature is turned off by this default and snapCount is 
the
--- End diff --

> This is because leader has to scan through the appropriate log file on 
disk to find the transaction to start sync from.


Could you plz explain it for me? when this happens?

BTW:only change zookeeperAdmin.xml is ok,let html,pdf go


---


[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...

2018-07-30 Thread suyogmapara
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...

2018-07-27 Thread anmolnar
Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/567#discussion_r205933566
  
--- 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 --

I mean `zookeeperAdmin.xml`.
Html and pdf files are generated from xml.


---


[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...

2018-07-27 Thread anmolnar
Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/567#discussion_r205933558
  
--- 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 --

Yes. This new feature should be documented in zookeeperAdmin. Are you 
planning to address docs here or in separate patch?


---


[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...

2018-07-27 Thread suyogmapara
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...

2018-07-27 Thread suyogmapara
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 pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...

2018-07-27 Thread suyogmapara
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 pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...

2018-07-26 Thread breed
Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/567#discussion_r205667420
  
--- 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 --

we should also add this property to the doc.


---


[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...

2018-07-26 Thread breed
Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/567#discussion_r205667343
  
--- 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 --

shouldn't it be txnLogSizeLimitInKb?


---


[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...

2018-07-26 Thread breed
Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/567#discussion_r205667305
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java ---
@@ -102,6 +102,20 @@
 /** 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]
+ *
+ * The feature is disabled by default (-1)
+ */
+public static final String LOG_SIZE_LIMIT = 
"zookeeper.txnlogSizeLimit";
+
+/**
+ * The actual txnlog size limit in bytes.
+ */
+public static long logSizeLimit = -1;
+
--- End diff --

good points @maoling ! is there a reason these two variables should be 
public? it would be nice to make the variable have a similar name to the 
property.


---


[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...

2018-07-26 Thread maoling
Github user maoling commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/567#discussion_r205526853
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java ---
@@ -102,6 +102,20 @@
 /** 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]
+ *
+ * The feature is disabled by default (-1)
+ */
+public static final String LOG_SIZE_LIMIT = 
"zookeeper.txnlogSizeLimit";
+
+/**
+ * The actual txnlog size limit in bytes.
+ */
+public static long logSizeLimit = -1;
+
--- End diff --

private? txnLogSizeLimit is better?


---


[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...

2018-07-26 Thread maoling
Github user maoling commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/567#discussion_r205525593
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java ---
@@ -109,7 +109,7 @@
  *
  * The feature is disabled by default (-1)
  */
-public static final String LOG_SIZE_LIMIT = 
"zookeeper.txnlogSizeLimit";
+public static final String LOG_SIZE_LIMIT = 
"zookeeper.txnlogSizeLimitInKb";
--- End diff --

private?


---


[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...

2018-07-26 Thread maoling
Github user maoling commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/567#discussion_r205527457
  
--- 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 --

the unit tests in the `TxnLogSizeLimitTest` can be moved to 
`FileTxnLogTest`?


---


[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...

2018-07-26 Thread maoling
Github user maoling commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/567#discussion_r205526028
  
--- 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 --

{} value is {} Bytes


---


[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...

2018-07-16 Thread suyogmapara
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...

2018-07-16 Thread suyogmapara
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...

2018-07-13 Thread anmolnar
Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/567#discussion_r202364176
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java ---
@@ -347,6 +397,13 @@ public synchronized void commit() throws IOException {
 while (streamsToFlush.size() > 1) {
 streamsToFlush.removeFirst().close();
 }
+
+// Roll the log file if we exceed the size limit
+long logSize = getCurrentLogSize();
+if ((logSizeLimit > 0) && (logSize > logSizeLimit)) {
+LOG.debug("Log size limit reached: " + logSize);
--- End diff --

Use message formatter.


---


[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...

2018-07-13 Thread anmolnar
Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/567#discussion_r202363134
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java ---
@@ -102,6 +102,20 @@
 /** 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]
+ *
+ * The feature is disabled by default (-1)
+ */
+public static final String LOG_SIZE_LIMIT = 
"zookeeper.txnlogSizeLimit";
--- End diff --

I think it would be nice to indicate the unit in the name of the setting. 
e.g. `txnlogSizeLimitInKb`


---


[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...

2018-07-13 Thread anmolnar
Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/567#discussion_r202364713
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java ---
@@ -347,6 +397,13 @@ public synchronized void commit() throws IOException {
 while (streamsToFlush.size() > 1) {
 streamsToFlush.removeFirst().close();
 }
+
+// Roll the log file if we exceed the size limit
+long logSize = getCurrentLogSize();
--- End diff --

Please change the order of the if conditions. You probably don't want to 
calculate the current log file size if the feature is disabled. Probably not a 
big deal performance-wise, but I think it's cleaner code.


---


[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...

2018-07-13 Thread anmolnar
Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/567#discussion_r202363520
  
--- 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 --

Please indicate the unit in the message (you converted the value to bytes) 
and use log4j message formatter.


---


[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...

2018-07-13 Thread anmolnar
Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/567#discussion_r202362761
  
--- 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 --

language: "less than ..."


---


[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...

2018-07-13 Thread anmolnar
Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/567#discussion_r202364057
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java ---
@@ -347,6 +397,13 @@ public synchronized void commit() throws IOException {
 while (streamsToFlush.size() > 1) {
 streamsToFlush.removeFirst().close();
 }
+
+// Roll the log file if we exceed the size limit
+long logSize = getCurrentLogSize();
+if ((logSizeLimit > 0) && (logSize > logSizeLimit)) {
--- End diff --

Redundant inner brackets.


---


[GitHub] zookeeper pull request #567: ZOOKEEPER-3071: Add a config parameter to contr...

2018-07-12 Thread suyogmapara
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




---