Skip to content

HDDS-16084. Add ErrorProne to Ozone CI - #11056

Open
ivandika3 wants to merge 10 commits into
apache:masterfrom
ivandika3:HDDS-16084
Open

HDDS-16084. Add ErrorProne to Ozone CI#11056
ivandika3 wants to merge 10 commits into
apache:masterfrom
ivandika3:HDDS-16084

Conversation

@ivandika3

@ivandika3 ivandika3 commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

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 CompactionDag and ReconContainerMetadataManagerImpl as 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 ThreadSafe annotation. We can add this in the future to catch thread safety violation. This ThreadSafe annotation can hopefully work in conjunction with fbinfer HDDS-15560.

Note for reviewer:

  • The possible risk is at the ProtobufBufferOrdinal rule (https://errorprone.info/bugpattern/ProtocolBufferOrdinal), but seems currently the ResultCodes.valueOf(protoStatus.name()) should be safe.
  • If we were to merge this, we might need to merge master to all the ongoing PRs since otherwise some the commit might fail when it's merged to master.

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).

@ivandika3
ivandika3 requested a review from Tejaskriya August 19, 2026 01:54
@ivandika3 ivandika3 changed the title HDDS-16084. Add ErrorProne into Ozone verification pipeline HDDS-16084. Add ErrorProne into Ozone CI Aug 19, 2026
@ivandika3 ivandika3 changed the title HDDS-16084. Add ErrorProne into Ozone CI HDDS-16084. Add ErrorProne to Ozone CI Aug 19, 2026
@ivandika3 ivandika3 added test build Pull request that modifies the build process labels Aug 19, 2026
@ivandika3
ivandika3 requested a review from smengcl August 19, 2026 09:44
@ivandika3 ivandika3 self-assigned this Aug 19, 2026
@ivandika3
ivandika3 requested a review from adoroszlai August 19, 2026 10:24
@ivandika3
ivandika3 marked this pull request as ready for review August 19, 2026 16:06

@rich7420 rich7420 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ErrorProne gate looks well-built and CI is green. One thing: a few "cleanup" fixes are actually behavior changes / real bug fixes bundled in a lint PR worth a test and a description note. Two comments beloww

omKeyLocationInfoGroup.getLocationList()
.stream().map(omKeyLocationInfo -> pipelines.add(
omKeyLocationInfo.getPipeline())));
omKeyInfo.getKeyLocationVersions().forEach(omKeyLocationInfoGroup ->

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rich7420

Copy link
Copy Markdown
Contributor

@ivandika3 thanks for the patch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build Pull request that modifies the build process test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants