yashmayya commented on code in PR #16615:
URL: https://github.com/apache/pinot/pull/16615#discussion_r2280228320
##########
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:
`skipping` is a little vague here and could be misinterpreted as the
rebalance itself being skipped?
##########
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:
nit: `rebalance summary is null` -> `rebalance summary is disabled` since
this is user facing?
--
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]
