J-HowHuang commented on PR #16341: URL: https://github.com/apache/pinot/pull/16341#issuecomment-3084812946
@somandal The `downloadUrl` change could only be from empty (peer download implied) to deep store url right? If we check all the segments once before the rebalance start, instead of every step when next assignment is called, we might get false positive for `hasDataLossRisk` but not false negative. This should be acceptable? I don't know how much this could help in terms of the performance, but it can then reduce the access to one time. Nevertheless if the overhead of the ZK metadata access for each segment is not that significant I think it shouldn't be a problem. Also, do we need a `forceDowntime` flag if we already warned the user by pre-check? Imo if they did a non-dry-run, they should've learned the risk of doing so, therefore this second security door seems a bit redundant to me. Plus we have `DataLossRiskAssessor` to prevent risk, so setting `forceDowntime` to `true` doesn't even make the rebalance operation from safe to unsafe. -- 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
