You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by leventov <gi...@git.apache.org> on 2018/03/17 17:24:08 UTC
[GitHub] zookeeper pull request #490: ZOOKEEPER-3000: Use error-prone compiler
GitHub user leventov opened a pull request:
https://github.com/apache/zookeeper/pull/490
ZOOKEEPER-3000: Use error-prone compiler
Fix some bugs in tests.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/leventov/zookeeper error-prone
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/490.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 #490
----
commit d03fcb3bbe9bde8aaab2603dd8face4b5c0b256f
Author: leventov <le...@...>
Date: 2018-03-17T17:22:42Z
ZOOKEEPER-3000: Use error-prone compiler
----
---
[GitHub] zookeeper pull request #490: ZOOKEEPER-3000: Use error-prone compiler
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/490#discussion_r175416977
--- Diff: src/java/test/org/apache/zookeeper/test/ZooKeeperTestClient.java ---
@@ -71,13 +71,14 @@ private void deleteZKDir(ZooKeeper zk, String nodeName)
}
List<String> children1 = zk.getChildren(nodeName, false);
- List<String> c2 = zk.getChildren(nodeName, false, stat);
+ Stat stat2 = new Stat();
--- End diff --
Which rule has triggered this change?
---
[GitHub] zookeeper issue #490: ZOOKEEPER-3000: Use error-prone compiler
Posted by leventov <gi...@git.apache.org>.
Github user leventov commented on the issue:
https://github.com/apache/zookeeper/pull/490
@anmolnar see http://errorprone.info/docs/installation, the first paragraph: "Please note that Error Prone must be run on JDK 8 or newer. It can be used to build Java 6 or 7 code by setting the appropriate -source / -target / -bootclasspath flags."
---
[GitHub] zookeeper issue #490: ZOOKEEPER-3000: Use error-prone compiler
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/490
We just upgraded master and 3.5 branches to Java 8 a few days ago. I wrote my comment almost a month back.
In which case your patch is targeted to master and 3.5 branches only and won't be merged to 3.4. Is that okay for you?
---
[GitHub] zookeeper pull request #490: ZOOKEEPER-3000: Use error-prone compiler
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/490#discussion_r175718970
--- Diff: src/java/test/org/apache/zookeeper/test/AsyncHammerTest.java ---
@@ -61,6 +61,7 @@ protected void restart() throws Exception {
qb.startServers();
}
+ @SuppressWarnings("JUnit4TearDownNotRun")
--- End diff --
Wow, that's crazy. We must fix this sometime.
Do you think it could fit into this PR?
---
[GitHub] zookeeper issue #490: ZOOKEEPER-3000: Use error-prone compiler
Posted by leventov <gi...@git.apache.org>.
Github user leventov commented on the issue:
https://github.com/apache/zookeeper/pull/490
Yes, Java 8 is required for the build, but the produced bytecode could target Java 6 or 7. Here is a citation from http://errorprone.info/docs/installation:
> Our goal is to make it simple to add Error Prone checks to your existing Java compilation. Please note that Error Prone must be run on JDK 8 or newer. It can be used to build Java 6 or 7 code by setting the appropriate -source / -target / -bootclasspath flags.
---
[GitHub] zookeeper pull request #490: ZOOKEEPER-3000: Use error-prone compiler
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/490#discussion_r175416374
--- Diff: src/java/test/org/apache/zookeeper/test/AsyncHammerTest.java ---
@@ -61,6 +61,7 @@ protected void restart() throws Exception {
qb.startServers();
}
+ @SuppressWarnings("JUnit4TearDownNotRun")
--- End diff --
Why not annotate with After instead?
Does it break any test?
---
[GitHub] zookeeper issue #490: ZOOKEEPER-3000: Use error-prone compiler
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/490
@leventov Got that, however my issue still stands: why `ant test` command doesn't compile with java7?
Is it just me who experience it? Have you tested the patch with java7?
---
[GitHub] zookeeper pull request #490: ZOOKEEPER-3000: Use error-prone compiler
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/490#discussion_r175778556
--- Diff: src/java/test/org/apache/zookeeper/test/AsyncHammerTest.java ---
@@ -61,6 +61,7 @@ protected void restart() throws Exception {
qb.startServers();
}
+ @SuppressWarnings("JUnit4TearDownNotRun")
--- End diff --
Annotate with After and remove the call the tests.
---
[GitHub] zookeeper issue #490: ZOOKEEPER-3000: Use error-prone compiler
Posted by leventov <gi...@git.apache.org>.
Github user leventov commented on the issue:
https://github.com/apache/zookeeper/pull/490
@anmolnar if you try to run `ant test` while the test are not compiled yet (i. e. they have to be compiled), and Ant uses Java 7 which is installed on your machine for compilation, yes, this is expected that it will fail. You would need to run with Java 8.
However, I see that `javac.source` and `javac.target` in `build.xml` already set to 1.8, how do you test it with Java 7 anyway?
---
[GitHub] zookeeper issue #490: ZOOKEEPER-3000: Use error-prone compiler
Posted by leventov <gi...@git.apache.org>.
Github user leventov commented on the issue:
https://github.com/apache/zookeeper/pull/490
> However we already integrated findbugs tool in our build process, don't you think it would be better to replace that instead of using two similar tools side-by-side?
@anmolnar no, static analysis tools complement each other. Bugs found by one tool are not found by another, and vice versa. So it's good to run them all.
> Additionally I think it would be better for reviewers to split this PR into 1) integration of new static analyis tool, 2) test fixes.
Test problems are catched by the tool. If the tool is integrated first, the compilation will fail briefly. So I don't see the point of the separation. The whole PR is not that big, after all.
> Could we add the error prone compiler as an optional tool, so that similarly to e.g. code coverage tools, the build could be run with or without the tool?
@mfenes I'm not proficient in Ant, I managed to run the tool the simplest way possible that I could figure out. If you could implement this so that the tool is optional, you could do this. I don't know how to do this.
> in the commits, there are test code changes, too. Are these related to the error prone compiler change, or these are just other bugs which you've fixed?
All changes in this PR are imposed by error-prone only.
---
[GitHub] zookeeper pull request #490: ZOOKEEPER-3000: Use error-prone compiler
Posted by mfenes <gi...@git.apache.org>.
Github user mfenes commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/490#discussion_r175393897
--- Diff: build.xml ---
@@ -475,23 +484,36 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant">
pattern="${ivy.lib}/[artifact]-[revision].[ext]"/>
<ivy:cachepath pathid="mvn-ant-task-classpath" conf="mvn-ant-task"/>
</target>
- <target name="compile" depends="ivy-retrieve,clover,build-generated">
- <javac srcdir="${java.src.dir}" destdir="${build.classes}" includeantruntime="false"
+ <target name="compile" depends="ivy-retrieve,clover,build-generated,ivy-retrieve-error_prone">
+ <javac compiler="com.google.errorprone.ErrorProneAntCompilerAdapter"
+ srcdir="${java.src.dir}" destdir="${build.classes}" includeantruntime="false"
target="${javac.target}" source="${javac.source}" debug="on" encoding="${build.encoding}">
+ <compilerclasspath>
+ <pathelement location="${ivy.error_prone.lib}/error_prone_ant-${error_prone.version}.jar"/>
+ </compilerclasspath>
<classpath refid="java.classpath"/>
<compilerarg value="-Xlint:all"/>
<compilerarg value="-Xlint:-path"/>
+
</javac>
</target>
- <target name="compile-test" depends="ivy-retrieve-test,compile">
+ <target name="compile-test" depends="ivy-retrieve-test,compile,ivy-retrieve-error_prone">
--- End diff --
Could we add the error prone compiler as an optional tool, so that similarly to e.g. code coverage tools, the build could be run with or without the tool?
---
[GitHub] zookeeper issue #490: ZOOKEEPER-3000: Use error-prone compiler
Posted by leventov <gi...@git.apache.org>.
Github user leventov commented on the issue:
https://github.com/apache/zookeeper/pull/490
Yes.
---
[GitHub] zookeeper pull request #490: ZOOKEEPER-3000: Use error-prone compiler
Posted by leventov <gi...@git.apache.org>.
Github user leventov commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/490#discussion_r175569277
--- Diff: src/java/test/org/apache/zookeeper/test/AsyncHammerTest.java ---
@@ -61,6 +61,7 @@ protected void restart() throws Exception {
qb.startServers();
}
+ @SuppressWarnings("JUnit4TearDownNotRun")
--- End diff --
Because it's called from test bodies directly. I've fixed that by making the methods private instead of adding suppression.
---
[GitHub] zookeeper issue #490: ZOOKEEPER-3000: Use error-prone compiler
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/490
@leventov do you still work on this patch?
---
[GitHub] zookeeper issue #490: ZOOKEEPER-3000: Use error-prone compiler
Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on the issue:
https://github.com/apache/zookeeper/pull/490
@leventov Can we move on?
---
[GitHub] zookeeper pull request #490: ZOOKEEPER-3000: Use error-prone compiler
Posted by leventov <gi...@git.apache.org>.
Github user leventov commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/490#discussion_r177043995
--- Diff: src/java/test/org/apache/zookeeper/test/AsyncHammerTest.java ---
@@ -61,6 +61,7 @@ protected void restart() throws Exception {
qb.startServers();
}
+ @SuppressWarnings("JUnit4TearDownNotRun")
--- End diff --
I would keep it as is for symmetry with setup() method, that couldn't be declared `@Before` because it has a parameter.
---
[GitHub] zookeeper issue #490: ZOOKEEPER-3000: Use error-prone compiler
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/490
This is a good addition, thanks for bringing to our attention @leventov .
Unfortunately however we can't replace the std javac compiler with error_prone in the toolchain outright - we can have a separate option/target to do this, but it can't be the default.
I would recommend separating this into two jiras/pullrequests - one to change the build, the other to address the found issues.
@leventov you mentioned "I'm not proficient in Ant" - perhaps one of the other folks could help with this? @anmolnar you mentioned hbase is using it - anything we can pull from there wrt how they integrate it into their build?
ps. I like the idea of also getting this into 3.4 branch as well, which means it needs to be optional so that the user can run it when they have java8+, but otw not. Which matches my intent/statement above.
pps. looks like there is a new version of error_prone, might want to take this opportunity to update the dependency version. I notice the library is ASL2.0 which is also good (license is in the COPYING file currently)
---
[GitHub] zookeeper pull request #490: ZOOKEEPER-3000: Use error-prone compiler
Posted by leventov <gi...@git.apache.org>.
Github user leventov commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/490#discussion_r175777029
--- Diff: src/java/test/org/apache/zookeeper/test/AsyncHammerTest.java ---
@@ -61,6 +61,7 @@ protected void restart() throws Exception {
qb.startServers();
}
+ @SuppressWarnings("JUnit4TearDownNotRun")
--- End diff --
Fixed what?
---
[GitHub] zookeeper issue #490: ZOOKEEPER-3000: Use error-prone compiler
Posted by leventov <gi...@git.apache.org>.
Github user leventov commented on the issue:
https://github.com/apache/zookeeper/pull/490
I didn't have time to consider your latest questions yet
---
[GitHub] zookeeper issue #490: ZOOKEEPER-3000: Use error-prone compiler
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/490
Also would you please elaborate a little bit more on reasoning in Jira's description for the records?
---
[GitHub] zookeeper issue #490: ZOOKEEPER-3000: Use error-prone compiler
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/490
@maoling [hbase](https://github.com/apache/hbase/blob/f63a7ff635fa02a8a97d9a36505732f647d2e7ff/hbase-build-configuration/pom.xml) do use errorprone. At least in some modules.
---
[GitHub] zookeeper pull request #490: ZOOKEEPER-3000: Use error-prone compiler
Posted by leventov <gi...@git.apache.org>.
Github user leventov commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/490#discussion_r175555748
--- Diff: src/java/test/org/apache/zookeeper/test/ZooKeeperTestClient.java ---
@@ -71,13 +71,14 @@ private void deleteZKDir(ZooKeeper zk, String nodeName)
}
List<String> children1 = zk.getChildren(nodeName, false);
- List<String> c2 = zk.getChildren(nodeName, false, stat);
+ Stat stat2 = new Stat();
--- End diff --
"SelfEquals" (see a few lines below in diff)
---
[GitHub] zookeeper pull request #490: ZOOKEEPER-3000: Use error-prone compiler
Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/490#discussion_r175772100
--- Diff: src/java/systest/org/apache/zookeeper/test/system/InstanceManager.java ---
@@ -266,7 +282,7 @@ synchronized public void removeInstance(String name) throws InterruptedException
if (assigned == null) {
return;
}
- assignments.get(assigned.container).remove(name);
+ assignments.get(assigned.container).remove(new Assigned(name, 0));
doDelete(assignmentsNode + '/' + assigned.container + '/' + name);
--- End diff --
+1
---
[GitHub] zookeeper pull request #490: ZOOKEEPER-3000: Use error-prone compiler
Posted by mfenes <gi...@git.apache.org>.
Github user mfenes commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/490#discussion_r175393694
--- Diff: build.xml ---
@@ -475,23 +484,36 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant">
pattern="${ivy.lib}/[artifact]-[revision].[ext]"/>
<ivy:cachepath pathid="mvn-ant-task-classpath" conf="mvn-ant-task"/>
</target>
- <target name="compile" depends="ivy-retrieve,clover,build-generated">
- <javac srcdir="${java.src.dir}" destdir="${build.classes}" includeantruntime="false"
+ <target name="compile" depends="ivy-retrieve,clover,build-generated,ivy-retrieve-error_prone">
--- End diff --
Could we add the error prone compiler as an optional tool, so that similarly to e.g. code coverage tools, the build could be run with or without the tool?
---
[GitHub] zookeeper pull request #490: ZOOKEEPER-3000: Use error-prone compiler
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/490#discussion_r175718655
--- Diff: src/java/test/org/apache/zookeeper/test/ZooKeeperTestClient.java ---
@@ -71,13 +71,14 @@ private void deleteZKDir(ZooKeeper zk, String nodeName)
}
List<String> children1 = zk.getChildren(nodeName, false);
- List<String> c2 = zk.getChildren(nodeName, false, stat);
+ Stat stat2 = new Stat();
--- End diff --
Cool, thanks.
---
[GitHub] zookeeper pull request #490: ZOOKEEPER-3000: Use error-prone compiler
Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/490#discussion_r175774472
--- Diff: src/java/test/org/apache/zookeeper/test/ZooKeeperTestClient.java ---
@@ -71,13 +71,14 @@ private void deleteZKDir(ZooKeeper zk, String nodeName)
}
List<String> children1 = zk.getChildren(nodeName, false);
- List<String> c2 = zk.getChildren(nodeName, false, stat);
+ Stat stat2 = new Stat();
+ List<String> c2 = zk.getChildren(nodeName, false, stat2);
--- End diff --
**stat** has been changed(copyed) in **zk.getChildren()** . `stat.equals(stat) `is right in findbugs
---
[GitHub] zookeeper issue #490: ZOOKEEPER-3000: Use error-prone compiler
Posted by mfenes <gi...@git.apache.org>.
Github user mfenes commented on the issue:
https://github.com/apache/zookeeper/pull/490
Hi @leventov, in the commits, there are test code changes, too. Are these related to the error prone compiler change, or these are just other bugs which you've fixed?
---
[GitHub] zookeeper issue #490: ZOOKEEPER-3000: Use error-prone compiler
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/490
@leventov I've just verified your patch on my local machine.
Is Java8 a requirement for error-prone to work?
Because running `ant clean test` with java7 I got the following error message:
> BUILD FAILED
/Users/andor/git/my-zookeeper/build.xml:1417: The following error occurred while executing this line:
/Users/andor/git/my-zookeeper/build.xml:490: Class com.google.errorprone.ErrorProneAntCompilerAdapter could not be loaded because of an invalid dependency.
If that's the case, I'm afraid we can only merge this patch to master, once we upgraded to java8 which is coming soon, but not there yet.
Additionally running with Java8 I got 75 warning messages. Is that accurate?
---