Skip to content

Commit

Permalink
KAFKA-5887; Replace findBugs with spotBugs and upgrade to Gradle 4.10
Browse files Browse the repository at this point in the history
findBugs is abandoned, it doesn't work with Java 9 and the Gradle plugin will be deprecated in
Gradle 5.0: gradle/gradle#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
  • Loading branch information
ijuma authored and lindong28 committed Sep 10, 2018
1 parent 0d535b1 commit f123d2f
Show file tree
Hide file tree
Showing 7 changed files with 39 additions and 31 deletions.
16 changes: 8 additions & 8 deletions README.md
Expand Up @@ -168,7 +168,7 @@ Please note for this to work you should create/update user maven settings (typic
./gradlew dependencyUpdates

### 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 enforces a consistent coding style in Kafka.
Expand All @@ -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
subproject build directories. They are also are printed to the console. The build will fail if Checkstyle fails.

#### Findbugs ####
Findbugs uses static analysis to look for bugs in the code.
You can run findbugs using:
#### Spotbugs ####
Spotbugs uses static analysis to look for bugs in the code.
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
directories. Use -PxmlFindBugsReport=true to generate an XML report instead of an HTML one.
The spotbugs warnings will be found in `reports/spotbugs/main.html` and `reports/spotbugs/test.html` files in the subproject build
directories. Use -PxmlSpotBugsReport=true to generate an XML report instead of an HTML one.

### Common build options ###

Expand All @@ -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.
* `skipSigning`: skips signing of artifacts.
* `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 ###

Expand Down
31 changes: 18 additions & 13 deletions build.gradle
Expand Up @@ -19,6 +19,9 @@ buildscript {
repositories {
mavenCentral()
jcenter()
maven {
url "https://plugins.gradle.org/m2/"
}
}
apply from: file('gradle/buildscript.gradle'), to: buildscript

Expand All @@ -30,6 +33,7 @@ buildscript {
classpath 'com.github.jengelman.gradle.plugins:shadow:2.0.4'
classpath 'org.owasp:dependency-check-gradle:3.2.1'
classpath "com.diffplug.spotless:spotless-plugin-gradle:3.10.0"
classpath "gradle.plugin.com.github.spotbugs:spotbugs-gradle-plugin:1.6.3"
}
}

Expand Down Expand Up @@ -85,7 +89,7 @@ allprojects {
}

ext {
gradleVersion = "4.8.1"
gradleVersion = "4.10"
minJavaVersion = "8"
buildVersionFileName = "kafka-version.properties"

Expand Down Expand Up @@ -145,9 +149,8 @@ subprojects {
apply plugin: 'maven'
apply plugin: 'signing'
apply plugin: 'checkstyle'

if (!JavaVersion.current().isJava9Compatible())
apply plugin: 'findbugs'
if (!JavaVersion.current().isJava11Compatible())
apply plugin: "com.github.spotbugs"

sourceCompatibility = minJavaVersion
targetCompatibility = minJavaVersion
Expand Down Expand Up @@ -364,18 +367,20 @@ subprojects {
}
test.dependsOn('checkstyleMain', 'checkstyleTest')

if (!JavaVersion.current().isJava9Compatible()) {
findbugs {
toolVersion = "3.0.1"
excludeFilter = file("$rootDir/gradle/findbugs-exclude.xml")
if (!JavaVersion.current().isJava11Compatible()) {
spotbugs {
// 3.1.6 has a regression that breaks our build, seems to be https://github.com/spotbugs/spotbugs/pull/688
toolVersion = '3.1.5'
excludeFilter = file("$rootDir/gradle/spotbugs-exclude.xml")
ignoreFailures = false
}
test.dependsOn('findbugsMain')
test.dependsOn('spotbugsMain')

tasks.withType(FindBugs) {
tasks.withType(com.github.spotbugs.SpotBugsTask) {
reports {
xml.enabled(project.hasProperty('xmlFindBugsReport'))
html.enabled(!project.hasProperty('xmlFindBugsReport'))
// Continue supporting `xmlFindBugsReport` for compatibility
xml.enabled(project.hasProperty('xmlSpotBugsReport') || project.hasProperty('xmlFindBugsReport'))
html.enabled(!project.hasProperty('xmlSpotBugsReport') && !project.hasProperty('xmlFindBugsReport'))
}
}
}
Expand Down Expand Up @@ -408,7 +413,7 @@ subprojects {
}

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
}
}
Expand Down
Expand Up @@ -942,7 +942,7 @@ public static class NonNullValidator implements Validator {
@Override
public void ensureValid(String name, Object value) {
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");
}
}
Expand Down
Expand Up @@ -22,6 +22,7 @@
import org.slf4j.LoggerFactory;

import java.io.IOException;
import java.io.InputStream;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.Map;
Expand Down Expand Up @@ -64,7 +65,9 @@ public void initialize(Subject subject, CallbackHandler callbackHandler, Map<Str
if (!credentialPropertiesMap.containsKey(fileName)) {
Properties credentialProperties = new Properties();
try {
credentialProperties.load(Files.newInputStream(Paths.get(fileName)));
try (InputStream inputStream = Files.newInputStream(Paths.get(fileName))) {
credentialProperties.load(inputStream);
}
credentialPropertiesMap.putIfAbsent(fileName, credentialProperties);
} catch (IOException e) {
log.error("Error loading credentials file ", e);
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/scala/kafka/server/ClientQuotaManager.scala
Expand Up @@ -180,7 +180,7 @@ class ClientQuotaManager(private val config: ClientQuotaManagerConfig,
delayQueueSensor.add(metrics.metricName("queue-size",
quotaType.toString,
"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() {
throttledChannelReaper.start()
}
Expand Down
12 changes: 6 additions & 6 deletions gradle/findbugs-exclude.xml → gradle/spotbugs-exclude.xml
Expand Up @@ -15,12 +15,12 @@
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.
This file dictates which categories of bugs and individual false positives that we supress.
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 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>
<Match>
Expand All @@ -44,8 +44,8 @@ For a detailed description of findbugs bug categories, see http://findbugs.sourc
</Match>

<Match>
<!-- Findbugs 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
<!-- Spotbugs tends to work a little bit better with Java than with Scala. We suppress
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.
NP_LOAD_OF_KNOWN_NULL_VALUE: The variable referenced at this point is known to be null
Expand Down
2 changes: 1 addition & 1 deletion jenkins.sh
Expand Up @@ -17,4 +17,4 @@
# 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
# 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 "$@"

0 comments on commit f123d2f

Please sign in to comment.