somandal commented on code in PR #15360:
URL: https://github.com/apache/pinot/pull/15360#discussion_r2012510847
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/DefaultRebalancePreChecker.java:
##########
@@ -97,7 +98,8 @@ public Map<String, RebalancePreCheckerResult>
check(PreCheckContext preCheckCont
* TODO: Add an API to check for whether segments in deep store are up to
date with the table configs and schema
* and add a pre-check here to call that API.
*/
- private RebalancePreCheckerResult checkReloadNeededOnServers(String
rebalanceJobId, String tableNameWithType) {
+ private RebalancePreCheckerResult checkReloadNeededOnServers(String
rebalanceJobId, String tableNameWithType,
Review Comment:
`preCheckContext.getCurrentAssignment()` is indeed from the IS
The idea here is we should run this pre-check prior to starting a rebalance.
There could be cases where EV might not fully match, but in that scenario we
won't query the servers in EV that aren't in IS too and might miss out on some
results. Today the `needReload` API doesn't handle this either since it doesn't
check either IS or EV.
This change is still more correct that what happens today, where we fetch
the server list from the instances tagged, which might be completely different
than what is in IS (e.g. if we are migrating the tenant, the newly tagged
instances might not have any segments for the table)
This check is kind of a best effort check. Do you see any specific concern
with not checking the EV too?
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
