You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by afine <gi...@git.apache.org> on 2017/04/24 18:10:25 UTC
[GitHub] zookeeper pull request #239: ZOOKEEPER-2762: Cleanup findbug warnings in bra...
GitHub user afine opened a pull request:
https://github.com/apache/zookeeper/pull/239
ZOOKEEPER-2762: Cleanup findbug warnings in branch-3.4: Multithreaded correctness Warnings
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/afine/zookeeper ZOOKEEPER-2762
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/239.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 #239
----
commit 8740f0a6ab959072533c2d23a56efe9c4f56aa82
Author: Abraham Fine <af...@apache.org>
Date: 2017-04-18T22:00:53Z
ZOOKEEPER-2762: Multithreaded correctness Warnings
----
---
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 #239: ZOOKEEPER-2762: Cleanup findbug warnings in bra...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/239#discussion_r117211921
--- Diff: ivy.xml ---
@@ -49,6 +49,8 @@
<dependency org="log4j" name="log4j" rev="1.2.16" transitive="false" conf="default"/>
<dependency org="jline" name="jline" rev="0.9.94" transitive="false" conf="default"/>
+ <dependency org="com.google.code.findbugs" name="annotations" rev="3.0.1" conf="default"/>
--- End diff --
Any alternative other than intro dependency to google libraries. Presently, zk code doesn't have any dependency with google libraries.
---
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 #239: ZOOKEEPER-2762: Cleanup findbug warnings in branch-3.4...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on the issue:
https://github.com/apache/zookeeper/pull/239
Merged, please close the PR, @afine
---
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 #239: ZOOKEEPER-2762: Cleanup findbug warnings in bra...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/239#discussion_r117696607
--- Diff: src/java/test/config/findbugsExcludeFile.xml ---
@@ -90,6 +90,16 @@
<Bug code="IS"/>
</Match>
<Match>
+ <Class name="org.apache.zookeeper.server.quorum.Leader"/>
+ <Method name="lead"/>
--- End diff --
Following are the variables, reported for MultiThreadedCorrectness warnings.
```
QuorumPeer.tick
FastLeaderElection.logicalclock
AuthFastLeaderElection.logicalclock
```
How about using `AtomicLong & AtomicLong` instead of excluding. Please refer branch-3.5 or trunk code as a reference.
```
AtomicLong logicalclock = new AtomicLong(); /* Election instance */
AtomicInteger tick = new AtomicInteger();
```
---
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 #239: ZOOKEEPER-2762: Cleanup findbug warnings in bra...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/239#discussion_r118267625
--- Diff: src/java/test/config/findbugsExcludeFile.xml ---
@@ -90,6 +90,16 @@
<Bug code="IS"/>
</Match>
<Match>
+ <Class name="org.apache.zookeeper.server.quorum.Leader"/>
+ <Method name="lead"/>
--- End diff --
OK, agreed. `AuthFastLeaderElection.logicalclock` change is not required.
---
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 #239: ZOOKEEPER-2762: Cleanup findbug warnings in branch-3.4...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on the issue:
https://github.com/apache/zookeeper/pull/239
+1 LGTM.
@afine, could you please rebase the patch on latest branch-3.4 code, there are few conflicts due to `ZOOKEEPER-2732` commits.
---
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 #239: ZOOKEEPER-2762: Cleanup findbug warnings in branch-3.4...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on the issue:
https://github.com/apache/zookeeper/pull/239
Thanks @afine for the updates.
+1, changes looks pretty good. I will merge this shortly.
---
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 #239: ZOOKEEPER-2762: Cleanup findbug warnings in branch-3.4...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on the issue:
https://github.com/apache/zookeeper/pull/239
@rakeshadr this should be rebased and ready to be merged.
---
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 #239: ZOOKEEPER-2762: Cleanup findbug warnings in branch-3.4...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on the issue:
https://github.com/apache/zookeeper/pull/239
Done, thanks for the review @rakeshadr
---
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 #239: ZOOKEEPER-2762: Cleanup findbug warnings in bra...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/239#discussion_r117851157
--- Diff: src/java/test/config/findbugsExcludeFile.xml ---
@@ -90,6 +90,16 @@
<Bug code="IS"/>
</Match>
<Match>
+ <Class name="org.apache.zookeeper.server.quorum.Leader"/>
+ <Method name="lead"/>
--- End diff --
@rakeshadr Initially I avoided this change because I did not want to dirty up the diff and we were doing the correct things to prevent a race condition with these variables. I should have checked 3.5 to see what we were doing there. I made the change to `QuorumPeer.tick` and `FastLeaderElection.logicalclock` but I did not make the change to `AuthFastLeaderElection.logicalclock` because I thought it would be best to avoid changing a deprecated class. Let me know if that's ok.
---
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 #239: ZOOKEEPER-2762: Cleanup findbug warnings in bra...
Posted by afine <gi...@git.apache.org>.
Github user afine closed the pull request at:
https://github.com/apache/zookeeper/pull/239
---
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.
---