HDDS-16084. Add ErrorProne to Ozone CI - #11056
Conversation
ivandika3
changed the title
ivandika3
changed the title
| omKeyLocationInfoGroup.getLocationList() | ||
| .stream().map(omKeyLocationInfo -> pipelines.add( | ||
| omKeyLocationInfo.getPipeline()))); | ||
| omKeyInfo.getKeyLocationVersions().forEach(omKeyLocationInfoGroup -> |
There was a problem hiding this comment.
Not a test-lint fix — getPipelines() returned empty before (the old stream().map(...add...) was never consumed), so this actually changes what Recon reports. Please add a test for a non-empty pipeline list and call the behavior fix out in the description instead of under "Fix tests".
| if (resp.getStatus() != OK) { | ||
| throw new OMException(resp.getMessage(), | ||
| ResultCodes.values()[resp.getStatus().ordinal()]); | ||
| ResultCodes.valueOf(resp.getStatus().name())); |
There was a problem hiding this comment.
valueOf(name()) is the right pattern, but this changes error-code translation for any status where the Status and ResultCodes ordinals differ. Is this an intended behavior fix? If so, a small test (a status past the first divergence) + a note in the description would help.
|
@ivandika3 thanks for the patch! |
smengcl
left a comment
smengcl
left a comment
There was a problem hiding this comment.
Thanks @ivandika3 for the addition.
| declare -i rc | ||
|
|
||
| #shellcheck disable=SC2086 | ||
| mvn $MAVEN_OPTIONS test-compile "$@" | tee "${REPORT_DIR}/output.log" |
There was a problem hiding this comment.
Should this be clean test-compile? If the classes were previously compiled without the errorprone profile, Maven considers them up to date and skips javac, so Error Prone does not run.
| mvn $MAVEN_OPTIONS test-compile "$@" | tee "${REPORT_DIR}/output.log" | |
| mvn $MAVEN_OPTIONS clean test-compile "$@" | tee "${REPORT_DIR}/output.log" |
| grep -E "^\[(ERROR|WARNING)\] .*:\[[0-9]+,[0-9]+\] \[[A-Za-z][A-Za-z0-9]+\]" \ | ||
| "${REPORT_DIR}/output.log" | awk '!seen[$0]++' > "$REPORT_FILE" |
There was a problem hiding this comment.
It is guaranteed that forked javac always emit errorprone diagnostics with [WARNING] or [ERROR] prefix?
3 participants
What changes were proposed in this pull request?
ErrorProne is a well-known static analysis tool used in other ASF projects such as Celeborn, Druid, HBase, Solr, Beam, etc used to catch common Java coding errors. It is pretty lightweight compared to fbinfer (HDDS-15560).
This patch adds ErrorProne to the Ozone CI that fails if there are any ERROR bug patterns found (https://errorprone.info/bugpatterns), where as WARNING are included in the summary.txt but does not fail the CI.
This patch also fixes all the ERROR bug patterns found in the current Ozone codebase. The fixes are split into multiple commits. I have attached errorprone-error-fixes.md for the bug patterns and the possible risks. This seems to catch some real bugs such as in
CompactionDagandReconContainerMetadataManagerImplas well as noop assertions.Note that the WARNING issues are currently not fixed and we can raise follow up tasks to handle these. Additionally, we currently do not have any ErrorProne
ThreadSafeannotation. We can add this in the future to catch thread safety violation. ThisThreadSafeannotation can hopefully work in conjunction with fbinfer HDDS-15560.Note for reviewer:
ProtobufBufferOrdinalrule (https://errorprone.info/bugpattern/ProtocolBufferOrdinal), but seems currently theResultCodes.valueOf(protoStatus.name())should be safe.Generated by: GPT 5.6 Sol
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-16084
How was this patch tested?
CI (Clean CI: https://github.com/ivandika3/ozone/actions/runs/32216855338).