yashmayya commented on code in PR #19123:
URL: https://github.com/apache/pinot/pull/19123#discussion_r3678862937
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/RebalancePreChecker.java:
##########
@@ -41,11 +42,13 @@ class PreCheckContext {
private final TableSizeReader.TableSubTypeSizeDetails
_tableSubTypeSizeDetails;
private final RebalanceConfig _rebalanceConfig;
private final RebalanceSummaryResult _rebalanceSummaryResult;
+ private final Map<String, Set<String>> _providedTierToSegmentsMap;
public PreCheckContext(String rebalanceJobId, String tableNameWithType,
TableConfig tableConfig,
Review Comment:
This is a public nested type on a public interface. Mind keeping the old
8-arg constructor as an overload that delegates with `null`? Cheap insurance
for anything outside the reactor that builds one.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/DefaultRebalancePreChecker.java:
##########
@@ -274,9 +272,18 @@ private RebalancePreCheckerResult
checkIsMinimizeDataMovement(TableConfig tableC
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) {
+ /**
+ * Estimates whether the servers of the target assignment stay within the
disk utilization threshold, based on the
+ * average segment size and the number of segments added to (and, unless
checking for the worst case, removed from)
+ * each server. Every segment is assumed to take up disk space on each
server it is assigned to. Downstream projects
Review Comment:
Worth spelling out that the other half of this assumption lives in
`getAverageSegmentSize`, which divides the whole reported per-replica size by
the segment count. A subclass that excludes some segments from the assignment
still inherits an average computed over all of them, including the excluded
bytes — and it can't override that without reimplementing this method. Right
now a downstream reader of this javadoc would reasonably think excluding
segments from the assignment is sufficient.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/DefaultRebalancePreChecker.java:
##########
@@ -274,9 +272,18 @@ private RebalancePreCheckerResult
checkIsMinimizeDataMovement(TableConfig tableC
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) {
+ /**
+ * Estimates whether the servers of the target assignment stay within the
disk utilization threshold, based on the
+ * average segment size and the number of segments added to (and, unless
checking for the worst case, removed from)
+ * each server. Every segment is assumed to take up disk space on each
server it is assigned to. Downstream projects
+ * where that does not hold (e.g. because a segment can be stored outside of
the server, as indicated by
+ * {@link TierConfig#getTierBackend()}) can override this.
+ */
+ protected RebalancePreCheckerResult checkDiskUtilization(PreCheckContext
preCheckContext, double threshold,
Review Comment:
Passing the whole context in only to destructure three fields out of it
right below means a subclass that wants to influence two of them has to rebuild
the entire 9-arg `PreCheckContext`. That's exactly what the StarTree override
ends up doing.
Could we keep the original parameter list and add a narrow hook instead,
something like `protected Set<String>
getSegmentsExcludedFromDiskEstimate(PreCheckContext)`? Subclasses then can't
accidentally drop a field when this class gains one, and the downstream code
shrinks to one method.
--
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]
