J-HowHuang commented on code in PR #15233:
URL: https://github.com/apache/pinot/pull/15233#discussion_r1989780089
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/DefaultRebalancePreChecker.java:
##########
@@ -53,16 +53,18 @@ public void init(PinotHelixResourceManager
pinotHelixResourceManager, @Nullable
}
@Override
- public Map<String, String> check(String rebalanceJobId, String
tableNameWithType, TableConfig tableConfig) {
+ public Map<String, RebalancePreCheckerResult> check(String rebalanceJobId,
String tableNameWithType,
+ TableConfig tableConfig) {
LOGGER.info("Start pre-checks for table: {} with rebalanceJobId: {}",
tableNameWithType, rebalanceJobId);
- Map<String, String> preCheckResult = new HashMap<>();
+ Map<String, RebalancePreCheckerResult> preCheckResult = new HashMap<>();
// Check for reload status
- Boolean needsReload = checkReloadNeededOnServers(rebalanceJobId,
tableNameWithType);
- preCheckResult.put(NEEDS_RELOAD_STATUS, needsReload == null ? "error" :
String.valueOf(needsReload));
+ RebalancePreCheckerResult needsReloadMessage =
checkReloadNeededOnServers(rebalanceJobId, tableNameWithType);
+ preCheckResult.put(NEEDS_RELOAD_STATUS, needsReloadMessage);
// Check whether minimizeDataMovement is set in TableConfig
- boolean isMinimizeDataMovement =
checkIsMinimizeDataMovement(rebalanceJobId, tableNameWithType, tableConfig);
- preCheckResult.put(IS_MINIMIZE_DATA_MOVEMENT,
String.valueOf(isMinimizeDataMovement));
+ RebalancePreCheckerResult isMinimizeDataMovementMessage =
checkIsMinimizeDataMovement(rebalanceJobId,
+ tableNameWithType, tableConfig);
+ preCheckResult.put(IS_MINIMIZE_DATA_MOVEMENT,
isMinimizeDataMovementMessage);
Review Comment:
```suggestion
preCheckResult.put(IS_MINIMIZE_DATA_MOVEMENT,
checkIsMinimizeDataMovement(rebalanceJobId, tableNameWithType, tableConfig));
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/DefaultRebalancePreChecker.java:
##########
@@ -105,37 +106,100 @@ private Boolean checkReloadNeededOnServers(String
rebalanceJobId, String tableNa
LOGGER.warn("Caught exception while trying to fetch reload status from
servers", e);
}
- return needsReload;
+ needsReloadMessage = needsReload == null ? "Could not determine needReload
status, run manually"
+ : !needsReload ? "No need to reload" : "Reload needed prior to running
rebalance";
+ RebalancePreCheckerResult.PreCheckStatus preCheckStatus = needsReload ==
null
+ ? RebalancePreCheckerResult.PreCheckStatus.ERROR
+ : !needsReload
+ ? RebalancePreCheckerResult.PreCheckStatus.PASS :
RebalancePreCheckerResult.PreCheckStatus.WARNING;
Review Comment:
Consider using if, else if, else to mark three conditions and set status and
message together for each condition? Would be easier to tell the outcome of
each cases
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/DefaultRebalancePreChecker.java:
##########
@@ -105,37 +106,100 @@ private Boolean checkReloadNeededOnServers(String
rebalanceJobId, String tableNa
LOGGER.warn("Caught exception while trying to fetch reload status from
servers", e);
}
- return needsReload;
+ needsReloadMessage = needsReload == null ? "Could not determine needReload
status, run manually"
+ : !needsReload ? "No need to reload" : "Reload needed prior to running
rebalance";
+ RebalancePreCheckerResult.PreCheckStatus preCheckStatus = needsReload ==
null
+ ? RebalancePreCheckerResult.PreCheckStatus.ERROR
+ : !needsReload
+ ? RebalancePreCheckerResult.PreCheckStatus.PASS :
RebalancePreCheckerResult.PreCheckStatus.WARNING;
+ return new RebalancePreCheckerResult(preCheckStatus, needsReloadMessage);
}
/**
* Checks if minimize data movement is set for the given table in the
TableConfig
*/
- private boolean checkIsMinimizeDataMovement(String rebalanceJobId, String
tableNameWithType,
+ private RebalancePreCheckerResult checkIsMinimizeDataMovement(String
rebalanceJobId, String tableNameWithType,
TableConfig tableConfig) {
LOGGER.info("Checking whether minimizeDataMovement is set for table: {}
with rebalanceJobId: {}", tableNameWithType,
rebalanceJobId);
try {
if (tableConfig.getTableType() == TableType.OFFLINE) {
- InstanceAssignmentConfig instanceAssignmentConfig =
-
InstanceAssignmentConfigUtils.getInstanceAssignmentConfig(tableConfig,
InstancePartitionsType.OFFLINE);
- return instanceAssignmentConfig.isMinimizeDataMovement();
+ boolean isInstanceAssignmentAllowed =
InstanceAssignmentConfigUtils.allowInstanceAssignment(tableConfig,
+ InstancePartitionsType.OFFLINE);
+ InstanceAssignmentConfig instanceAssignmentConfig;
+ String instanceAssignmentStatusMessage;
+ RebalancePreCheckerResult.PreCheckStatus preCheckStatus;
+ if (isInstanceAssignmentAllowed) {
+ instanceAssignmentConfig =
+
InstanceAssignmentConfigUtils.getInstanceAssignmentConfig(tableConfig,
InstancePartitionsType.OFFLINE);
+ instanceAssignmentStatusMessage =
instanceAssignmentConfig.isMinimizeDataMovement()
+ ? "minimizeDataMovement is enabled"
+ : "minimizeDataMovement is not enabled but instance assignment
is allowed";
+ preCheckStatus = instanceAssignmentConfig.isMinimizeDataMovement()
+ ? RebalancePreCheckerResult.PreCheckStatus.PASS :
RebalancePreCheckerResult.PreCheckStatus.WARNING;
+ } else {
+ instanceAssignmentStatusMessage = "Instance assignment not allowed,
no need for minimizeDataMovement";
+ preCheckStatus = RebalancePreCheckerResult.PreCheckStatus.PASS;
+ }
+ return new RebalancePreCheckerResult(preCheckStatus,
instanceAssignmentStatusMessage);
} else {
- InstanceAssignmentConfig instanceAssignmentConfigConsuming =
-
InstanceAssignmentConfigUtils.getInstanceAssignmentConfig(tableConfig,
InstancePartitionsType.CONSUMING);
+ boolean isInstanceAssignmentAllowedConsuming =
InstanceAssignmentConfigUtils.allowInstanceAssignment(
+ tableConfig, InstancePartitionsType.CONSUMING);
+ InstanceAssignmentConfig instanceAssignmentConfigConsuming = null;
+ if (isInstanceAssignmentAllowedConsuming) {
+ instanceAssignmentConfigConsuming =
+
InstanceAssignmentConfigUtils.getInstanceAssignmentConfig(tableConfig,
InstancePartitionsType.CONSUMING);
+ }
// For REALTIME tables need to check for both CONSUMING and COMPLETED
segments if relocation is enabled
if
(!InstanceAssignmentConfigUtils.shouldRelocateCompletedSegments(tableConfig)) {
- return instanceAssignmentConfigConsuming.isMinimizeDataMovement();
+ String instanceAssignmentStatusMessage;
+ RebalancePreCheckerResult.PreCheckStatus preCheckStatus;
+ if (isInstanceAssignmentAllowedConsuming) {
+ instanceAssignmentStatusMessage =
instanceAssignmentConfigConsuming.isMinimizeDataMovement()
+ ? "minimizeDataMovement is enabled"
+ : "minimizeDataMovement is not enabled but instance assignment
is allowed";
+ preCheckStatus =
instanceAssignmentConfigConsuming.isMinimizeDataMovement()
+ ? RebalancePreCheckerResult.PreCheckStatus.PASS :
RebalancePreCheckerResult.PreCheckStatus.WARNING;
+ } else {
+ instanceAssignmentStatusMessage = "Instance assignment not
allowed, no need for minimizeDataMovement";
+ preCheckStatus = RebalancePreCheckerResult.PreCheckStatus.PASS;
+ }
+ return new RebalancePreCheckerResult(preCheckStatus,
instanceAssignmentStatusMessage);
}
- InstanceAssignmentConfig instanceAssignmentConfigCompleted =
-
InstanceAssignmentConfigUtils.getInstanceAssignmentConfig(tableConfig,
InstancePartitionsType.COMPLETED);
- return instanceAssignmentConfigConsuming.isMinimizeDataMovement()
- && instanceAssignmentConfigCompleted.isMinimizeDataMovement();
+ boolean isInstanceAssignmentAllowedCompleted =
InstanceAssignmentConfigUtils.allowInstanceAssignment(
+ tableConfig, InstancePartitionsType.COMPLETED);
+ InstanceAssignmentConfig instanceAssignmentConfigCompleted = null;
+ if (isInstanceAssignmentAllowedCompleted) {
+ instanceAssignmentConfigCompleted =
+
InstanceAssignmentConfigUtils.getInstanceAssignmentConfig(tableConfig,
InstancePartitionsType.COMPLETED);
+ }
+
+ String instanceAssignmentStatusMessage;
+ RebalancePreCheckerResult.PreCheckStatus preCheckStatus;
+ if (!isInstanceAssignmentAllowedConsuming &&
!isInstanceAssignmentAllowedCompleted) {
+ instanceAssignmentStatusMessage = "Instance assignment not allowed,
no need for minimizeDataMovement";
+ preCheckStatus = RebalancePreCheckerResult.PreCheckStatus.PASS;
+ } else if (instanceAssignmentConfigConsuming != null &&
instanceAssignmentConfigCompleted != null) {
+ instanceAssignmentStatusMessage =
instanceAssignmentConfigConsuming.isMinimizeDataMovement()
+ && instanceAssignmentConfigCompleted.isMinimizeDataMovement()
+ ? "minimizeDataMovement is enabled"
+ : "minimizeDataMovement may not enabled for consuming or
completed but instance assignment is allowed "
+ + "for both";
+ preCheckStatus =
instanceAssignmentConfigConsuming.isMinimizeDataMovement()
+ && instanceAssignmentConfigCompleted.isMinimizeDataMovement()
+ ? RebalancePreCheckerResult.PreCheckStatus.PASS :
RebalancePreCheckerResult.PreCheckStatus.WARNING;
Review Comment:
Same here, group conditions together using if else?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/DefaultRebalancePreChecker.java:
##########
@@ -53,16 +53,18 @@ public void init(PinotHelixResourceManager
pinotHelixResourceManager, @Nullable
}
@Override
- public Map<String, String> check(String rebalanceJobId, String
tableNameWithType, TableConfig tableConfig) {
+ public Map<String, RebalancePreCheckerResult> check(String rebalanceJobId,
String tableNameWithType,
+ TableConfig tableConfig) {
LOGGER.info("Start pre-checks for table: {} with rebalanceJobId: {}",
tableNameWithType, rebalanceJobId);
- Map<String, String> preCheckResult = new HashMap<>();
+ Map<String, RebalancePreCheckerResult> preCheckResult = new HashMap<>();
// Check for reload status
- Boolean needsReload = checkReloadNeededOnServers(rebalanceJobId,
tableNameWithType);
- preCheckResult.put(NEEDS_RELOAD_STATUS, needsReload == null ? "error" :
String.valueOf(needsReload));
+ RebalancePreCheckerResult needsReloadMessage =
checkReloadNeededOnServers(rebalanceJobId, tableNameWithType);
+ preCheckResult.put(NEEDS_RELOAD_STATUS, needsReloadMessage);
Review Comment:
```suggestion
preCheckResult.put(NEEDS_RELOAD_STATUS,
checkReloadNeededOnServers(rebalanceJobId, tableNameWithType));
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/DefaultRebalancePreChecker.java:
##########
@@ -105,37 +106,100 @@ private Boolean checkReloadNeededOnServers(String
rebalanceJobId, String tableNa
LOGGER.warn("Caught exception while trying to fetch reload status from
servers", e);
}
- return needsReload;
+ needsReloadMessage = needsReload == null ? "Could not determine needReload
status, run manually"
+ : !needsReload ? "No need to reload" : "Reload needed prior to running
rebalance";
+ RebalancePreCheckerResult.PreCheckStatus preCheckStatus = needsReload ==
null
+ ? RebalancePreCheckerResult.PreCheckStatus.ERROR
+ : !needsReload
+ ? RebalancePreCheckerResult.PreCheckStatus.PASS :
RebalancePreCheckerResult.PreCheckStatus.WARNING;
+ return new RebalancePreCheckerResult(preCheckStatus, needsReloadMessage);
}
/**
* Checks if minimize data movement is set for the given table in the
TableConfig
*/
- private boolean checkIsMinimizeDataMovement(String rebalanceJobId, String
tableNameWithType,
+ private RebalancePreCheckerResult checkIsMinimizeDataMovement(String
rebalanceJobId, String tableNameWithType,
TableConfig tableConfig) {
LOGGER.info("Checking whether minimizeDataMovement is set for table: {}
with rebalanceJobId: {}", tableNameWithType,
rebalanceJobId);
try {
if (tableConfig.getTableType() == TableType.OFFLINE) {
- InstanceAssignmentConfig instanceAssignmentConfig =
-
InstanceAssignmentConfigUtils.getInstanceAssignmentConfig(tableConfig,
InstancePartitionsType.OFFLINE);
- return instanceAssignmentConfig.isMinimizeDataMovement();
+ boolean isInstanceAssignmentAllowed =
InstanceAssignmentConfigUtils.allowInstanceAssignment(tableConfig,
+ InstancePartitionsType.OFFLINE);
+ InstanceAssignmentConfig instanceAssignmentConfig;
+ String instanceAssignmentStatusMessage;
+ RebalancePreCheckerResult.PreCheckStatus preCheckStatus;
+ if (isInstanceAssignmentAllowed) {
+ instanceAssignmentConfig =
+
InstanceAssignmentConfigUtils.getInstanceAssignmentConfig(tableConfig,
InstancePartitionsType.OFFLINE);
+ instanceAssignmentStatusMessage =
instanceAssignmentConfig.isMinimizeDataMovement()
+ ? "minimizeDataMovement is enabled"
+ : "minimizeDataMovement is not enabled but instance assignment
is allowed";
+ preCheckStatus = instanceAssignmentConfig.isMinimizeDataMovement()
+ ? RebalancePreCheckerResult.PreCheckStatus.PASS :
RebalancePreCheckerResult.PreCheckStatus.WARNING;
+ } else {
+ instanceAssignmentStatusMessage = "Instance assignment not allowed,
no need for minimizeDataMovement";
+ preCheckStatus = RebalancePreCheckerResult.PreCheckStatus.PASS;
+ }
+ return new RebalancePreCheckerResult(preCheckStatus,
instanceAssignmentStatusMessage);
} else {
- InstanceAssignmentConfig instanceAssignmentConfigConsuming =
-
InstanceAssignmentConfigUtils.getInstanceAssignmentConfig(tableConfig,
InstancePartitionsType.CONSUMING);
+ boolean isInstanceAssignmentAllowedConsuming =
InstanceAssignmentConfigUtils.allowInstanceAssignment(
+ tableConfig, InstancePartitionsType.CONSUMING);
+ InstanceAssignmentConfig instanceAssignmentConfigConsuming = null;
+ if (isInstanceAssignmentAllowedConsuming) {
+ instanceAssignmentConfigConsuming =
+
InstanceAssignmentConfigUtils.getInstanceAssignmentConfig(tableConfig,
InstancePartitionsType.CONSUMING);
+ }
// For REALTIME tables need to check for both CONSUMING and COMPLETED
segments if relocation is enabled
if
(!InstanceAssignmentConfigUtils.shouldRelocateCompletedSegments(tableConfig)) {
- return instanceAssignmentConfigConsuming.isMinimizeDataMovement();
+ String instanceAssignmentStatusMessage;
+ RebalancePreCheckerResult.PreCheckStatus preCheckStatus;
+ if (isInstanceAssignmentAllowedConsuming) {
+ instanceAssignmentStatusMessage =
instanceAssignmentConfigConsuming.isMinimizeDataMovement()
+ ? "minimizeDataMovement is enabled"
+ : "minimizeDataMovement is not enabled but instance assignment
is allowed";
+ preCheckStatus =
instanceAssignmentConfigConsuming.isMinimizeDataMovement()
+ ? RebalancePreCheckerResult.PreCheckStatus.PASS :
RebalancePreCheckerResult.PreCheckStatus.WARNING;
Review Comment:
Same here, group conditions together using if else?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/DefaultRebalancePreChecker.java:
##########
@@ -105,37 +106,100 @@ private Boolean checkReloadNeededOnServers(String
rebalanceJobId, String tableNa
LOGGER.warn("Caught exception while trying to fetch reload status from
servers", e);
}
- return needsReload;
+ needsReloadMessage = needsReload == null ? "Could not determine needReload
status, run manually"
+ : !needsReload ? "No need to reload" : "Reload needed prior to running
rebalance";
+ RebalancePreCheckerResult.PreCheckStatus preCheckStatus = needsReload ==
null
+ ? RebalancePreCheckerResult.PreCheckStatus.ERROR
+ : !needsReload
+ ? RebalancePreCheckerResult.PreCheckStatus.PASS :
RebalancePreCheckerResult.PreCheckStatus.WARNING;
+ return new RebalancePreCheckerResult(preCheckStatus, needsReloadMessage);
}
/**
* Checks if minimize data movement is set for the given table in the
TableConfig
*/
- private boolean checkIsMinimizeDataMovement(String rebalanceJobId, String
tableNameWithType,
+ private RebalancePreCheckerResult checkIsMinimizeDataMovement(String
rebalanceJobId, String tableNameWithType,
TableConfig tableConfig) {
LOGGER.info("Checking whether minimizeDataMovement is set for table: {}
with rebalanceJobId: {}", tableNameWithType,
rebalanceJobId);
try {
if (tableConfig.getTableType() == TableType.OFFLINE) {
- InstanceAssignmentConfig instanceAssignmentConfig =
-
InstanceAssignmentConfigUtils.getInstanceAssignmentConfig(tableConfig,
InstancePartitionsType.OFFLINE);
- return instanceAssignmentConfig.isMinimizeDataMovement();
+ boolean isInstanceAssignmentAllowed =
InstanceAssignmentConfigUtils.allowInstanceAssignment(tableConfig,
+ InstancePartitionsType.OFFLINE);
+ InstanceAssignmentConfig instanceAssignmentConfig;
+ String instanceAssignmentStatusMessage;
+ RebalancePreCheckerResult.PreCheckStatus preCheckStatus;
+ if (isInstanceAssignmentAllowed) {
+ instanceAssignmentConfig =
+
InstanceAssignmentConfigUtils.getInstanceAssignmentConfig(tableConfig,
InstancePartitionsType.OFFLINE);
+ instanceAssignmentStatusMessage =
instanceAssignmentConfig.isMinimizeDataMovement()
+ ? "minimizeDataMovement is enabled"
+ : "minimizeDataMovement is not enabled but instance assignment
is allowed";
+ preCheckStatus = instanceAssignmentConfig.isMinimizeDataMovement()
+ ? RebalancePreCheckerResult.PreCheckStatus.PASS :
RebalancePreCheckerResult.PreCheckStatus.WARNING;
Review Comment:
Same here, group conditions together using if else?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/RebalancePreCheckerResult.java:
##########
@@ -0,0 +1,68 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller.helix.core.rebalance;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonInclude;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import javax.annotation.Nullable;
+
+
+/**
+ * Holds the pre-check result for each pre-check performed as part of
RebalancePreChecker
+ */
+@JsonIgnoreProperties(ignoreUnknown = true)
+@JsonInclude(JsonInclude.Include.NON_NULL)
+public class RebalancePreCheckerResult {
+
+ @JsonInclude(JsonInclude.Include.NON_NULL)
+ private final PreCheckStatus _preCheckStatus;
+ @JsonInclude(JsonInclude.Include.NON_NULL)
+ private final String _message;
+
+ /**
+ * Constructor for RebalancePreCheckerResult
+ * @param preCheckStatus server related summary information
+ * @param message segment related summary information
+ */
+ @JsonCreator
+ public RebalancePreCheckerResult(@JsonProperty("preCheckStatus")
PreCheckStatus preCheckStatus,
+ @JsonProperty("message") @Nullable String message) {
+ _preCheckStatus = preCheckStatus;
+ _message = message;
+ }
+
Review Comment:
```suggestion
public static RebalancePreCheckerResult pass(@Nullable String msg) {
return new RebalancePreCheckerResult(PreCheckStatus.PASS, msg);
}
public static RebalancePreCheckerResult warn(@Nullable String msg) {
return new RebalancePreCheckerResult(PreCheckStatus.WARNING, msg);
}
public static RebalancePreCheckerResult error(@Nullable String msg) {
return new RebalancePreCheckerResult(PreCheckStatus.ERROR, msg);
}
```
Do you think adding methods like this helps simplify the return statements
in pre-checkers? This way there's almost no need to set the variable
`precheckStatus`
--
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
