Jackie-Jiang opened a new pull request, #19835: URL: https://github.com/apache/pinot/pull/19835
## Summary Table config validation (`TableConfigValidationUtils.validateInstanceAssignment`) constructed a `TableRebalancer`, with no observer, metrics, pre-checker or segment manager, only to run `getInstancePartitionsMap` in dry-run mode. The branching that resolves a table's instance partitions, assignment from the instance configs, pre-configured instance partitions renamed for the table, or pre-configured instance partitions used as the server pool of the mirror server set selector, was duplicated in `TableRebalancer.getInstancePartitions`, `PinotHelixResourceManager.assignInstances` and `PinotInstanceAssignmentRestletResource`, with validation borrowing the rebalancer's copy. This PR: - Adds `InstanceAssignmentUtils.computeInstancePartitions`, which holds that branching and never persists, and uses it from all four places. Validation computes the instance partitions of the relevant types directly (OFFLINE, or CONSUMING plus COMPLETED when completed segments are relocated), falling back to the default instance partitions where instance assignment is not allowed, exactly as the dry run did. - Replaces `InstanceAssignmentDriver` with the static `InstanceAssignmentUtils`, taking the table config as the first parameter. The driver's only state was the table config, every caller built one for a single call, and nothing subclassed or mocked it. No StarTree usages. - Reads the instance configs once per instance partitions computation in `TableRebalancer`, through a memoized supplier, instead of once per instance partitions type and once per tier. - Uses `getPropertyStore()` consistently in `PinotInstanceAssignmentRestletResource`. ## Behavior The computed instance partitions are identical in every path. Validation previously forced minimize-data-movement off and now uses the default; that is equivalent because it passes no existing instance partitions, and the selectors derive the flag as `... && existingInstancePartitions != null`. The observable differences are logging (the "(based on …)" and "(referencing …)" details on persisted pre-configured partitions are gone), fewer ZK reads of instance configs during a rebalance, and one extra read of the existing instance partitions on the REST endpoint's plain pre-configured path, where it is unused. `InstanceAssignmentTest` is converted to the static calls and gains `testComputeInstancePartitions`, which covers the three branches against the existing `assignInstances` overloads with a mocked property store. The touched files were also reformatted, so the diff is larger than the logic change. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
