You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "szetszwo (via GitHub)" <gi...@apache.org> on 2025/12/15 20:37:20 UTC
[PR] HDDS-14175. DefaultVolumeChoosingPolicy should not call getCurrentUsage() multiple times. [ozone]
szetszwo opened a new pull request, #9505:
URL: https://github.com/apache/ozone/pull/9505
## 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 is that it has filtered the volumes > threshold first and then choose destVolume. The destVolume must also be > threshold, which is undesiable.
## What is the link to the Apache JIRA
HDDS-14175
## How was this patch tested?
By updating existing tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14175. DiskBalancer should not call getCurrentUsage() multiple times. [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #9505:
URL: https://github.com/apache/ozone/pull/9505#issuecomment-3667155550
> For this description, would you elaborate it a bit more? ...
Updated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14175. DefaultVolumeChoosingPolicy should not call getCurrentUsage() multiple times. [ozone]
Posted by "Gargi-jais11 (via GitHub)" <gi...@apache.org>.
Gargi-jais11 commented on code in PR #9505:
URL: https://github.com/apache/ozone/pull/9505#discussion_r2621790628
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/policy/DefaultVolumeChoosingPolicy.java:
##########
@@ -50,67 +51,43 @@ public DefaultVolumeChoosingPolicy(ReentrantLock globalLock) {
@Override
public Pair<HddsVolume, HddsVolume> chooseVolume(MutableVolumeSet volumeSet,
- double threshold, Map<HddsVolume, Long> deltaMap, long containerSize) {
+ double thresholdPercentage, Map<HddsVolume, Long> deltaMap, long containerSize) {
Review Comment:
The method parameter is named `thresholdPercentage` but the interface (`DiskBalancerVolumeChoosingPolicy`) uses `threshold`so the parameter name should match the interface for consistency. Could you please change in interface as well from `threshold` to `thresholdPercentage`.
Current code:
https://github.com/apache/ozone/blob/388084e44ac312c26d7efe113193bbd2a1a790ba/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/policy/DiskBalancerVolumeChoosingPolicy.java#L33-L39
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerVolumeCalculation.java:
##########
@@ -126,4 +125,47 @@ public static double calculateVolumeDataDensity(ImmutableList<HddsVolume> volume
return -1.0;
}
}
+
+ public static double computeUtilization(HddsVolume volume, long required) {
+ return computeUtilization(volume.getCurrentUsage(), volume.getCommittedBytes(), required);
Review Comment:
@szetszwo I think this calls `getCurrentUsage()` which can return different values on each call, potentially causing inconsistent behavior.
Its better to accept `VolumeFixedUsage` in`computeUtilization` rather than
`computeUtilization(HddsVolume volume, long required)`
What do you say about this?
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/policy/DefaultVolumeChoosingPolicy.java:
##########
@@ -50,67 +51,43 @@ public DefaultVolumeChoosingPolicy(ReentrantLock globalLock) {
@Override
public Pair<HddsVolume, HddsVolume> chooseVolume(MutableVolumeSet volumeSet,
- double threshold, Map<HddsVolume, Long> deltaMap, long containerSize) {
+ double thresholdPercentage, Map<HddsVolume, Long> deltaMap, long containerSize) {
lock.lock();
try {
// Create truly immutable snapshot of volumes to ensure consistency
- ImmutableList<HddsVolume> allVolumes = DiskBalancerVolumeCalculation.getImmutableVolumeSet(volumeSet);
-
+ final List<StorageVolume> allVolumes = volumeSet.getVolumesList();
if (allVolumes.size() < 2) {
return null; // Can't balance with less than 2 volumes.
}
-
- // Calculate ideal usage using the same immutable volume
- double idealUsage = DiskBalancerVolumeCalculation.getIdealUsage(allVolumes, deltaMap);
- // Threshold is given as a percentage
- double normalizedThreshold = threshold / 100;
- List<HddsVolume> volumes = allVolumes
- .stream()
- .filter(volume -> {
- SpaceUsageSource usage = volume.getCurrentUsage();
+ // Calculate usages and sort in ascending order of utilization
+ final List<VolumeFixedUsage> volumeUsages = allVolumes.stream()
+ .map(VolumeFixedUsage::new)
Review Comment:
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());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14175. DiskBalancer should not call getCurrentUsage() multiple times. [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #9505:
URL: https://github.com/apache/ozone/pull/9505#discussion_r2628482131
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/policy/DiskBalancerVolumeChoosingPolicy.java:
##########
@@ -30,11 +30,11 @@ public interface DiskBalancerVolumeChoosingPolicy {
* Choose a pair of volumes for balancing.
*
* @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.
Review Comment:
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` interchangingly 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].
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14175. DiskBalancer should not call getCurrentUsage() multiple times. [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #9505:
URL: https://github.com/apache/ozone/pull/9505#discussion_r2631619754
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/policy/DefaultVolumeChoosingPolicy.java:
##########
@@ -50,67 +52,43 @@ public DefaultVolumeChoosingPolicy(ReentrantLock globalLock) {
@Override
public Pair<HddsVolume, HddsVolume> chooseVolume(MutableVolumeSet volumeSet,
- double threshold, Map<HddsVolume, Long> deltaMap, long containerSize) {
+ double thresholdPercentage, Map<HddsVolume, Long> deltaMap, long containerSize) {
lock.lock();
try {
// Create truly immutable snapshot of volumes to ensure consistency
- ImmutableList<HddsVolume> allVolumes = DiskBalancerVolumeCalculation.getImmutableVolumeSet(volumeSet);
-
+ final List<StorageVolume> allVolumes = volumeSet.getVolumesList();
if (allVolumes.size() < 2) {
return null; // Can't balance with less than 2 volumes.
}
-
- // Calculate ideal usage using the same immutable volume
- double idealUsage = DiskBalancerVolumeCalculation.getIdealUsage(allVolumes, deltaMap);
- // Threshold is given as a percentage
- double normalizedThreshold = threshold / 100;
- List<HddsVolume> volumes = allVolumes
- .stream()
- .filter(volume -> {
- SpaceUsageSource usage = volume.getCurrentUsage();
Review Comment:
> ... 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14175. DiskBalancer should not call getCurrentUsage() multiple times. [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #9505:
URL: https://github.com/apache/ozone/pull/9505#discussion_r2625979384
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/policy/DiskBalancerVolumeChoosingPolicy.java:
##########
@@ -30,11 +30,11 @@ public interface DiskBalancerVolumeChoosingPolicy {
* Choose a pair of volumes for balancing.
*
* @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.
Review Comment:
Threshold range 0 and 100 are exclusive, so (0, 100).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14175. DiskBalancer should not call getCurrentUsage() multiple times. [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #9505:
URL: https://github.com/apache/ozone/pull/9505#discussion_r2629600277
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/policy/DiskBalancerVolumeChoosingPolicy.java:
##########
@@ -30,11 +30,11 @@ public interface DiskBalancerVolumeChoosingPolicy {
* Choose a pair of volumes for balancing.
*
* @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.
Review Comment:
Ya, "threshold.percentage" is more clear, and a overall check of threshold configuration is desired.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14175. DiskBalancer should not call getCurrentUsage() multiple times. [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #9505:
URL: https://github.com/apache/ozone/pull/9505#issuecomment-3676134528
@Gargi-jais11 , @ChenSammi , thanks for reviewing this!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14175. DiskBalancer should not call getCurrentUsage() multiple times. [ozone]
Posted by "Gargi-jais11 (via GitHub)" <gi...@apache.org>.
Gargi-jais11 commented on code in PR #9505:
URL: https://github.com/apache/ozone/pull/9505#discussion_r2633560748
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/policy/DefaultVolumeChoosingPolicy.java:
##########
@@ -50,67 +52,43 @@ public DefaultVolumeChoosingPolicy(ReentrantLock globalLock) {
@Override
public Pair<HddsVolume, HddsVolume> chooseVolume(MutableVolumeSet volumeSet,
- double threshold, Map<HddsVolume, Long> deltaMap, long containerSize) {
+ double thresholdPercentage, Map<HddsVolume, Long> deltaMap, long containerSize) {
lock.lock();
try {
// Create truly immutable snapshot of volumes to ensure consistency
- ImmutableList<HddsVolume> allVolumes = DiskBalancerVolumeCalculation.getImmutableVolumeSet(volumeSet);
-
+ final List<StorageVolume> allVolumes = volumeSet.getVolumesList();
if (allVolumes.size() < 2) {
return null; // Can't balance with less than 2 volumes.
}
-
- // Calculate ideal usage using the same immutable volume
- double idealUsage = DiskBalancerVolumeCalculation.getIdealUsage(allVolumes, deltaMap);
- // Threshold is given as a percentage
- double normalizedThreshold = threshold / 100;
- List<HddsVolume> volumes = allVolumes
- .stream()
- .filter(volume -> {
- SpaceUsageSource usage = volume.getCurrentUsage();
Review Comment:
sure @ChenSammi
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14175. DiskBalancer should not call getCurrentUsage() multiple times. [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #9505:
URL: https://github.com/apache/ozone/pull/9505#discussion_r2635967025
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/policy/DefaultVolumeChoosingPolicy.java:
##########
@@ -50,67 +52,43 @@ public DefaultVolumeChoosingPolicy(ReentrantLock globalLock) {
@Override
public Pair<HddsVolume, HddsVolume> chooseVolume(MutableVolumeSet volumeSet,
- double threshold, Map<HddsVolume, Long> deltaMap, long containerSize) {
+ double thresholdPercentage, Map<HddsVolume, Long> deltaMap, long containerSize) {
lock.lock();
try {
// Create truly immutable snapshot of volumes to ensure consistency
- ImmutableList<HddsVolume> allVolumes = DiskBalancerVolumeCalculation.getImmutableVolumeSet(volumeSet);
-
+ final List<StorageVolume> allVolumes = volumeSet.getVolumesList();
if (allVolumes.size() < 2) {
return null; // Can't balance with less than 2 volumes.
}
-
- // Calculate ideal usage using the same immutable volume
- double idealUsage = DiskBalancerVolumeCalculation.getIdealUsage(allVolumes, deltaMap);
- // Threshold is given as a percentage
- double normalizedThreshold = threshold / 100;
- List<HddsVolume> volumes = allVolumes
- .stream()
- .filter(volume -> {
- SpaceUsageSource usage = volume.getCurrentUsage();
Review Comment:
For the follow up JIRA, we should add the tests described in https://github.com/apache/ozone/pull/9505/changes#r2629666258
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14175. DefaultVolumeChoosingPolicy should not call getCurrentUsage() multiple times. [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #9505:
URL: https://github.com/apache/ozone/pull/9505#discussion_r2624055083
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/policy/DefaultVolumeChoosingPolicy.java:
##########
@@ -50,67 +51,43 @@ public DefaultVolumeChoosingPolicy(ReentrantLock globalLock) {
@Override
public Pair<HddsVolume, HddsVolume> chooseVolume(MutableVolumeSet volumeSet,
- double threshold, Map<HddsVolume, Long> deltaMap, long containerSize) {
+ double thresholdPercentage, Map<HddsVolume, Long> deltaMap, long containerSize) {
lock.lock();
try {
// Create truly immutable snapshot of volumes to ensure consistency
- ImmutableList<HddsVolume> allVolumes = DiskBalancerVolumeCalculation.getImmutableVolumeSet(volumeSet);
-
+ final List<StorageVolume> allVolumes = volumeSet.getVolumesList();
if (allVolumes.size() < 2) {
return null; // Can't balance with less than 2 volumes.
}
-
- // Calculate ideal usage using the same immutable volume
- double idealUsage = DiskBalancerVolumeCalculation.getIdealUsage(allVolumes, deltaMap);
- // Threshold is given as a percentage
- double normalizedThreshold = threshold / 100;
- List<HddsVolume> volumes = allVolumes
- .stream()
- .filter(volume -> {
- SpaceUsageSource usage = volume.getCurrentUsage();
+ // Calculate usages and sort in ascending order of utilization
+ final List<VolumeFixedUsage> volumeUsages = allVolumes.stream()
+ .map(VolumeFixedUsage::new)
Review Comment:
If we use getVolumeUsages(volumeSet), it will copy the list again. The list may has changed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14175. DiskBalancer should not call getCurrentUsage() multiple times. [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #9505:
URL: https://github.com/apache/ozone/pull/9505#discussion_r2633428774
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/policy/DefaultVolumeChoosingPolicy.java:
##########
@@ -50,67 +52,43 @@ public DefaultVolumeChoosingPolicy(ReentrantLock globalLock) {
@Override
public Pair<HddsVolume, HddsVolume> chooseVolume(MutableVolumeSet volumeSet,
- double threshold, Map<HddsVolume, Long> deltaMap, long containerSize) {
+ double thresholdPercentage, Map<HddsVolume, Long> deltaMap, long containerSize) {
lock.lock();
try {
// Create truly immutable snapshot of volumes to ensure consistency
- ImmutableList<HddsVolume> allVolumes = DiskBalancerVolumeCalculation.getImmutableVolumeSet(volumeSet);
-
+ final List<StorageVolume> allVolumes = volumeSet.getVolumesList();
if (allVolumes.size() < 2) {
return null; // Can't balance with less than 2 volumes.
}
-
- // Calculate ideal usage using the same immutable volume
- double idealUsage = DiskBalancerVolumeCalculation.getIdealUsage(allVolumes, deltaMap);
- // Threshold is given as a percentage
- double normalizedThreshold = threshold / 100;
- List<HddsVolume> volumes = allVolumes
- .stream()
- .filter(volume -> {
- SpaceUsageSource usage = volume.getCurrentUsage();
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14175. DiskBalancer should not call getCurrentUsage() multiple times. [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #9505:
URL: https://github.com/apache/ozone/pull/9505#discussion_r2629666258
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/policy/DefaultVolumeChoosingPolicy.java:
##########
@@ -50,67 +52,43 @@ public DefaultVolumeChoosingPolicy(ReentrantLock globalLock) {
@Override
public Pair<HddsVolume, HddsVolume> chooseVolume(MutableVolumeSet volumeSet,
- double threshold, Map<HddsVolume, Long> deltaMap, long containerSize) {
+ double thresholdPercentage, Map<HddsVolume, Long> deltaMap, long containerSize) {
lock.lock();
try {
// Create truly immutable snapshot of volumes to ensure consistency
- ImmutableList<HddsVolume> allVolumes = DiskBalancerVolumeCalculation.getImmutableVolumeSet(volumeSet);
-
+ final List<StorageVolume> allVolumes = volumeSet.getVolumesList();
if (allVolumes.size() < 2) {
return null; // Can't balance with less than 2 volumes.
}
-
- // Calculate ideal usage using the same immutable volume
- double idealUsage = DiskBalancerVolumeCalculation.getIdealUsage(allVolumes, deltaMap);
- // Threshold is given as a percentage
- double normalizedThreshold = threshold / 100;
- List<HddsVolume> volumes = allVolumes
- .stream()
- .filter(volume -> {
- SpaceUsageSource usage = volume.getCurrentUsage();
Review Comment:
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14175. DiskBalancer should not call getCurrentUsage() multiple times. [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #9505:
URL: https://github.com/apache/ozone/pull/9505#discussion_r2628482131
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/policy/DiskBalancerVolumeChoosingPolicy.java:
##########
@@ -30,11 +30,11 @@ public interface DiskBalancerVolumeChoosingPolicy {
* Choose a pair of volumes for balancing.
*
* @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.
Review Comment:
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].
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14175. DiskBalancer should not call getCurrentUsage() multiple times. [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi merged PR #9505:
URL: https://github.com/apache/ozone/pull/9505
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14175. DiskBalancer should not call getCurrentUsage() multiple times. [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on PR #9505:
URL: https://github.com/apache/ozone/pull/9505#issuecomment-3664147887
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14175. DiskBalancer should not call getCurrentUsage() multiple times. [ozone]
Posted by "Gargi-jais11 (via GitHub)" <gi...@apache.org>.
Gargi-jais11 commented on code in PR #9505:
URL: https://github.com/apache/ozone/pull/9505#discussion_r2625514899
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/policy/DefaultVolumeChoosingPolicy.java:
##########
@@ -50,67 +51,43 @@ public DefaultVolumeChoosingPolicy(ReentrantLock globalLock) {
@Override
public Pair<HddsVolume, HddsVolume> chooseVolume(MutableVolumeSet volumeSet,
- double threshold, Map<HddsVolume, Long> deltaMap, long containerSize) {
+ double thresholdPercentage, Map<HddsVolume, Long> deltaMap, long containerSize) {
lock.lock();
try {
// Create truly immutable snapshot of volumes to ensure consistency
- ImmutableList<HddsVolume> allVolumes = DiskBalancerVolumeCalculation.getImmutableVolumeSet(volumeSet);
-
+ final List<StorageVolume> allVolumes = volumeSet.getVolumesList();
if (allVolumes.size() < 2) {
return null; // Can't balance with less than 2 volumes.
}
-
- // Calculate ideal usage using the same immutable volume
- double idealUsage = DiskBalancerVolumeCalculation.getIdealUsage(allVolumes, deltaMap);
- // Threshold is given as a percentage
- double normalizedThreshold = threshold / 100;
- List<HddsVolume> volumes = allVolumes
- .stream()
- .filter(volume -> {
- SpaceUsageSource usage = volume.getCurrentUsage();
+ // Calculate usages and sort in ascending order of utilization
+ final List<VolumeFixedUsage> volumeUsages = allVolumes.stream()
+ .map(VolumeFixedUsage::new)
Review Comment:
Yaa correct that makes sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14175. DiskBalancer should not call getCurrentUsage() multiple times. [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #9505:
URL: https://github.com/apache/ozone/pull/9505#discussion_r2628432178
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/policy/DefaultVolumeChoosingPolicy.java:
##########
@@ -50,67 +52,43 @@ public DefaultVolumeChoosingPolicy(ReentrantLock globalLock) {
@Override
public Pair<HddsVolume, HddsVolume> chooseVolume(MutableVolumeSet volumeSet,
- double threshold, Map<HddsVolume, Long> deltaMap, long containerSize) {
+ double thresholdPercentage, Map<HddsVolume, Long> deltaMap, long containerSize) {
lock.lock();
try {
// Create truly immutable snapshot of volumes to ensure consistency
- ImmutableList<HddsVolume> allVolumes = DiskBalancerVolumeCalculation.getImmutableVolumeSet(volumeSet);
-
+ final List<StorageVolume> allVolumes = volumeSet.getVolumesList();
if (allVolumes.size() < 2) {
return null; // Can't balance with less than 2 volumes.
}
-
- // Calculate ideal usage using the same immutable volume
- double idealUsage = DiskBalancerVolumeCalculation.getIdealUsage(allVolumes, deltaMap);
- // Threshold is given as a percentage
- double normalizedThreshold = threshold / 100;
- List<HddsVolume> volumes = allVolumes
- .stream()
- .filter(volume -> {
- SpaceUsageSource usage = volume.getCurrentUsage();
Review Comment:
@ChenSammi , this is the bug filtering out < threashold volumes. Later on, it uses `volumes` list to find destVolume
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14175. DiskBalancer should not call getCurrentUsage() multiple times. [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #9505:
URL: https://github.com/apache/ozone/pull/9505#discussion_r2628482131
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/policy/DiskBalancerVolumeChoosingPolicy.java:
##########
@@ -30,11 +30,11 @@ public interface DiskBalancerVolumeChoosingPolicy {
* Choose a pair of volumes for balancing.
*
* @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.
Review Comment:
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].
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14175. DiskBalancer should not call getCurrentUsage() multiple times. [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #9505:
URL: https://github.com/apache/ozone/pull/9505#discussion_r2629600277
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/policy/DiskBalancerVolumeChoosingPolicy.java:
##########
@@ -30,11 +30,11 @@ public interface DiskBalancerVolumeChoosingPolicy {
* Choose a pair of volumes for balancing.
*
* @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.
Review Comment:
Ya, "threshold.percentage" is more clear, and a overall check of threshold configuration is desired. We can do this in a new JIRA.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14175. DefaultVolumeChoosingPolicy should not call getCurrentUsage() multiple times. [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #9505:
URL: https://github.com/apache/ozone/pull/9505#discussion_r2624086824
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerVolumeCalculation.java:
##########
@@ -126,4 +125,47 @@ public static double calculateVolumeDataDensity(ImmutableList<HddsVolume> volume
return -1.0;
}
}
+
+ public static double computeUtilization(HddsVolume volume, long required) {
+ return computeUtilization(volume.getCurrentUsage(), volume.getCommittedBytes(), required);
Review Comment:
For correctness, it is okay. But for performance, it is better to get it first and keep using the same object. Let me change it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14175. DefaultVolumeChoosingPolicy should not call getCurrentUsage() multiple times. [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #9505:
URL: https://github.com/apache/ozone/pull/9505#discussion_r2624086824
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerVolumeCalculation.java:
##########
@@ -126,4 +125,47 @@ public static double calculateVolumeDataDensity(ImmutableList<HddsVolume> volume
return -1.0;
}
}
+
+ public static double computeUtilization(HddsVolume volume, long required) {
+ return computeUtilization(volume.getCurrentUsage(), volume.getCommittedBytes(), required);
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org