HDDS-14175. DiskBalancer should not call getCurrentUsage() multiple times. #9505

szetszwo · 2025-12-15T20:37:18Z

What changes were proposed in this pull request?

In DefaultVolumeChoosingPolicy.chooseVolume(..), getCurrentUsage() is called multiple times so it can return different values. The biggest problem is the sorted(..) call. The code calls getCurrentUsage() in the comparator. The behavior is ill-defined for sorting changing values.
Another bug in the code below is that it has filtered the volumes > threshold first and then choose destVolume. The destVolume must also be > threshold, which is inccorrect.

Code filtering out volumes > threshold:

ozone/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/policy/DefaultVolumeChoosingPolicy.java

Lines 68 to 78 in 388084e

    
           List<HddsVolume> volumes = allVolumes 
        
               .stream() 
        
               .filter(volume -> { 
        
                 SpaceUsageSource usage = volume.getCurrentUsage(); 
        
                 return Math.abs( 
        
                       ((double)((usage.getCapacity() - usage.getAvailable()) 
        
                           + deltaMap.getOrDefault(volume, 0L) + volume.getCommittedBytes())) 
        
                           / usage.getCapacity() - idealUsage) >= normalizedThreshold; 
        
               }).sorted((v1, v2) -> {

Code choosing destVolume:

ozone/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/policy/DefaultVolumeChoosingPolicy.java

Lines 98 to 110 in 388084e

    
           HddsVolume destVolume = volumes.get(volumes.size() - 1); 
        
           while (!filter.test(destVolume)) { 
        
             // If the destination volume does not have enough space, try the next 
        
             // one in the list. 
        
             LOG.debug("Destination volume {} does not have enough space, trying next volume.", 
        
                 destVolume.getStorageID()); 
        
             volumes.remove(destVolume); 
        
             if (volumes.size() <= 1) { 
        
               LOG.debug("Can not find appropriate Source volume and Dest Volume."); 
        
               return null; 
        
             } 
        
             destVolume = volumes.get(volumes.size() - 1); 
        
           }

What is the link to the Apache JIRA

HDDS-14175

How was this patch tested?

By updating existing tests.

…ge() multiple times.

Gargi-jais11

Thanks @szetszwo for finding out these potential bugs. The patch overall LGTM.
Below are some comments.

Gargi-jais11 · 2025-12-16T05:00:29Z

.../java/org/apache/hadoop/ozone/container/diskbalancer/policy/DefaultVolumeChoosingPolicy.java

  @Override
  public Pair<HddsVolume, HddsVolume> chooseVolume(MutableVolumeSet volumeSet,
-      double threshold, Map<HddsVolume, Long> deltaMap, long containerSize) {
+      double thresholdPercentage, Map<HddsVolume, Long> deltaMap, long containerSize) {


The method parameter is named thresholdPercentage but the interface (DiskBalancerVolumeChoosingPolicy) uses thresholdso the parameter name should match the interface for consistency. Could you please change in interface as well from threshold to thresholdPercentage.

Current code:

ozone/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/policy/DiskBalancerVolumeChoosingPolicy.java

Lines 33 to 39 in 388084e

* @param threshold - the threshold to choose source and dest volumes.

* @param deltaSizes - the sizes changes of inProgress balancing jobs.

* @param containerSize - the estimated size of container to be moved.

* @return Source volume and Dest volume.

*/

Pair<HddsVolume, HddsVolume> chooseVolume(MutableVolumeSet volumeSet,

double threshold, Map<HddsVolume, Long> deltaSizes, long containerSize);

Gargi-jais11 · 2025-12-16T05:14:58Z

.../java/org/apache/hadoop/ozone/container/diskbalancer/policy/DefaultVolumeChoosingPolicy.java

+      // Calculate usages and sort in ascending order of utilization
+      final List<VolumeFixedUsage> volumeUsages = allVolumes.stream()
+          .map(VolumeFixedUsage::new)


I think here as well we should use getVolumeUsages() utility method that exists in DiskBalancerVolumeCalculation instead of creating inline VolumeFixedUsage objects.

final List<VolumeFixedUsage> volumeUsages = getVolumeUsages(volumeSet).stream() .sorted(Comparator.comparingDouble(v -> v.computeUtilization(deltaMap))) .collect(Collectors.toList());

If we use getVolumeUsages(volumeSet), it will copy the list again. The list may has changed.

Yaa correct that makes sense.

Gargi-jais11 · 2025-12-16T05:19:26Z

.../main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerVolumeCalculation.java

+  public static double computeUtilization(HddsVolume volume, long required) {
+    return computeUtilization(volume.getCurrentUsage(), volume.getCommittedBytes(), required);


@szetszwo I think this calls getCurrentUsage() which can return different values on each call, potentially causing inconsistent behavior.
Its better to accept VolumeFixedUsage incomputeUtilization rather than
computeUtilization(HddsVolume volume, long required)

What do you say about this?

For correctness, it is okay since it is used in choosing container. But for performance, it is better to get it first and keep using the same object. Let me change it.

ChenSammi · 2025-12-17T07:55:51Z

.../org/apache/hadoop/ozone/container/diskbalancer/policy/DiskBalancerVolumeChoosingPolicy.java

   *
   * @param volumeSet - volumes to choose from.
-   * @param threshold - the threshold to choose source and dest volumes.
+   * @param thresholdPercentage the threshold percentage in range [0, 100] to choose the source volume.


Threshold range 0 and 100 are exclusive, so (0, 100).

For double, the end values are unimportant except for spacial reasons such as division-by-zeros. For example, why 0.001 and 99.999 are allowed but not 0 and 100?

BTW, 0 and 100 should be allowed in this case

0: balance any volumes > average.

100: disable it in this method.

If we want to disallow them, we should validate the conf and throw exceptions instead of relying on the javadoc.

In the javadoc here, I just want to emphasize that the threshold here is a percentage [0,100] but not a ratio [0,1]. The previous code use the word "threshold" interchangeably for both percentage and ratio. (Currently, some other code may still do that.)

Actually, it is better to change the conf to understand percentage -- i.e. users can use both "0.1" and "10%" for the same value. After parsing the conf, the code always treat threshold as a ratio [0,1].

Ya, "threshold.percentage" is more clear, and a overall check of threshold configuration is desired. We can do this in a new JIRA.

ChenSammi · 2025-12-17T08:07:30Z

@szetszwo, thanks for this improvement. Overall it looks great. Just one minor comment about the threshold range comment.

Another bug is that it has filtered the volumes > threshold first and then choose destVolume. The destVolume must also be > threshold, which is undesiable.

For this description, would you elaborate it a bit more? IIUC, it seems not the case.

szetszwo · 2025-12-17T19:51:00Z

.../java/org/apache/hadoop/ozone/container/diskbalancer/policy/DefaultVolumeChoosingPolicy.java


-      // Threshold is given as a percentage
-      double normalizedThreshold = threshold / 100;
-      List<HddsVolume> volumes = allVolumes
-          .stream()
-          .filter(volume -> {
-            SpaceUsageSource usage = volume.getCurrentUsage();


@ChenSammi , this is the bug filtering out < threashold volumes. Later on, it uses volumes list to find destVolume

@szetszwo, I got the point after a second thought. My previous understanding of this filtering out < threashold volumes is it tries to implement a high efficient way of selecting destVolume, as a straightforward thinking is, if there is one volume beyond the utilization threshold, there is likely one volume below the utilization threshold, but realized that actually there are other cases, that there is one volume beyond threshold and no volumes under threshold, or there is one volume under threshold and no volumes beyond threshold,

(1) one volume beyond threshold and no volumes under threshold

Disk1, 30, 100 Disk2, 30, 100 Disk3, 40, 100 100 / 300 = 33.3% Disk1: 30% Disk2: 30% Disk3: 40% Threshold: 10 Disk utilization range (23.3, 43.3) Out range volume list: NULL Threshold: 5 Disk utilization range (28.3, 38.3) Out range volume list: Disk3

(2) one volume under threshold and no volumes beyond threshold

Disk1, 30, 100 Disk2, 30, 100 Disk3, 20, 100 80 / 300 = 26.7% Disk1: 30% Disk2: 30% Disk3: 20% Threshold: 10 Disk utilization range (16.7, 36.7) Out range volume list: NULL Threshold: 5 Disk utilization range (21.7, 31.7) Out range volume list: Disk3

So above two cases, are typical cases which are not covered by existing logic. And it looks like case (2) is not covered by new logic.

... And it looks like case (2) is not covered by new logic.

@ChenSammi , both the old and the new code won't balance case (2). I thought it is the expectation. No?

In your comment, there are upper and lower thresholds but, in the (old) code, it only considers the upper the threshold but never uses the lower threshold.

BTW, I am not trying to change logic here. Just want to fix the bugs described.

Make sense. Will address case (2) with a new JIRA. @Gargi-jais11 , could you open a new follow up JIRA for this?

Thanks for pointing out the issue.

sure @ChenSammi

For the follow up JIRA, we should add the tests described in https://github.com/apache/ozone/pull/9505/changes#r2629666258

szetszwo · 2025-12-17T21:04:58Z

For this description, would you elaborate it a bit more? ...

Updated

ChenSammi

Thanks @szetszwo .

szetszwo · 2025-12-19T18:45:38Z

@Gargi-jais11 , @ChenSammi , thanks for reviewing this!

szetszwo added 3 commits December 15, 2025 12:32

HDDS-14175. DefaultVolumeChoosingPolicy should not call getCurrentUsa…

15226eb

…ge() multiple times.

Do not use AvailableSpaceFilter since it calls getCurrentUsage().

b9e0bf4

Fix a bug

990924a

ivandika3 requested a review from ChenSammi December 16, 2025 01:41

Gargi-jais11 reviewed Dec 16, 2025

View reviewed changes

szetszwo added 2 commits December 16, 2025 10:24

Address review comments and get delta only once.

905e0f4

Fix checkstyle

c106e8c

szetszwo changed the title ~~HDDS-14175. DefaultVolumeChoosingPolicy should not call getCurrentUsage() multiple times.~~ HDDS-14175. DiskBalancer should not call getCurrentUsage() multiple times. Dec 16, 2025

ChenSammi reviewed Dec 17, 2025

View reviewed changes

szetszwo commented Dec 17, 2025

View reviewed changes

ChenSammi approved these changes Dec 19, 2025

View reviewed changes

ChenSammi merged commit 2abd751 into apache:HDDS-5713 Dec 19, 2025
47 checks passed

	List<HddsVolume> volumes = allVolumes
	.stream()
	.filter(volume -> {
	SpaceUsageSource usage = volume.getCurrentUsage();

	return Math.abs(
	((double)((usage.getCapacity() - usage.getAvailable())
	+ deltaMap.getOrDefault(volume, 0L) + volume.getCommittedBytes()))
	/ usage.getCapacity() - idealUsage) >= normalizedThreshold;

	}).sorted((v1, v2) -> {

	HddsVolume destVolume = volumes.get(volumes.size() - 1);
	while (!filter.test(destVolume)) {
	// If the destination volume does not have enough space, try the next
	// one in the list.
	LOG.debug("Destination volume {} does not have enough space, trying next volume.",
	destVolume.getStorageID());
	volumes.remove(destVolume);
	if (volumes.size() <= 1) {
	LOG.debug("Can not find appropriate Source volume and Dest Volume.");
	return null;
	}
	destVolume = volumes.get(volumes.size() - 1);
	}

	* @param threshold - the threshold to choose source and dest volumes.
	* @param deltaSizes - the sizes changes of inProgress balancing jobs.
	* @param containerSize - the estimated size of container to be moved.
	* @return Source volume and Dest volume.
	*/
	Pair<HddsVolume, HddsVolume> chooseVolume(MutableVolumeSet volumeSet,
	double threshold, Map<HddsVolume, Long> deltaSizes, long containerSize);

		public static double computeUtilization(HddsVolume volume, long required) {
		return computeUtilization(volume.getCurrentUsage(), volume.getCommittedBytes(), required);

HDDS-14175. DiskBalancer should not call getCurrentUsage() multiple times. #9505

HDDS-14175. DiskBalancer should not call getCurrentUsage() multiple times. #9505

Uh oh!

Conversation

szetszwo commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

Gargi-jais11 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szetszwo Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szetszwo Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChenSammi Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChenSammi commented Dec 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szetszwo commented Dec 17, 2025

Uh oh!

ChenSammi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

szetszwo commented Dec 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

szetszwo commented Dec 15, 2025 •

edited

Loading

szetszwo Dec 16, 2025 •

edited

Loading

szetszwo Dec 17, 2025 •

edited

Loading

ChenSammi Dec 18, 2025 •

edited

Loading