You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by cpoerschke <gi...@git.apache.org> on 2018/07/11 19:06:10 UTC
[GitHub] zookeeper pull request #566: ZOOKEEPER-3062: mention fsync.warningthresholdm...
GitHub user cpoerschke opened a pull request:
https://github.com/apache/zookeeper/pull/566
ZOOKEEPER-3062: mention fsync.warningthresholdms in FileTxnLog LOG.warn message
https://issues.apache.org/jira/browse/ZOOKEEPER-3062
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cpoerschke/zookeeper master-ZOOKEEPER-3062
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/566.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 #566
----
commit a2dc484d41e0408dc70f10e37508c93ae2c683af
Author: Christine Poerschke <cp...@...>
Date: 2018-07-11T18:00:57Z
ZOOKEEPER-3062: mention fsync.warningthresholdms in FileTxnLog LOG.warn message
----
---
[GitHub] zookeeper issue #566: ZOOKEEPER-3062: mention fsync.warningthresholdms in Fi...
Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on the issue:
https://github.com/apache/zookeeper/pull/566
Is is really necessary to log this property `fsync.warningthresholdms`?
---
[GitHub] zookeeper issue #566: ZOOKEEPER-3062: mention fsync.warningthresholdms in Fi...
Posted by cpoerschke <gi...@git.apache.org>.
Github user cpoerschke commented on the issue:
https://github.com/apache/zookeeper/pull/566
Thanks everyone for your feedback!
> ... are you ok with removing the extra words in the log message?
Hmm, ok, done. Should I update the pull request and https://issues.apache.org/jira/browse/ZOOKEEPER-3062 title(s) to reflect the changed scope or would that unnecessarily confuse the ticket history and the continuous integration tools?
---
[GitHub] zookeeper issue #566: ZOOKEEPER-3062: mention fsync.warningthresholdms in Fi...
Posted by breed <gi...@git.apache.org>.
Github user breed commented on the issue:
https://github.com/apache/zookeeper/pull/566
i agree with @maoling, logging it with every warning seems overkill. the rest of the change looks great though. are you ok with removing the extra words in the log message?
---
[GitHub] zookeeper pull request #566: ZOOKEEPER-3062: mention fsync.warningthresholdm...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/566#discussion_r202508017
--- Diff: src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java ---
@@ -339,7 +342,8 @@ public synchronized void commit() throws IOException {
+ Thread.currentThread().getName()
+ " took " + syncElapsedMS
+ "ms which will adversely effect operation latency. "
- + "File size is " + channel.size() + " bytes. "
+ + "File size is " + channel.size() + " bytes and "
--- End diff --
It's cleaner to change the logger format to using placeholder with {}.
---
[GitHub] zookeeper pull request #566: ZOOKEEPER-3062: mention fsync.warningthresholdm...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/566#discussion_r202507971
--- Diff: src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java ---
@@ -99,6 +99,9 @@
public static final String LOG_FILE_PREFIX = "log";
+ static final String FSYNC_WARNING_THRESHOLD_MS_PROPERTY = "fsync.warningthresholdms";
+ static final String ZOOKEEPER_FSYNC_WARNING_THRESHOLD_MS_PROPERTY = "zookeeper."+FSYNC_WARNING_THRESHOLD_MS_PROPERTY;
--- End diff --
nit: add space between '+'
---
[GitHub] zookeeper issue #566: ZOOKEEPER-3062: mention fsync.warningthresholdms in Fi...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/566
lgtm. +1, thanks @cpoerschke .
Perhaps consider logging the value during startup (initial read of the value) instead?
---
[GitHub] zookeeper pull request #566: ZOOKEEPER-3062: mention fsync.warningthresholdm...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/566
---