KAFKA-5887; Replace findBugs with spotBugs and upgrade to Gradle 4.10

findBugs is abandoned, it doesn't work with Java 9 and the Gradle plugin will be deprecated in
Gradle 5.0: https://github.com/gradle/gradle/pull/6664

spotBugs is actively maintained and it supports Java 8, 9 and 10. Java 11 is not supported yet,
but it's likely to happen soon.

Also fixed a file leak in Connect identified by spotbugs.

Manually tested spotBugsMain, jarAll and importing kafka in IntelliJ and running
a build in the IDE.

Author: Ismael Juma <ismael@juma.me.uk>

Reviewers: Colin Patrick McCabe <colin@cmccabe.xyz>, Dong Lin <lindong28@gmail.com>

Closes #5625 from ijuma/kafka-5887-spotbugs
This commit is contained in:
Ismael Juma 2018-09-10 13:14:00 -07:00 committed by Dong Lin
parent 0d535b167a
commit f123d2f18c
7 changed files with 39 additions and 31 deletions

View File

@ -168,7 +168,7 @@ Please note for this to work you should create/update user maven settings (typic
./gradlew dependencyUpdates ./gradlew dependencyUpdates
### Running code quality checks ### ### Running code quality checks ###
There are two code quality analysis tools that we regularly run, findbugs and checkstyle. There are two code quality analysis tools that we regularly run, spotbugs and checkstyle.
#### Checkstyle #### #### Checkstyle ####
Checkstyle enforces a consistent coding style in Kafka. Checkstyle enforces a consistent coding style in Kafka.
@ -179,14 +179,14 @@ You can run checkstyle using:
The checkstyle warnings will be found in `reports/checkstyle/reports/main.html` and `reports/checkstyle/reports/test.html` files in the The checkstyle warnings will be found in `reports/checkstyle/reports/main.html` and `reports/checkstyle/reports/test.html` files in the
subproject build directories. They are also are printed to the console. The build will fail if Checkstyle fails. subproject build directories. They are also are printed to the console. The build will fail if Checkstyle fails.
#### Findbugs #### #### Spotbugs ####
Findbugs uses static analysis to look for bugs in the code. Spotbugs uses static analysis to look for bugs in the code.
You can run findbugs using: You can run spotbugs using:
./gradlew findbugsMain findbugsTest -x test ./gradlew spotbugsMain spotbugsTest -x test
The findbugs warnings will be found in `reports/findbugs/main.html` and `reports/findbugs/test.html` files in the subproject build The spotbugs warnings will be found in `reports/spotbugs/main.html` and `reports/spotbugs/test.html` files in the subproject build
directories. Use -PxmlFindBugsReport=true to generate an XML report instead of an HTML one. directories. Use -PxmlSpotBugsReport=true to generate an XML report instead of an HTML one.
### Common build options ### ### Common build options ###
@ -198,7 +198,7 @@ The following options should be set with a `-P` switch, for example `./gradlew -
* `showStandardStreams`: shows standard out and standard error of the test JVM(s) on the console. * `showStandardStreams`: shows standard out and standard error of the test JVM(s) on the console.
* `skipSigning`: skips signing of artifacts. * `skipSigning`: skips signing of artifacts.
* `testLoggingEvents`: unit test events to be logged, separated by comma. For example `./gradlew -PtestLoggingEvents=started,passed,skipped,failed test`. * `testLoggingEvents`: unit test events to be logged, separated by comma. For example `./gradlew -PtestLoggingEvents=started,passed,skipped,failed test`.
* `xmlFindBugsReport`: enable XML reports for findBugs. This also disables HTML reports as only one can be enabled at a time. * `xmlSpotBugsReport`: enable XML reports for spotBugs. This also disables HTML reports as only one can be enabled at a time.
### Running in Vagrant ### ### Running in Vagrant ###

View File

@ -19,6 +19,9 @@ buildscript {
repositories { repositories {
mavenCentral() mavenCentral()
jcenter() jcenter()
maven {
url "https://plugins.gradle.org/m2/"
}
} }
apply from: file('gradle/buildscript.gradle'), to: buildscript apply from: file('gradle/buildscript.gradle'), to: buildscript
@ -30,6 +33,7 @@ buildscript {
classpath 'com.github.jengelman.gradle.plugins:shadow:2.0.4' classpath 'com.github.jengelman.gradle.plugins:shadow:2.0.4'
classpath 'org.owasp:dependency-check-gradle:3.2.1' classpath 'org.owasp:dependency-check-gradle:3.2.1'
classpath "com.diffplug.spotless:spotless-plugin-gradle:3.10.0" classpath "com.diffplug.spotless:spotless-plugin-gradle:3.10.0"
classpath "gradle.plugin.com.github.spotbugs:spotbugs-gradle-plugin:1.6.3"
} }
} }
@ -85,7 +89,7 @@ allprojects {
} }
ext { ext {
gradleVersion = "4.8.1" gradleVersion = "4.10"
minJavaVersion = "8" minJavaVersion = "8"
buildVersionFileName = "kafka-version.properties" buildVersionFileName = "kafka-version.properties"
@ -145,9 +149,8 @@ subprojects {
apply plugin: 'maven' apply plugin: 'maven'
apply plugin: 'signing' apply plugin: 'signing'
apply plugin: 'checkstyle' apply plugin: 'checkstyle'
if (!JavaVersion.current().isJava11Compatible())
if (!JavaVersion.current().isJava9Compatible()) apply plugin: "com.github.spotbugs"
apply plugin: 'findbugs'
sourceCompatibility = minJavaVersion sourceCompatibility = minJavaVersion
targetCompatibility = minJavaVersion targetCompatibility = minJavaVersion
@ -364,18 +367,20 @@ subprojects {
} }
test.dependsOn('checkstyleMain', 'checkstyleTest') test.dependsOn('checkstyleMain', 'checkstyleTest')
if (!JavaVersion.current().isJava9Compatible()) { if (!JavaVersion.current().isJava11Compatible()) {
findbugs { spotbugs {
toolVersion = "3.0.1" // 3.1.6 has a regression that breaks our build, seems to be https://github.com/spotbugs/spotbugs/pull/688
excludeFilter = file("$rootDir/gradle/findbugs-exclude.xml") toolVersion = '3.1.5'
excludeFilter = file("$rootDir/gradle/spotbugs-exclude.xml")
ignoreFailures = false ignoreFailures = false
} }
test.dependsOn('findbugsMain') test.dependsOn('spotbugsMain')
tasks.withType(FindBugs) { tasks.withType(com.github.spotbugs.SpotBugsTask) {
reports { reports {
xml.enabled(project.hasProperty('xmlFindBugsReport')) // Continue supporting `xmlFindBugsReport` for compatibility
html.enabled(!project.hasProperty('xmlFindBugsReport')) xml.enabled(project.hasProperty('xmlSpotBugsReport') || project.hasProperty('xmlFindBugsReport'))
html.enabled(!project.hasProperty('xmlSpotBugsReport') && !project.hasProperty('xmlFindBugsReport'))
} }
} }
} }
@ -408,7 +413,7 @@ subprojects {
} }
gradle.taskGraph.whenReady { taskGraph -> gradle.taskGraph.whenReady { taskGraph ->
taskGraph.getAllTasks().findAll { it.name.contains('findbugsScoverage') || it.name.contains('findbugsTest') }.each { task -> taskGraph.getAllTasks().findAll { it.name.contains('spotbugsScoverage') || it.name.contains('spotbugsTest') }.each { task ->
task.enabled = false task.enabled = false
} }
} }

View File

@ -942,7 +942,7 @@ public class ConfigDef {
@Override @Override
public void ensureValid(String name, Object value) { public void ensureValid(String name, Object value) {
if (value == null) { if (value == null) {
// Pass in the string null to avoid the findbugs warning // Pass in the string null to avoid the spotbugs warning
throw new ConfigException(name, "null", "entry must be non null"); throw new ConfigException(name, "null", "entry must be non null");
} }
} }

View File

@ -22,6 +22,7 @@ import org.slf4j.Logger;
import org.slf4j.LoggerFactory; import org.slf4j.LoggerFactory;
import java.io.IOException; import java.io.IOException;
import java.io.InputStream;
import java.nio.file.Files; import java.nio.file.Files;
import java.nio.file.Paths; import java.nio.file.Paths;
import java.util.Map; import java.util.Map;
@ -64,7 +65,9 @@ public class PropertyFileLoginModule implements LoginModule {
if (!credentialPropertiesMap.containsKey(fileName)) { if (!credentialPropertiesMap.containsKey(fileName)) {
Properties credentialProperties = new Properties(); Properties credentialProperties = new Properties();
try { try {
credentialProperties.load(Files.newInputStream(Paths.get(fileName))); try (InputStream inputStream = Files.newInputStream(Paths.get(fileName))) {
credentialProperties.load(inputStream);
}
credentialPropertiesMap.putIfAbsent(fileName, credentialProperties); credentialPropertiesMap.putIfAbsent(fileName, credentialProperties);
} catch (IOException e) { } catch (IOException e) {
log.error("Error loading credentials file ", e); log.error("Error loading credentials file ", e);

View File

@ -180,7 +180,7 @@ class ClientQuotaManager(private val config: ClientQuotaManagerConfig,
delayQueueSensor.add(metrics.metricName("queue-size", delayQueueSensor.add(metrics.metricName("queue-size",
quotaType.toString, quotaType.toString,
"Tracks the size of the delay queue"), new Total()) "Tracks the size of the delay queue"), new Total())
start() // Use start method to keep findbugs happy start() // Use start method to keep spotbugs happy
private def start() { private def start() {
throttledChannelReaper.start() throttledChannelReaper.start()
} }

View File

@ -15,12 +15,12 @@
limitations under the License. limitations under the License.
--> -->
<!-- Findbugs filtering. <!-- Spotbugs filtering.
Findbugs is a static code analysis tool run as part of the "check" phase of the build. Spotbugs is a static code analysis tool run as part of the "check" phase of the build.
This file dictates which categories of bugs and individual false positives that we supress. This file dictates which categories of bugs and individual false positives that we suppress.
For a detailed description of findbugs bug categories, see http://findbugs.sourceforge.net/bugDescriptions.html For a detailed description of spotbugs bug categories, see https://spotbugs.readthedocs.io/en/latest/bugDescriptions.html
--> -->
<FindBugsFilter> <FindBugsFilter>
<Match> <Match>
@ -44,8 +44,8 @@ For a detailed description of findbugs bug categories, see http://findbugs.sourc
</Match> </Match>
<Match> <Match>
<!-- Findbugs tends to work a little bit better with Java than with Scala. We suppress <!-- Spotbugs tends to work a little bit better with Java than with Scala. We suppress
some categories of bug reports when using Scala, since findbugs generates huge some categories of bug reports when using Scala, since spotbugs generates huge
numbers of false positives in some of these categories when examining Scala code. numbers of false positives in some of these categories when examining Scala code.
NP_LOAD_OF_KNOWN_NULL_VALUE: The variable referenced at this point is known to be null NP_LOAD_OF_KNOWN_NULL_VALUE: The variable referenced at this point is known to be null

View File

@ -17,4 +17,4 @@
# This script is used for verifying changes in Jenkins. In order to provide faster feedback, the tasks are ordered so # This script is used for verifying changes in Jenkins. In order to provide faster feedback, the tasks are ordered so
# that faster tasks are executed in every module before slower tasks (if possible). For example, the unit tests for all # that faster tasks are executed in every module before slower tasks (if possible). For example, the unit tests for all
# the modules are executed before the integration tests. # the modules are executed before the integration tests.
./gradlew clean compileJava compileScala compileTestJava compileTestScala spotlessScalaCheck checkstyleMain checkstyleTest findbugsMain unitTest rat integrationTest --no-daemon --continue -PxmlFindBugsReport=true -PtestLoggingEvents=started,passed,skipped,failed "$@" ./gradlew clean compileJava compileScala compileTestJava compileTestScala spotlessScalaCheck checkstyleMain checkstyleTest spotbugsMain unitTest rat integrationTest --no-daemon --continue -PxmlSpotBugsReport=true -PtestLoggingEvents=started,passed,skipped,failed "$@"