mwkang commented on PR #8778: URL: https://github.com/apache/storm/pull/8778#issuecomment-4644799978
thanks for the review **`#1` (dead public method)** Removed `Cluster.hasIdleSupervisorReusableBy`. Production already gates per topology through the private `topologyCanReuseIdleSupervisor`, so the public method was test-only and could drift. The six tests that called it now assert the observable relocation effect of `redistributeOntoIdleSupervisors` instead, and its design javadoc moved onto `isIdleSupervisorAvailableForEvenRebalance`. The only non-comment change in main is that deletion — the scheduling path itself is byte-identical. **Test coverage** — added the two missing cases, both mutation-checked: - `twoIdleSupervisors_budgetScalesWithIdleSupervisorCount`: two simultaneously-idle supervisors so `floor(8/4) * 2 = 4` exercises the `* idleSupervisorCount` term (dropping it computes 2 and the test fails). - `maxFreePerTopologyClampsBelowEvenBudget`: even-budget 2 vs `maxFree = 1`, so `Math.min(target, maxFree)` is the strictly binding constraint (removing it computes 2 and the test fails). **Nits** — **`#2`** commented both uptime defaults (`Long.MAX_VALUE` in the bare constructors vs `0L` in `Nimbus.supervisorUptimeSecs`, the conservative production path); **`#3`** javadoc now notes the cap uses the declared `numWorkers` and is an upper bound the donor guard may keep actual moves below; **`#4`** documented the intentionally non-atomic freeSlot/assign ordering (target is a pre-verified idle slot); **`#5`** dropped `@VisibleForTesting` since it's a genuine cross-class entry point. -- 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]
