Enable spotbugs and fix high priority bugs #38
Conversation
common/src/main/java/org/apache/uniffle/common/config/RssConf.java
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #38 +/- ##
============================================
- Coverage 54.96% 54.86% -0.10%
+ Complexity 1094 1092 -2
============================================
Files 146 146
Lines 7778 7775 -3
Branches 749 749
============================================
- Hits 4275 4266 -9
- Misses 3267 3272 +5
- Partials 236 237 +1
Continue to review full report at Codecov.
|
storage/src/main/java/org/apache/uniffle/storage/util/ShuffleStorageUtils.java
Show resolved
Hide resolved
|
It is better to decouple '3. Fix bugs reported by spotbugs' in another PRs to make this PR more independent. |
If we only include 1 and 2, the CI will fail. |
It is possible to make this checker with optional rules to avoid break CI? If some changes are not what we really wanted. |
Yes, it is possible to add suppressions (by annotation) and exclusions (by config).
It's not powered by AI, it's just static code analysis by pattern matching. |
| @@ -820,6 +820,7 @@ | |||
| </dependency> | |||
| </dependencies> | |||
| <configuration> | |||
| <threshold>high</threshold> | |||
There was a problem hiding this comment.
What about other threshold?
There was a problem hiding this comment.
It will report too many errors, and some of them you might not want to fix.
For example, the default threshold is medium, see "Summary of failures" in
https://github.com/apache/incubator-uniffle/runs/7299882996?check_suite_focus=true
There was a problem hiding this comment.
If there is a need, we can change it step by step, instead of trying to fix all of them in one huge PR.
There was a problem hiding this comment.
I think high is OK, LGTM.
|
Thanks @jerqi and @frankliee for the review. |
What changes were proposed in this pull request?
Because currently there are too many medium (default) priority bugs,
and many of them are difficult to fix.
Why are the changes needed?
To improve code quality.
Does this PR introduce any user-facing change?
One notable change is we are comparing content instead of reference of the array in
ShufflePartitionedBlock#equals(), see: a994cff. We can revert it if it's not desired.Another is
toString()methods will callArrays.toString()instead of printing array reference as string now.How was this patch tested?
mvn test-compile spotbugs:check -Pspark3mvn test-compile spotbugs:check -Pspark2mvn test-compile spotbugs:check -PmrAlso by CI: https://github.com/kaijchen/incubator-uniffle/actions/runs/2634089750