yashmayya commented on code in PR #19123:
URL: https://github.com/apache/pinot/pull/19123#discussion_r3678862934
##########
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.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/RebalancePreChecker.java:
##########
@@ -87,6 +91,15 @@ public RebalanceConfig getRebalanceConfig() {
public RebalanceSummaryResult getRebalanceSummaryResult() {
return _rebalanceSummaryResult;
}
+
+ /**
+ * Returns the tier name to segments map computed while updating the
target tiers of this rebalance, or
Review Comment:
Two things I'd add here:
- The map can also be **empty**, not just null — `updateTargetTier` returns
an empty map when the table has tier configs but no segment currently matches a
tier. Callers need to handle both.
- These are the **target** tiers just persisted to ZK, not where the
segments live today. Worth saying so, because for a segment being promoted
local -> remote the two differ and a consumer might assume otherwise.
Also maybe mention that `updateTargetTier` defaults to false, so a plain
`dryRun=true&preChecks=true` request gets null here.
##########
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/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;
Review Comment:
Nothing in OSS exercises this field — `TableRebalancer` is the only thing
that constructs a `PreCheckContext`, and no test does. Since the whole point of
the field is to be read by a downstream pre-checker, a small test that builds a
context with a tier map and asserts it survives to the pre-checker would keep
the plumbing from silently regressing.
--
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]
