HDDS-2195. Apply spotbugs check to test code #1806

symious · 2021-01-15T16:15:14Z

What changes were proposed in this pull request?

Add spotbugs plugin.
Fixed two bugs reported by spotbugs.
Add other bugs reported by spotbugs to spotbugs-exclude.xml.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-2195

How was this patch tested?

Manually test passed. Spotbugs plugin works fine by running "mvn compile site".
The compile is needed according to spotbugs/spotbugs-maven-plugin#93.

symious · 2021-01-21T14:01:17Z

@adoroszlai Could you help to review the PR?

adoroszlai

Thanks @symious for working on this.

Ozone already uses Spotbugs for non-test code:

ozone/pom.xml

Lines 1704 to 1712 in 3d70fab

    
           <plugin> 
        
             <groupId>com.github.spotbugs</groupId> 
        
             <artifactId>spotbugs-maven-plugin</artifactId> 
        
             <version>${spotbugs.version}</version> 
        
             <configuration> 
        
               <maxHeap>1024</maxHeap> 
        
               <xmlOutput>true</xmlOutput> 
        
             </configuration> 
        
           </plugin>

and most sub-projects have their own exclude file:

hadoop-hdds/common/dev-support/findbugsExcludeFile.xml
hadoop-hdds/container-service/dev-support/findbugsExcludeFile.xml
hadoop-hdds/interface-admin/dev-support/findbugsExcludeFile.xml
hadoop-hdds/interface-client/dev-support/findbugsExcludeFile.xml
hadoop-hdds/interface-server/dev-support/findbugsExcludeFile.xml
hadoop-ozone/common/dev-support/findbugsExcludeFile.xml
hadoop-ozone/csi/dev-support/findbugsExcludeFile.xml
hadoop-ozone/insight/dev-support/findbugsExcludeFile.xml
hadoop-ozone/interface-client/dev-support/findbugsExcludeFile.xml
hadoop-ozone/interface-storage/dev-support/findbugsExcludeFile.xml
hadoop-ozone/recon/dev-support/findbugsExcludeFile.xml
hadoop-ozone/tools/dev-support/findbugsExcludeFile.xml

Spotbugs has a configuration setting (<includeTests>) for including tests, which defaults to false. I think we only need to set it to true, and fix or exclude any failures reported for test code.

There is a helper script to run Spotbugs (named findbugs.sh for historical reasons), which is also run by the findbugs check in CI:

https://github.com/apache/ozone/blob/master/hadoop-ozone/dev-support/checks/findbugs.sh

symious · 2021-01-21T23:20:42Z

@adoroszlai Thanks for the detailed comment, I will fix the patch later.

symious · 2021-01-23T07:51:20Z

@adoroszlai Please have a check on the commits, passed the findbugs check and tests locally.

symious · 2021-01-23T09:17:35Z

Still have some findbugs and checkstyle to fix.

symious · 2021-01-23T22:06:41Z

@adoroszlai Check green now, could you help to review the changes?

@symious

Thanks a lot @symious for updating the patch and fixing a whole bunch of spotbugs issues. Currently I don't have time to check it in detail, but my previous change request is no longer valid.

symious · 2021-01-28T15:25:26Z

Understand. @adoroszlai

adoroszlai

Thanks @symious for this huge patch. It mostly looks good to me, few minor items noted inline.

hadoop-hdds/config/src/test/java/org/apache/hadoop/hdds/conf/TestConfigFileGenerator.java

adoroszlai · 2021-02-02T12:23:14Z

...tion-test/mini-chaos-tests/src/test/java/org/apache/hadoop/ozone/failure/FailureManager.java

@@ -68,7 +68,7 @@ private void fail() {
      f.fail(cluster);
    } catch (Throwable t) {
      LOG.info("Caught exception while inducing failure:{}", f.getName(), t);
-      System.exit(-2);
+      throw new RuntimeException();


@mukul1987 will this keep existing behavior of chaos tests?

...tegration-test/src/test/java/org/apache/hadoop/fs/contract/AbstractContractUnbufferTest.java

...test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneClientRetriesOnException.java

...one/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyRequest.java

...c/test/java/org/apache/hadoop/ozone/om/request/security/TestOMGetDelegationTokenRequest.java

hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestEndpoints.java

...org/apache/hadoop/hdds/scm/container/placement/algorithms/TestContainerPlacementFactory.java

...s/server-scm/src/test/java/org/apache/hadoop/hdds/scm/server/TestSCMBlockProtocolServer.java

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/MiniOzoneHAClusterImpl.java

symious · 2021-02-02T16:23:53Z

@adoroszlai Thanks for the detailed review. Will fix the issues later.

adoroszlai · 2021-02-02T16:29:01Z

Will fix the issues later.

Sure, take your time. I'll try to help with resolving any merge conflicts.

adoroszlai

Thanks @symious for updating the patch.

symious · 2021-02-03T13:42:29Z

@adoroszlai Thanks for the review.

adoroszlai · 2021-02-04T06:15:10Z

Thanks @symious for the contribution!

adoroszlai previously requested changes Jan 21, 2021

View reviewed changes

symious added 4 commits January 22, 2021 18:33

HDDS-2195. P1

ecbbd4d

HDDS-2195. hdds server_scm

b87d456

HDDS-2195. P3

6e2d715

HDDS-2195. Finish spotbugs check for test code

b0246a0

symious force-pushed the HDDS-2195 branch from 1129ce1 to b0246a0 Compare January 23, 2021 07:49

symious added 2 commits January 23, 2021 16:09

HDDS-2195. Fix checkstyle

5b10c2c

HDDS-2195. Fix checkstyle 2

5f64413

symious added 3 commits January 23, 2021 21:47

HDDS-2195. Fix integration-test checkstyle and findbugs

7660a24

HDDS-2195. Fix findbugs for fault-injection-test

ae5418f

HDDS-2195. Recover static init() and shutdown()

7e5dd85

symious and others added 2 commits January 28, 2021 23:13

Merge branch 'master' into HDDS-2195

42e6e83

HDDS-2195. Remove unused import

8a76e04

adoroszlai added 2 commits February 1, 2021 17:33

Merge remote-tracking branch 'origin/master' into HDDS-2195

e40e21e

Fix checkstyle: modifier order

d3db848

symious mentioned this pull request Feb 2, 2021

HDDS-4780. Disable spotbugs for (the empty) hadoop-ozone-datanode project #1878

Merged

adoroszlai requested changes Feb 2, 2021

View reviewed changes

symious and others added 3 commits February 3, 2021 11:49

HDDS-2195. Fix some issues

6f40583

Merge branch 'master' into HDDS-2195

10bf5c2

HDDS-2195. Add assertNotNull in validateFullFileContents

f810f2a

adoroszlai approved these changes Feb 3, 2021

View reviewed changes

adoroszlai merged commit 1e04c0e into apache:master Feb 4, 2021

symious deleted the HDDS-2195 branch February 4, 2021 07:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HDDS-2195. Apply spotbugs check to test code #1806

HDDS-2195. Apply spotbugs check to test code #1806

symious commented Jan 15, 2021

symious commented Jan 21, 2021

adoroszlai left a comment

symious commented Jan 21, 2021

symious commented Jan 23, 2021

symious commented Jan 23, 2021

symious commented Jan 23, 2021

symious commented Jan 28, 2021

adoroszlai left a comment

adoroszlai Feb 2, 2021

symious commented Feb 2, 2021

adoroszlai commented Feb 2, 2021

adoroszlai left a comment

symious commented Feb 3, 2021

adoroszlai commented Feb 4, 2021

	<plugin>
	<groupId>com.github.spotbugs</groupId>
	<artifactId>spotbugs-maven-plugin</artifactId>
	<version>${spotbugs.version}</version>
	<configuration>
	<maxHeap>1024</maxHeap>
	<xmlOutput>true</xmlOutput>
	</configuration>
	</plugin>

HDDS-2195. Apply spotbugs check to test code #1806

HDDS-2195. Apply spotbugs check to test code #1806

Conversation

symious commented Jan 15, 2021

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

symious commented Jan 21, 2021

adoroszlai left a comment

Choose a reason for hiding this comment

symious commented Jan 21, 2021

symious commented Jan 23, 2021

symious commented Jan 23, 2021

symious commented Jan 23, 2021

symious commented Jan 28, 2021

adoroszlai left a comment

Choose a reason for hiding this comment

adoroszlai Feb 2, 2021

Choose a reason for hiding this comment

symious commented Feb 2, 2021

adoroszlai commented Feb 2, 2021

adoroszlai left a comment

Choose a reason for hiding this comment

symious commented Feb 3, 2021

adoroszlai commented Feb 4, 2021