somandal commented on code in PR #16615:
URL: https://github.com/apache/pinot/pull/16615#discussion_r2280349393
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/DefaultRebalancePreChecker.java:
##########
@@ -412,16 +412,21 @@ private RebalancePreCheckerResult
checkRebalanceConfig(RebalanceConfig rebalance
}
// --- Batch size per server recommendation check using summary ---
- int maxSegmentsToAddOnServer =
rebalanceSummaryResult.getSegmentInfo().getMaxSegmentsAddedToASingleServer();
- int batchSizePerServer = rebalanceConfig.getBatchSizePerServer();
- if (maxSegmentsToAddOnServer > SEGMENT_ADD_THRESHOLD) {
- if (batchSizePerServer == RebalanceConfig.DISABLE_BATCH_SIZE_PER_SERVER
- || batchSizePerServer > RECOMMENDED_BATCH_SIZE) {
- pass = false;
- warnings.add("Number of segments to add to a single server (" +
maxSegmentsToAddOnServer + ") is high (>"
- + SEGMENT_ADD_THRESHOLD + "). It is recommended to set
batchSizePerServer to " + RECOMMENDED_BATCH_SIZE
- + " or lower to avoid excessive load on servers.");
+ if (rebalanceSummaryResult != null) {
+ int maxSegmentsToAddOnServer =
rebalanceSummaryResult.getSegmentInfo().getMaxSegmentsAddedToASingleServer();
+ int batchSizePerServer = rebalanceConfig.getBatchSizePerServer();
+ if (maxSegmentsToAddOnServer > SEGMENT_ADD_THRESHOLD) {
+ if (batchSizePerServer == RebalanceConfig.DISABLE_BATCH_SIZE_PER_SERVER
+ || batchSizePerServer > RECOMMENDED_BATCH_SIZE) {
+ pass = false;
+ warnings.add("Number of segments to add to a single server (" +
maxSegmentsToAddOnServer + ") is high (>"
+ + SEGMENT_ADD_THRESHOLD + "). It is recommended to set
batchSizePerServer to " + RECOMMENDED_BATCH_SIZE
+ + " or lower to avoid excessive load on servers.");
+ }
}
+ } else {
+ pass = false;
+ warnings.add("Could not assess batchSizePerServer recommendation as
rebalance summary is null");
}
Review Comment:
good point, we don't allow summary to be disabled when pre-checks are
enabled, but it can still be null if an exception is thrown. updated the
comment to reflect that and added a comment as well
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -355,18 +362,28 @@ private RebalanceResult doRebalance(TableConfig
tableConfig, RebalanceConfig reb
// Calculate summary here itself so that even if the table is already
balanced, the caller can verify whether that
// is expected or not based on the summary results
- RebalanceSummaryResult summaryResult =
- calculateDryRunSummary(currentAssignment, targetAssignment,
tableNameWithType, tableSubTypeSizeDetails,
- tableConfig, tableRebalanceLogger);
+ RebalanceSummaryResult summaryResult = null;
+ if (!disableSummary) {
+ try {
+ summaryResult = calculateRebalanceSummary(currentAssignment,
targetAssignment, tableNameWithType,
+ tableSubTypeSizeDetails, tableConfig, tableRebalanceLogger);
+ } catch (Exception e) {
+ tableRebalanceLogger.warn("Caught exception while trying to run the
rebalance summary, skipping", e);
Review Comment:
good point, updated
--
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]
