yashmayya commented on code in PR #19176:
URL: https://github.com/apache/pinot/pull/19176#discussion_r3733558866
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/DefaultRebalancePreChecker.java:
##########
@@ -327,22 +332,51 @@ protected RebalancePreCheckerResult
checkDiskUtilization(PreCheckContext preChec
long diskUtilizationGain = newSegmentSet.size() * avgSegmentSize;
long diskUtilizationLoss = removedSegmentSet.size() * avgSegmentSize;
- long diskUtilizationFootprint =
- diskUsage.getUsedSpaceBytes() + diskUtilizationGain - (worstCase ? 0
: diskUtilizationLoss);
- double diskUtilizationFootprintRatio =
- (double) diskUtilizationFootprint / diskUsage.getTotalSpaceBytes();
-
- if (diskUtilizationFootprintRatio >= threshold) {
- isDiskUtilSafe = false;
- message.append(sep)
- .append(server)
- .append(String.format(" (%d%%)", (short)
(diskUtilizationFootprintRatio * 100)));
- sep = ", ";
- }
+ // While the rebalance is running, the segments being added can co-exist
with the ones being removed
+ addIfOverThreshold(serversUnsafeDuringRebalance, server,
+ (double) (diskUsage.getUsedSpaceBytes() + diskUtilizationGain) /
diskUsage.getTotalSpaceBytes(), threshold);
+ addIfOverThreshold(serversUnsafeAfterRebalance, server,
+ (double) (diskUsage.getUsedSpaceBytes() + diskUtilizationGain -
diskUtilizationLoss)
+ / diskUsage.getTotalSpaceBytes(), threshold);
+ }
+
+ // A server over the threshold once the rebalance is done is over it
during the rebalance as well, so the end state
+ // is what to report first: it is both the more severe problem and the one
that has to be solved by adding capacity
+ // rather than by tuning the rebalance config
+ if (!serversUnsafeAfterRebalance.isEmpty()) {
+ return RebalancePreCheckerResult.error(
+ getUnsafeDiskUtilizationMessage("AFTER rebalance",
serversUnsafeAfterRebalance, threshold));
+ }
+ String withinThreshold = String.format("Within threshold (<%d%%)", (short)
(threshold * 100));
+ if (serversUnsafeDuringRebalance.isEmpty()) {
+ return RebalancePreCheckerResult.pass(withinThreshold);
}
- return isDiskUtilSafe ? RebalancePreCheckerResult.pass(
- String.format("Within threshold (<%d%%)", (short) (threshold * 100)))
- : RebalancePreCheckerResult.error(message.toString());
+ // lowDiskMode is the only way to rule the transient disk usage above out,
since it waits for the segments to be
+ // deleted before adding the new ones. It is however only honored by the
incremental rebalance path, which downtime
+ // skips altogether by replacing the IdealState with the target assignment
in one go
+ RebalanceConfig rebalanceConfig = preCheckContext.getRebalanceConfig();
+ if (rebalanceConfig.isLowDiskMode() && !rebalanceConfig.isDowntime()) {
+ return RebalancePreCheckerResult.pass(withinThreshold + " AFTER
rebalance. Some servers would go over it DURING "
Review Comment:
Might be useful to log which servers
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/DefaultRebalancePreChecker.java:
##########
@@ -327,22 +332,51 @@ protected RebalancePreCheckerResult
checkDiskUtilization(PreCheckContext preChec
long diskUtilizationGain = newSegmentSet.size() * avgSegmentSize;
long diskUtilizationLoss = removedSegmentSet.size() * avgSegmentSize;
- long diskUtilizationFootprint =
- diskUsage.getUsedSpaceBytes() + diskUtilizationGain - (worstCase ? 0
: diskUtilizationLoss);
- double diskUtilizationFootprintRatio =
- (double) diskUtilizationFootprint / diskUsage.getTotalSpaceBytes();
-
- if (diskUtilizationFootprintRatio >= threshold) {
- isDiskUtilSafe = false;
- message.append(sep)
- .append(server)
- .append(String.format(" (%d%%)", (short)
(diskUtilizationFootprintRatio * 100)));
- sep = ", ";
- }
+ // While the rebalance is running, the segments being added can co-exist
with the ones being removed
+ addIfOverThreshold(serversUnsafeDuringRebalance, server,
+ (double) (diskUsage.getUsedSpaceBytes() + diskUtilizationGain) /
diskUsage.getTotalSpaceBytes(), threshold);
Review Comment:
We'll be flagging servers where the existing used space is over the
threshold even if `diskUtilizationGain == 0`? Seems wrong to say `unsafe DURING
rebalance` / recommend enabling `lowDiskMode` which would do nothing in such a
scenario.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/DefaultRebalancePreChecker.java:
##########
@@ -327,22 +332,51 @@ protected RebalancePreCheckerResult
checkDiskUtilization(PreCheckContext preChec
long diskUtilizationGain = newSegmentSet.size() * avgSegmentSize;
long diskUtilizationLoss = removedSegmentSet.size() * avgSegmentSize;
- long diskUtilizationFootprint =
- diskUsage.getUsedSpaceBytes() + diskUtilizationGain - (worstCase ? 0
: diskUtilizationLoss);
- double diskUtilizationFootprintRatio =
- (double) diskUtilizationFootprint / diskUsage.getTotalSpaceBytes();
-
- if (diskUtilizationFootprintRatio >= threshold) {
- isDiskUtilSafe = false;
- message.append(sep)
- .append(server)
- .append(String.format(" (%d%%)", (short)
(diskUtilizationFootprintRatio * 100)));
- sep = ", ";
- }
+ // While the rebalance is running, the segments being added can co-exist
with the ones being removed
+ addIfOverThreshold(serversUnsafeDuringRebalance, server,
+ (double) (diskUsage.getUsedSpaceBytes() + diskUtilizationGain) /
diskUsage.getTotalSpaceBytes(), threshold);
+ addIfOverThreshold(serversUnsafeAfterRebalance, server,
+ (double) (diskUsage.getUsedSpaceBytes() + diskUtilizationGain -
diskUtilizationLoss)
+ / diskUsage.getTotalSpaceBytes(), threshold);
+ }
+
+ // A server over the threshold once the rebalance is done is over it
during the rebalance as well, so the end state
+ // is what to report first: it is both the more severe problem and the one
that has to be solved by adding capacity
+ // rather than by tuning the rebalance config
+ if (!serversUnsafeAfterRebalance.isEmpty()) {
+ return RebalancePreCheckerResult.error(
+ getUnsafeDiskUtilizationMessage("AFTER rebalance",
serversUnsafeAfterRebalance, threshold));
+ }
+ String withinThreshold = String.format("Within threshold (<%d%%)", (short)
(threshold * 100));
+ if (serversUnsafeDuringRebalance.isEmpty()) {
+ return RebalancePreCheckerResult.pass(withinThreshold);
}
- return isDiskUtilSafe ? RebalancePreCheckerResult.pass(
- String.format("Within threshold (<%d%%)", (short) (threshold * 100)))
- : RebalancePreCheckerResult.error(message.toString());
+ // lowDiskMode is the only way to rule the transient disk usage above out,
since it waits for the segments to be
+ // deleted before adding the new ones. It is however only honored by the
incremental rebalance path, which downtime
+ // skips altogether by replacing the IdealState with the target assignment
in one go
+ RebalanceConfig rebalanceConfig = preCheckContext.getRebalanceConfig();
+ if (rebalanceConfig.isLowDiskMode() && !rebalanceConfig.isDowntime()) {
+ return RebalancePreCheckerResult.pass(withinThreshold + " AFTER
rebalance. Some servers would go over it DURING "
+ + "the rebalance, but lowDiskMode avoids that transient disk usage");
+ }
+ return RebalancePreCheckerResult.error(
+ getUnsafeDiskUtilizationMessage("DURING rebalance",
serversUnsafeDuringRebalance, threshold) + (
+ rebalanceConfig.isLowDiskMode()
+ ? ". lowDiskMode has no effect while downtime is enabled,
disable downtime for it to delete segments "
+ + "before adding the new ones"
+ : ". Enable lowDiskMode to delete segments before adding the
new ones"));
+ }
+
+ private static void addIfOverThreshold(List<String> servers, String server,
double utilizationRatio,
+ double threshold) {
+ if (utilizationRatio >= threshold) {
+ servers.add(server + String.format(" (%d%%)", (short) (utilizationRatio
* 100)));
+ }
+ }
+
+ private static String getUnsafeDiskUtilizationMessage(String when,
List<String> servers, double threshold) {
+ return String.format("UNSAFE. Servers with unsafe disk utilization %s
(>%d%%): %s", when, (short) (threshold * 100),
Review Comment:
> `>`
Should it be `>=`?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/DefaultRebalancePreChecker.java:
##########
@@ -327,22 +332,51 @@ protected RebalancePreCheckerResult
checkDiskUtilization(PreCheckContext preChec
long diskUtilizationGain = newSegmentSet.size() * avgSegmentSize;
long diskUtilizationLoss = removedSegmentSet.size() * avgSegmentSize;
- long diskUtilizationFootprint =
- diskUsage.getUsedSpaceBytes() + diskUtilizationGain - (worstCase ? 0
: diskUtilizationLoss);
- double diskUtilizationFootprintRatio =
- (double) diskUtilizationFootprint / diskUsage.getTotalSpaceBytes();
-
- if (diskUtilizationFootprintRatio >= threshold) {
- isDiskUtilSafe = false;
- message.append(sep)
- .append(server)
- .append(String.format(" (%d%%)", (short)
(diskUtilizationFootprintRatio * 100)));
- sep = ", ";
- }
+ // While the rebalance is running, the segments being added can co-exist
with the ones being removed
+ addIfOverThreshold(serversUnsafeDuringRebalance, server,
+ (double) (diskUsage.getUsedSpaceBytes() + diskUtilizationGain) /
diskUsage.getTotalSpaceBytes(), threshold);
+ addIfOverThreshold(serversUnsafeAfterRebalance, server,
+ (double) (diskUsage.getUsedSpaceBytes() + diskUtilizationGain -
diskUtilizationLoss)
+ / diskUsage.getTotalSpaceBytes(), threshold);
+ }
+
+ // A server over the threshold once the rebalance is done is over it
during the rebalance as well, so the end state
+ // is what to report first: it is both the more severe problem and the one
that has to be solved by adding capacity
+ // rather than by tuning the rebalance config
+ if (!serversUnsafeAfterRebalance.isEmpty()) {
+ return RebalancePreCheckerResult.error(
+ getUnsafeDiskUtilizationMessage("AFTER rebalance",
serversUnsafeAfterRebalance, threshold));
+ }
+ String withinThreshold = String.format("Within threshold (<%d%%)", (short)
(threshold * 100));
+ if (serversUnsafeDuringRebalance.isEmpty()) {
+ return RebalancePreCheckerResult.pass(withinThreshold);
}
- return isDiskUtilSafe ? RebalancePreCheckerResult.pass(
- String.format("Within threshold (<%d%%)", (short) (threshold * 100)))
- : RebalancePreCheckerResult.error(message.toString());
+ // lowDiskMode is the only way to rule the transient disk usage above out,
since it waits for the segments to be
+ // deleted before adding the new ones. It is however only honored by the
incremental rebalance path, which downtime
+ // skips altogether by replacing the IdealState with the target assignment
in one go
+ RebalanceConfig rebalanceConfig = preCheckContext.getRebalanceConfig();
+ if (rebalanceConfig.isLowDiskMode() && !rebalanceConfig.isDowntime()) {
Review Comment:
Should we also check for `bestEfforts` which seems to also potentially
violate `lowDiskMode`?
--
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]
