Skip to content

HDDS-15275. DiskBalancer getIdealUsage handles zero capacity incorrectly.#10267

Merged
szetszwo merged 2 commits into
apache:masterfrom
slfan1989:HDDS-15275
May 15, 2026
Merged

HDDS-15275. DiskBalancer getIdealUsage handles zero capacity incorrectly.#10267
szetszwo merged 2 commits into
apache:masterfrom
slfan1989:HDDS-15275

Conversation

@slfan1989
Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

This PR makes DiskBalancerVolumeCalculation#getIdealUsage handle empty volume lists and zero total capacity explicitly.

Previously, getIdealUsage directly divided totalEffectiveUsed by totalCapacity. When the volume list was empty, totalCapacity was 0, which caused the method to return NaN. When all volumes had zero capacity, the method could also produce invalid utilization values. These invalid values could propagate to DiskBalancer reporting and balancing calculations.

What is the link to the Apache JIRA

JIRA: HDDS-15275. DiskBalancer getIdealUsage handles zero capacity incorrectly.

How was this patch tested?

Add Junit Test.

Copy link
Copy Markdown
Contributor

@Gargi-jais11 Gargi-jais11 left a comment

Choose a reason for hiding this comment

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

Thanks @slfan1989 for the patch. Nice catch! LGTM!

@Gargi-jais11 Gargi-jais11 requested a review from szetszwo May 14, 2026 14:09
Copy link
Copy Markdown
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

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

@slfan1989 , thanks for working on this! Please see the comments inlined.


for (VolumeFixedUsage volumeUsage : volumes) {
totalCapacity += volumeUsage.getUsage().getCapacity();
totalEffectiveUsed += volumeUsage.getEffectiveUsed();
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.

Let's check each volume and check more cases:

    for (VolumeFixedUsage volumeUsage : volumes) {
      final long capacity = volumeUsage.getUsage().getCapacity();
      if (capacity < 0) {
        throw new IllegalArgumentException("Negative capacity = " + capacity
            + ": " + volumeUsage.getVolume());
      }
      final long effectiveUsed = volumeUsage.getEffectiveUsed();
      if (effectiveUsed < 0) {
        throw new IllegalArgumentException("Negative effective used = " + effectiveUsed
            + ": " + volumeUsage.getVolume());
      }
      if (effectiveUsed > capacity) {
        throw new IllegalArgumentException("Effective used = " + effectiveUsed + " > capacity = " + capacity
            + ": " + volumeUsage.getVolume());
      }
      totalCapacity += capacity;
      totalEffectiveUsed += effectiveUsed;
    }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@szetszwo Thanks for the suggestion! Addressed by validating each volume’s capacity and effective used before calculating ideal usage, and added tests for the new edge cases.

}


if (totalCapacity <= 0) {
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.

To be consistent, if (totalCapacity == 0), let's return 0?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated. totalCapacity == 0 now returns 0.0 for consistency with the empty volume case.

@Gargi-jais11
Copy link
Copy Markdown
Contributor

Last patch LGTM, +1.

Copy link
Copy Markdown
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

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

+1 the change looks good.

@szetszwo szetszwo merged commit 0ded782 into apache:master May 15, 2026
47 checks passed
@slfan1989
Copy link
Copy Markdown
Contributor Author

@szetszwo @Gargi-jais11 Thanks for the review and for merging the patch!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants