klsince commented on code in PR #15317:
URL: https://github.com/apache/pinot/pull/15317#discussion_r2017149571
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/DefaultRebalancePreChecker.java:
##########
@@ -279,6 +299,43 @@ private RebalancePreCheckerResult
checkDiskUtilization(Map<String, Map<String, S
: RebalancePreCheckerResult.error(message.toString());
}
+ private RebalancePreCheckerResult checkRebalanceConfig(RebalanceConfig
rebalanceConfig, TableConfig tableConfig,
+ Map<String, Map<String, String>> currentAssignment, Map<String,
Map<String, String>> targetAssignment) {
+ StringBuilder message = new StringBuilder();
Review Comment:
nit: it might be easier to collect all warning into a List<String> and then
do a StringUtils.Join() in the end, so no need to worry about the format while
collecting those warning strings.
Also, instead of joining all strings into one string in the end, it might be
more readable to simply keep `message` field as a List field in the final
output JSON msg.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/DefaultRebalancePreChecker.java:
##########
@@ -135,26 +143,29 @@ private RebalancePreCheckerResult
checkReloadNeededOnServers(String rebalanceJob
* Checks if minimize data movement is set for the given table in the
TableConfig
*/
private RebalancePreCheckerResult checkIsMinimizeDataMovement(String
rebalanceJobId, String tableNameWithType,
- TableConfig tableConfig) {
+ TableConfig tableConfig, RebalanceConfig rebalanceConfig) {
LOGGER.info("Checking whether minimizeDataMovement is set for table: {}
with rebalanceJobId: {}", tableNameWithType,
rebalanceJobId);
try {
if (tableConfig.getTableType() == TableType.OFFLINE) {
boolean isInstanceAssignmentAllowed =
InstanceAssignmentConfigUtils.allowInstanceAssignment(tableConfig,
InstancePartitionsType.OFFLINE);
- RebalancePreCheckerResult rebalancePreCheckerResult;
if (isInstanceAssignmentAllowed) {
+ if (rebalanceConfig.getMinimizeDataMovement() ==
RebalanceConfig.MinimizeDataMovementOptions.ENABLE) {
+ return RebalancePreCheckerResult.pass("minimizeDataMovement is
enabled");
+ }
InstanceAssignmentConfig instanceAssignmentConfig =
InstanceAssignmentConfigUtils.getInstanceAssignmentConfig(tableConfig,
InstancePartitionsType.OFFLINE);
- rebalancePreCheckerResult =
instanceAssignmentConfig.isMinimizeDataMovement()
- ? RebalancePreCheckerResult.pass("minimizeDataMovement is
enabled")
- : RebalancePreCheckerResult.warn("minimizeDataMovement is not
enabled but instance assignment "
- + "is allowed");
- } else {
- rebalancePreCheckerResult =
- RebalancePreCheckerResult.pass("Instance assignment not allowed,
no need for minimizeDataMovement");
+ if (instanceAssignmentConfig.isMinimizeDataMovement()) {
+ return rebalanceConfig.getMinimizeDataMovement() ==
RebalanceConfig.MinimizeDataMovementOptions.DISABLE
+ ? RebalancePreCheckerResult.warn("minimizeDataMovement is
enabled in table config but it's overridden "
+ + "with disabled")
+ : RebalancePreCheckerResult.pass("minimizeDataMovement is
enabled");
Review Comment:
nit: fold this line up?
--
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]
