-
Notifications
You must be signed in to change notification settings - Fork 440
RATIS-2352. Update spotbugs to 4.8.6 and suppress new warnings #1307
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
adoroszlai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @symious for the patch, LGTM. We should follow-up with fixing warnings where possible.
szetszwo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@symious , thanks for working on this!
We should try to fix the existing warnings but not excluding all of them. Many of them should be easy to fix. The difficult ones could be excluded.
| <spotbugs.version>4.8.6</spotbugs.version> | ||
| <spotbugs-plugin.version>4.8.6.2</spotbugs-plugin.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not 4.9.x?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4.8.6 fits the building with JDK21, thus it's chosen.
Or do you suggest we choose a more up-to-date version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't version 4.9.x be built with JDK 21? I could not see more information about this in 4.9.0 release notte(https://github.com/spotbugs/spotbugs/releases/tag/4.9.0), could you please explain more details about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use 4.9.x. Apache Hadoop has been updated to 4.9.x. It is working fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to 4.9.7, same as Hadoop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps many of the EI_EXPOSE_REP2 and EI_EXPOSE_REP bug patterns can be easily fixed in this PR. If it causes the CI to fail, we may need to review any modifications to the returned objects and ensure proper access control when manipulating them outside of the class. It might be better to use the class API to modify the fields instead.
BTW, I’m not opposed to the approach of leaving the actual issues unfixed in this PR, with the intention of addressing them in a future PR. I'm ok with both approaches.
|
I'm also okay for both approaches. |
|
@OneSizeFitsQuorum @szetszwo Thanks for the comments! Let’s proceed with the current approach then — this will serve as a good baseline, and if any performance issues come up later, we can make adjustments based on it. |
|
Looks like Spotbugs 4.9 requires Java 11, so we'll need to stick to 4.8.6 until minimum JDK version for Ratis is bumped. |
Reverting back to 4.8.6. |
This reverts commit 505df03.
Sorry, I forgot to mention that Spotbugs 4.9 required Java 11 to run it in the build. Note that Spotbugs is a build tool but not a runtime library. So we don't have to bump the minimum required JDK version for Ratis -- i.e. Ratis itself still can be built with and run with JDK 8, although Spotbugs 4.9. have to be run with JDK 11 or above. @adoroszlai , is it possible to change our build to do that? I am fine to use Spotbugs 4.8 for now and change it later. |
|
I'd like to change CI build to use JDK 21 (similar to Ozone) in a follow-up. |
|
Thanks @symious and Yiyang for working on this! Thanks, @adoroszlai for reviewing this! |
What changes were proposed in this pull request?
This ticket is to update spotbugs version to 4.8.6, for the support of building with JDK21.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-2352
How was this patch tested?
CI check.