You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by lvfangmin <gi...@git.apache.org> on 2017/07/31 19:03:38 UTC
[GitHub] zookeeper pull request #322: [ZOOKEEPER-2853] Update lastZxidSeen in FileTxn...
GitHub user lvfangmin opened a pull request:
https://github.com/apache/zookeeper/pull/322
[ZOOKEEPER-2853] Update lastZxidSeen in FileTxnLog
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/lvfangmin/zookeeper ZOOKEEPER-2853
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/322.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 #322
----
commit 3e59b28d70439a88847f90beec8be1d48464f161
Author: Fangmin Lyu <al...@fb.com>
Date: 2017-07-31T19:02:33Z
[ZOOKEEPER-2853] Update lastZxidSeen in FileTxnLog
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #322: [ZOOKEEPER-2853] Update lastZxidSeen in FileTxn...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/322#discussion_r131052685
--- Diff: src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java ---
@@ -118,7 +118,7 @@
fsyncWarningThresholdMS = fsyncWarningThreshold;
}
- long lastZxidSeen;
+ long lastMaxZxidSeen;
--- End diff --
oh, last minute change left the issue here, was thinking of lastMaxZxidSeen will be more accurate, but thought a bit more and lastZxidSeen should be Ok too, so changed it back, forgot to change here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #322: [ZOOKEEPER-2853] Update lastZxidSeen in FileTxn...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/322#discussion_r131053635
--- Diff: src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java ---
@@ -199,14 +199,16 @@ public synchronized boolean append(TxnHeader hdr, Record txn)
LOG.warn("Current zxid " + hdr.getZxid()
--- End diff --
I would recommend make the early return changes here as the changes to be made is small and they are not hard to review. When we say "do it separately" in the context of zk, it probably implies "never" <grin>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #322: [ZOOKEEPER-2853] Update lastZxidSeen in FileTxn...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/322
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #322: [ZOOKEEPER-2853] Update lastZxidSeen in FileTxn...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/322#discussion_r131049471
--- Diff: src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java ---
@@ -199,14 +199,16 @@ public synchronized boolean append(TxnHeader hdr, Record txn)
LOG.warn("Current zxid " + hdr.getZxid()
--- End diff --
I like the suggestion here. Though ZK does not enforce such style, early exists is my preference for reasons documented here: https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code Though this change can be done separately as it's not an existing stylish issue.
>> BTW,why do I see so many flaky changes?
Not sure about what flaky changes you are referring to? @maoling
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #322: [ZOOKEEPER-2853] Update lastZxidSeen in FileTxn...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/322#discussion_r131052928
--- Diff: src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java ---
@@ -199,14 +199,16 @@ public synchronized boolean append(TxnHeader hdr, Record txn)
LOG.warn("Current zxid " + hdr.getZxid()
--- End diff --
I prefer return early too, I didn't change it as it's not related with this diff, and I'd like to make it with minimum change. I can update it here too if you guys prefer.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #322: [ZOOKEEPER-2853] Update lastZxidSeen in FileTxn...
Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/322#discussion_r130566850
--- Diff: src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java ---
@@ -118,7 +118,7 @@
fsyncWarningThresholdMS = fsyncWarningThreshold;
}
- long lastZxidSeen;
+ long lastMaxZxidSeen;
--- End diff --
a compile error. **lastZxidSeen** is still used below
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #322: [ZOOKEEPER-2853] Update lastZxidSeen in FileTxn...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/322#discussion_r131052814
--- Diff: src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java ---
@@ -199,14 +199,16 @@ public synchronized boolean append(TxnHeader hdr, Record txn)
LOG.warn("Current zxid " + hdr.getZxid()
--- End diff --
@maoling my vim editor will remove the tailing space automatically, that's why there is so many changes, I think it's better to format it without tailing space.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #322: [ZOOKEEPER-2853] Update lastZxidSeen in FileTxnLog
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/322
Committed to master: https://github.com/apache/zookeeper/commit/5c4e44332e55bbf21ca59583f3e8ca97fc4bb266
branch-3.5:
https://github.com/apache/zookeeper/commit/5d85cfa34ed9c7415dde7f7cda7918499d9ec065
branch-3.4 (I did a separate back port due to a merge conflict):
https://github.com/apache/zookeeper/commit/e4303a37a813c9f1bd4cdefd9c754267b12c32b4
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #322: [ZOOKEEPER-2853] Update lastZxidSeen in FileTxn...
Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/322#discussion_r130567289
--- Diff: src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java ---
@@ -199,14 +199,16 @@ public synchronized boolean append(TxnHeader hdr, Record txn)
LOG.warn("Current zxid " + hdr.getZxid()
--- End diff --
I think:
```
if (hdr == null) return false;
/**/
return true;
```
may be better,to avoid too deep if-else nest
BTW,why do I see so many flaky changes?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #322: [ZOOKEEPER-2853] Update lastZxidSeen in FileTxn...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/322#discussion_r131049142
--- Diff: src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java ---
@@ -118,7 +118,7 @@
fsyncWarningThresholdMS = fsyncWarningThreshold;
}
- long lastZxidSeen;
+ long lastMaxZxidSeen;
--- End diff --
Yes this patch does not build @lvfangmin
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---