somandal commented on code in PR #15175:
URL: https://github.com/apache/pinot/pull/15175#discussion_r1999729340
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/DefaultRebalancePreChecker.java:
##########
@@ -42,19 +47,27 @@ public class DefaultRebalancePreChecker implements
RebalancePreChecker {
public static final String NEEDS_RELOAD_STATUS = "needsReloadStatus";
public static final String IS_MINIMIZE_DATA_MOVEMENT =
"isMinimizeDataMovement";
+ public static final String DISK_UTILIZATION_FOOTPRINT =
"diskUtilizationFootprint";
+ public static final String DISK_UTILIZATION_AFTERWARD =
"diskUtilizationAfterward";
Review Comment:
nit: let's rename this to "diskUtilizationAfterRebalanceCompletion" just to
be more clear
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/DefaultRebalancePreChecker.java:
##########
@@ -184,4 +209,85 @@ private RebalancePreCheckerResult
checkIsMinimizeDataMovement(String rebalanceJo
return RebalancePreCheckerResult.error("Got exception when fetching
instance assignment, check manually");
}
}
+
+ private RebalancePreCheckerResult checkDiskUtilization(Map<String,
Map<String, String>> currentAssignment,
+ Map<String, Map<String, String>> targetAssignment,
+ TableSizeReader.TableSubTypeSizeDetails tableSubTypeSizeDetails, double
threshold, boolean worstCase) {
+ boolean isDiskUtilSafe = true;
+ StringBuilder message = new StringBuilder("UNSAFE. Servers with unsafe
disk util footprint: ");
+ String sep = "";
+ Map<String, Set<String>> existingServersToSegmentMap = new HashMap<>();
+ Map<String, Set<String>> newServersToSegmentMap = new HashMap<>();
+
+ for (Map.Entry<String, Map<String, String>> entrySet :
currentAssignment.entrySet()) {
+ for (String instanceName : entrySet.getValue().keySet()) {
+ existingServersToSegmentMap.computeIfAbsent(instanceName, k -> new
HashSet<>()).add(entrySet.getKey());
+ }
+ }
+
+ for (Map.Entry<String, Map<String, String>> entrySet :
targetAssignment.entrySet()) {
+ for (String instanceName : entrySet.getValue().keySet()) {
+ newServersToSegmentMap.computeIfAbsent(instanceName, k -> new
HashSet<>()).add(entrySet.getKey());
+ }
+ }
+
+ long avgSegmentSize = getAverageSegmentSize(tableSubTypeSizeDetails,
currentAssignment);
+
+ for (Map.Entry<String, Set<String>> entry :
newServersToSegmentMap.entrySet()) {
+ String server = entry.getKey();
+ DiskUsageInfo diskUsage = getDiskUsageInfoOfInstance(server);
+
+ if (diskUsage.getTotalSpaceBytes() < 0) {
+ return RebalancePreCheckerResult.warn(
+ "Disk usage info not enabled. Try later or set
controller.resource.utilization.checker.initial.delay to a"
Review Comment:
nit: should this be "Enable or set..."?
why would trying later help if this is disabled?
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java:
##########
@@ -4199,7 +4209,7 @@ public void testRebalanceDryRunSummary()
DefaultRebalancePreChecker.NEEDS_RELOAD_STATUS).getPreCheckStatus(),
RebalancePreCheckerResult.PreCheckStatus.PASS);
assertEquals(rebalanceResult.getPreChecksResult().get(
- DefaultRebalancePreChecker.IS_MINIMIZE_DATA_MOVEMENT).getMessage(),
+ DefaultRebalancePreChecker.IS_MINIMIZE_DATA_MOVEMENT).getMessage(),
Review Comment:
nit: unintended change here?
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java:
##########
@@ -227,6 +227,8 @@ public Map<String, Object>
getDefaultControllerConfiguration() {
properties.put(ControllerConf.DISABLE_GROOVY, false);
properties.put(ControllerConf.CONSOLE_SWAGGER_ENABLE, false);
properties.put(CommonConstants.CONFIG_OF_TIMEZONE, "UTC");
+ // Disable resource util check in test so that each test can set the
ResourceUtilizationInfo manually
+ properties.put(ControllerConf.RESOURCE_UTILIZATION_CHECKER_INITIAL_DELAY,
30_000);
Review Comment:
just thinking some more, instead of doing this for all tests, can you have
your specific test implement the following:
```
/**
* Can be overridden to add more properties.
*/
protected void overrideControllerConf(Map<String, Object> properties) {
}
```
And have the above do this instead?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/DefaultRebalancePreChecker.java:
##########
@@ -42,19 +47,27 @@ public class DefaultRebalancePreChecker implements
RebalancePreChecker {
public static final String NEEDS_RELOAD_STATUS = "needsReloadStatus";
public static final String IS_MINIMIZE_DATA_MOVEMENT =
"isMinimizeDataMovement";
+ public static final String DISK_UTILIZATION_FOOTPRINT =
"diskUtilizationFootprint";
Review Comment:
nit: let's rename this to "maxDiskUtilizationDuringRebalance" just to be
more clear
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
