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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
