ChenSammi commented on code in PR #9068:
URL: https://github.com/apache/ozone/pull/9068#discussion_r2418898105
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/policy/DefaultContainerChoosingPolicy.java:
##########
@@ -49,31 +51,70 @@ public class DefaultContainerChoosingPolicy implements
ContainerChoosingPolicy {
@Override
public ContainerData chooseContainer(OzoneContainer ozoneContainer,
- HddsVolume hddsVolume, Set<ContainerID> inProgressContainerIDs) {
+ HddsVolume srcVolume, HddsVolume destVolume,
+ Set<ContainerID> inProgressContainerIDs,
+ Double threshold, MutableVolumeSet volumeSet) {
Iterator<Container<?>> itr;
try {
- itr = CACHE.get().get(hddsVolume,
- () -> ozoneContainer.getController().getContainers(hddsVolume));
+ itr = CACHE.get().get(srcVolume,
+ () -> ozoneContainer.getController().getContainers(srcVolume));
} catch (ExecutionException e) {
- LOG.warn("Failed to get container iterator for volume {}", hddsVolume,
e);
+ LOG.warn("Failed to get container iterator for volume {}", srcVolume, e);
return null;
}
+ // Calculate maxAllowedUtilization
+ double idealUsage = volumeSet.getIdealUsage();
+ double maxAllowedUtilization = idealUsage + (threshold / 100.0);
+
while (itr.hasNext()) {
ContainerData containerData = itr.next().getContainerData();
if (!inProgressContainerIDs.contains(
ContainerID.valueOf(containerData.getContainerID())) &&
(containerData.isClosed() || (test &&
containerData.isQuasiClosed()))) {
- return containerData;
+
+ // This is a candidate container. Now, check if moving it would be
productive.
+ if (isMoveProductive(containerData, destVolume,
maxAllowedUtilization)) {
+ return containerData;
+ }
}
}
if (!itr.hasNext()) {
- CACHE.get().invalidate(hddsVolume);
+ CACHE.get().invalidate(srcVolume);
}
return null;
}
+ /**
+ * Checks if moving the given container from source to destination would
+ * result in the destination's utilization being less than or equal to the
+ * averageUtilization + threshold. This prevents "thrashing" where a move
+ * immediately makes the destination a candidate for a source volume.
+ *
+ * @param containerData The container to be moved.
+ * @param destVolume The destination volume.
+ * @param maxAllowedUtilization The maximum allowed utilization
+ * for the destination volume.
+ * @return true if the move is productive, false otherwise.
+ */
+ private boolean isMoveProductive(ContainerData containerData, HddsVolume
destVolume,
+ Double maxAllowedUtilization) {
+ long containerSize = containerData.getBytesUsed();
+ SpaceUsageSource usage = destVolume.getCurrentUsage();
+
+ double newDestUtilization =
+ (double) ((usage.getCapacity() - usage.getAvailable()) +
destVolume.getCommittedBytes() + containerSize)
+ / usage.getCapacity();
+
+ if (newDestUtilization <= maxAllowedUtilization) {
Review Comment:
@Gargi-jais11 , could you double check the whole disk balancer code, to see
whether threshold is used as an inclusive or exclusive value? If it's
inclusive, then "=" should be used. If it's exclusive, then we should not use
"=". Please check all codes, to make sure we have a consistent behavior.
--
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]
